bugThe FreeType Project - Bugs: bug #55235, CMake update build scripts to...

 
 

bug #55235: CMake update build scripts to support all features

Submitted by:  rim <rim>
Submitted on:  Mon 17 Dec 2018 02:50:38 AM UTC  
 
Severity:  3 - Normal Item Group:  None
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Planned Release:  None

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Wed 19 Jun 2019 10:43:16 PM UTC, comment #21:

> "Is this a CMake bug?" - I do not remember, but I remember that BZip2 does not work like all other staff, and this is the best solution that I found.


But it does work in the setup in git master on my Linux system? I am strongly opposed to adding platform-specific hacks. If it turns out to be a CMake bug, the bug must be fixed, not the CMakeLists.txt.

> "Does check_c_compiler_flag by itself not suffice?" - yes, it not enough.


I mean check with check_c_compiler_flag and then add it to the target via target_compile_options.

> "Why is this necessary?" - this supress some cmake warnings at least on freebsd, that adds extra spaces to CFLAGS.


I have not seen these warnings on my system, please check on your system where those spaces are coming from. The CMakeLists.txt file is not the place to scrub input.

> "This looks like it's duplicating what is possible with" - as I remember they in different format and, probably, different content.


Please use the "standard" CMake way to do things. I know that CMake has been anarchy for years, but there are efforts to make this less hare-brained. See e.g. https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1#modules. If we need a newer CMake to do this cleanly, then I'm all for it. Also, see further guides at https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1#sources.

Somewhere, there is an explanation for why you should not do freetype_static and freetype_shared (one reason is that it makes handling easier for projects that include FreeType; CMake will do the right thing with the library depending on what the parent project needs). More info: https://cgold.readthedocs.io/en/latest/tutorials/libraries/static-shared.html. This says that autotools compiles twice under the hood anyway, so...

> Good and proper way is add ftoption.h.cmake, ftconfig.in.cmake, freetype2.cmake, freetype-config.cmake and do not use hacks for pathing autotools files.


The problem is this: CMake is an unofficial build system for FreeType, so Werner won't do much with it and someone has to maintain it to keep it useful. Your patch already increases the maintenance burden, splitting off duplicate files does so even more.

Remember that you are asking a small handful of FreeType contributors to maintain your changeset, and time for that is sparse to nonexistent. I too would prefer we ditch the custom build system and go full-on CMake or (better yet) Meson, but unless we get Werner on board, we can forget about it.

What would go through is a change to the default build system so that there is one set of files that works for both systems.

> options (all options from autotools)


Mh, OK.

> No auto detection for dep libs!!! All off by default.


I am curious, where is that behavior the default? The custom build system certainly does autodetection. Do you have "best practice" guides for this somewhere?

> Options for hardening


I am not convinced that these need to be in the upstream build definition. They are also GCC specific from the looks of it. Please do all of these downstream, or on your local system.

Nikolaus Waxweiler <morksig>
Project Member
Wed 19 Jun 2019 08:44:40 PM UTC, comment #20:

"Is this a CMake bug?" - I do not remember, but I remember that BZip2 does not work like all other staff, and this is the best solution that I found.

"Does check_c_compiler_flag by itself not suffice?" - yes, it not enough.

"Please do not change variables with wide scope like CMAKE_C_FLAGS. Always use target_*." - ok, I will see.

"I don't see this in the default build system's output, so please don't." - I will make few options for hardening, off by default.

"Why is this necessary?" - this supress some cmake warnings at least on freebsd, that adds extra spaces to CFLAGS.

"What's the logic here? The builds/unix/ftsystem.c file should have indeed been used on Unix, but Unix has mmap so this won't be compiled on Unix?" -
This will build on all systems that have mmap(), see:
# Check platform API.
if (NOT FT_DISABLE_MMAP)
  check_function_exists(mmap HAVE_MMAP)
endif ()

"The static/shared nature of the library target can be controlled by the BUILD_SHARED_LIBS variable. What needs a seperate definition?" - this all to able to build static and dynamic lib at same time/one pass.

"This looks like it's duplicating what is possible with" - as I remember they in different format and, probably, different content.

"Please don't introduce a second freetype-config/freetype2.cmake file" - you wants cmake but autotools, that way to get ugly broken cmake.
Good and proper way is add ftoption.h.cmake, ftconfig.in.cmake, freetype2.cmake, freetype-config.cmake and do not use hacks for pathing autotools files.
Keep patching autotool files from cmake - way to hell :)

"Can you please list the changes to the build and installation behavior you need to use CMake as the sole build system to build packages, etc.? Then we can distil what changes are actually needed." - I already done all features that I need.

1. Proper generation freetype.pc and installation it.

2. Build static and dynamic libs using one cmake call and without recompilation of sources.

3. options (all options from autotools) :
FT_WITH_ZLIB - "Use system zlib instead of internal library."
FT_WITH_BZIP2 - "Support bzip2 compressed fonts."
FT_WITH_PNG - "Support PNG compressed OpenType embedded bitmaps."
FT_WITH_HARFBUZZ - "Improve auto-hinting of OpenType fonts."
FT_WITH_CONFIG - "Install freetype-config."
FT_DISABLE_MMAP - "do not check mmap() and do not use."
ENABLE_FULL_DEBUG - "Build with all possible debug."

4. No auto detection for dep libs!!! All off by default.

5. Options for hardening. (that was done without options in ferst versions)
"-fsanitize=safe-stack"
"-fsanitize=cfi"
"-mretpoline"
"-mfunction-return=thunk"
"-mindirect-branch=thunk"
"-fwrapv"
"-fPIE" + "-pie"
"-Wl,-z,retpolineplt"
"-Wl,-z,relro"
"-Wl,-z,now"
"-Wl,-z,noexecstack"

rim <rim>
Wed 19 Jun 2019 07:39:16 AM UTC, comment #19:

"What about  the opposite? A user  removes the comment by hand and cmake fails  to find it. Cmake should put the cheese moment back." - Build system should not care about all things that user can do after dist unpacked, it only build unmodified sources.
If users do some modifications by hands - then user should handle all build errors.

rim <rim>
Mon 17 Jun 2019 04:15:52 AM UTC, comment #18:

Re: #16
ftoption.h is there to mess with. Hence its name. This is where people choose features. When we have decided to override them depending on availability, we should let them know.

Alexei Podtelezhnikov <podtelez>
Project Member
Sun 16 Jun 2019 05:55:32 PM UTC, comment #17:

So I finally took the time to look at the proposed changes and I am unhappy with them, they increase the complexity of the build definition without a clear benefit.

if (FT_WITH_BZIP2)
  find_package(BZip2 REQUIRED)
  #list(APPEND CMAKE_REQUIRED_LIBRARIES_PRIV bzip2)
  set(BZIP2_LDFLAGS "-lbz2") # pkg_check_modules() always fail on BZip2.
endif ()

Is this a CMake bug? Please avoid verbatim linker and compiler flags and leave that to CMake if at all possible. If you find that something specific is not available in the minimum required CMake version, we can always increase the minimum required version.

macro(try_c_flag prop flag)
  # Try flag once on the C compiler.
  check_c_compiler_flag("-Werror ${flag}" C_FLAG_${prop})
  if (C_FLAG_${prop})
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${flag}")
  endif ()
endmacro()

macro(try_linker_flag prop flag)
  # Check with the C compiler.
  set(CMAKE_REQUIRED_FLAGS ${flag})
  check_c_compiler_flag(${flag} LINKER_FLAG_${prop})
  set(CMAKE_REQUIRED_FLAGS "")
  if (LINKER_FLAG_${prop})
    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${flag}")
    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${flag}")
    set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${flag}")
  endif ()
endmacro()

Does check_c_compiler_flag by itself not suffice? Please do not change variables with wide scope like CMAKE_C_FLAGS. Always use target_*.

Effective Modern CMake: https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1

# Hardening flags
try_c_flag(FPIE "-fPIE")
try_linker_flag(PIE "-pie")

I don't see this in the default build system's output, so please don't. This is something that downstream must decide.

# Silently strip whitespace.
string(STRIP "${CMAKE_C_FLAGS}" CMAKE_C_FLAGS)
string(STRIP "${CMAKE_EXE_LINKER_FLAGS}" CMAKE_EXE_LINKER_FLAGS)
string(STRIP "${CMAKE_MODULE_LINKER_FLAGS}" CMAKE_MODULE_LINKER_FLAGS)
string(STRIP "${CMAKE_SHARED_LINKER_FLAGS}" CMAKE_SHARED_LINKER_FLAGS)

Why is this necessary?

if (HAVE_MMAP)
  list(APPEND BASE_SRCS "src/base/ftsystem.c")
else ()
  # xxx: unix, amiga, vms.
  list(APPEND BASE_SRCS "builds/unix/ftsystem.c")
endif ()

What's the logic here? The builds/unix/ftsystem.c file should have indeed been used on Unix, but Unix has mmap so this won't be compiled on Unix?

add_library(freetype_static STATIC $<TARGET_OBJECTS:freetype>)
target_link_libraries(freetype_static PRIVATE
  ${BZIP2_LIBRARIES}
  ${ZLIB_LIBRARIES}
  ${PNG_LIBRARIES}
  ${HARFBUZZ_LIBRARIES})
set_target_properties(freetype_static PROPERTIES OUTPUT_NAME "freetype")

The static/shared nature of the library target can be controlled by the BUILD_SHARED_LIBS variable. What needs a seperate definition?

if (NOT SKIP_INSTALL_LIBRARIES AND NOT SKIP_INSTALL_ALL)
  if (UNIX)
    # Create libs LD flags list.
    set(LIBS_LDFLAGS_LIST ${BZIP2_LDFLAGS})
    foreach (__m ${CMAKE_REQUIRED_LIBRARIES_PRIV})
      string(TOUPPER ${__m} __prefix)
      pkg_check_modules(${__prefix} QUIET ${__m})
      list(APPEND LIBS_LDFLAGS_LIST ${${__prefix}_LDFLAGS})
      list(APPEND LIBS_LDFLAGS_LIST ${${__prefix}_LDFLAGS_OTHER})
    endforeach ()
    list(REMOVE_DUPLICATES LIBS_LDFLAGS_LIST)
    string(REPLACE ";" " " LIBS_LDFLAGS_LIST "${LIBS_LDFLAGS_LIST}")

This looks like it's duplicating what is possible with

target_link_libraries(freetype PRIVATE ${ZLIB_LIBRARIES})
target_include_directories(freetype PRIVATE ${ZLIB_INCLUDE_DIRS})

What keeps you from using the exported defines by FindPackage?

# Generate the freetype-config file.

Please don't introduce a second freetype-config/freetype2.cmake file, keep the hacks to modify the existing ones. I don't like it either and would prefer if the standard file would be easily filterable by configure_file, but that would require changes in the default build system and I just don't have the time for that. Maybe you can find a way to filter the file first to turn it into something that configure_file can work with.

Can you please list the changes to the build and installation behavior you need to use CMake as the sole build system to build packages, etc.? Then we can distil what changes are actually needed.

Nikolaus Waxweiler <morksig>
Project Member
Sun 16 Jun 2019 05:40:36 PM UTC, comment #16:

Umm... when would that happen and is it a valid use case to mess with the build system output outside the build system?

Nikolaus Waxweiler <morksig>
Project Member
Sun 16 Jun 2019 04:35:48 PM UTC, comment #15:

What about  the opposite? A user  removes the comment by hand and cmake fails  to find it. Cmake should put the cheese moment back.

I this is what configure does.

Alexei Podtelezhnikov <podtelez>
Project Member
Sun 16 Jun 2019 02:50:34 PM UTC, comment #14:

I have not tested this patch, my comment was referring to the existing CMakeLists.txt file in git master.

CMake caches things it finds, so if you have e.g. the libpng headers installed, CMake will find them and make a note somewhere in the build folder. It will not re-search the existence of the headers/libs on the next `cmake` run, so ftoptions.h will carry the un-commented #define because CMake thinks the headers still exist. If you want it to search for stuff again, either remove and re-create the build folder or do `cmake -U "*" .` in the build folder to clear the cache. This is a CMake feature, I guess.

I just tested this in a fresh Ubuntu 18.04 container. Without any of the -dev packages installed, all the external dependency #defines are commented. When I install libpng-dev, the next run of `cmake` finds the library (and zlib) and un-comments the #define. Uninstalling libpng-dev and running `cmake` again results in no change, the PNG #define is un-commented. Running `cmake -U "*" .` and running `cmake` again correctly does not find the library and produces a commented #define.

So I think this works as intended.

Nikolaus Waxweiler <morksig>
Project Member
Thu 13 Jun 2019 09:52:47 PM UTC, comment #13:

Not all, ~half.
There is many changes, probably it will be easy to read after you apply patch.

rim <rim>
Thu 13 Jun 2019 09:16:10 PM UTC, comment #12:

It is too hard to read a diff that rewrites the whole file. Please attached the actual file.

Alexei Podtelezhnikov <podtelez>
Project Member
Thu 13 Jun 2019 08:28:57 PM UTC, comment #11:

Add options:
FT_DEBUG_MEMORY - "Memory Debugging."
FT_DEBUG_LEVEL_ERROR - "Build in error mode."
FT_DEBUG_LEVEL_TRACE - "Build in trace mode."
PCF_CONFIG_OPTION_LONG_FAMILY_NAMES - "Prepend the foundry name (plus a space) to the family name."
AF_CONFIG_OPTION_TT_SIZE_METRICS - "Use TrueType-like size metrics for 'light' auto-hinting."

Update comments.

Leave only CFLAGS:-fPIE and LD_FLAGS:-pie

(file #47095)

rim <rim>
Thu 13 Jun 2019 02:28:28 AM UTC, comment #10:

Nikolaus, are you sure that you use latest attached patch?

By default
FT_WITH_ZLIB - "Use system zlib instead of internal library."
FT_WITH_BZIP2 - "Support bzip2 compressed fonts."
FT_WITH_PNG - "Support PNG compressed OpenType embedded bitmaps."
FT_WITH_HARFBUZZ - "Improve auto-hinting of OpenType fonts."
FT_WITH_CONFIG - "Install freetype-config."
FT_DISABLE_MMAP - "do not check mmap() and do not use."
is OFF.

That mean that if you not set FT_WITH_ZLIB=yes then CMAKE_DISABLE_FIND_PACKAGE_ZLIB has no affect.

These options maps to:
FT_CONFIG_OPTION_SYSTEM_ZLIB
FT_CONFIG_OPTION_USE_BZIP2
FT_CONFIG_OPTION_USE_PNG
FT_CONFIG_OPTION_USE_HARFBUZZ

These options can be only uncommented in ftoption.h.
If you think that this is not enough I can add code to ensure that option is commented then it off.
This behavior was in original cmake file, and it always works for because in release archive these options commented.

rim <rim>
Wed 12 Jun 2019 09:15:57 PM UTC, comment #9:

It should also be able to put the comment back if the package is not found. This is what configure does.

Alexei Podtelezhnikov <podtelez>
Project Member
Wed 12 Jun 2019 08:50:28 PM UTC, comment #8:

Err, fix what exactly?

Doing

cmake -E make_directory build && cmake -E chdir build cmake -DCMAKE_DISABLE_FIND_PACKAGE_HarfBuzz=TRUE -DCMAKE_DISABLE_FIND_PACKAGE_PNG=TRUE -DCMAKE_DISABLE_FIND_PACKAGE_BZip2=TRUE -DCMAKE_DISABLE_FIND_PACKAGE_ZLIB=TRUE ..

and toggling the options TRUE and FALSE randomly (de)comments the right #defines in build/include/freetype/config/ftoption.h.

Nikolaus Waxweiler <morksig>
Project Member
Wed 05 Jun 2019 02:51:33 PM UTC, comment #7:

It is imperative to stay aligned between cmake and autoconf/make procedures because FreeType requires ultra-portability. This is to say that we do not optimize for a particular system or distribution, we are stay agnostic. Each distribution can do it's own tricks and compilation flags but FreeType won't distribute them. So please stay conservative: no c11 or gcc-specific features please.

There is indeed a bug in cmake build that does not comment out (disable) missing PNG or HARFBUZZ. Note that autoconf does that. Nikolaus, please fix this.

Alexei Podtelezhnikov <podtelez>
Project Member
Wed 05 Jun 2019 12:50:53 PM UTC, comment #6:

Reattached after accidentally deleting.

(file #47038)

Alexei Podtelezhnikov <podtelez>
Project Member
Tue 04 Jun 2019 09:23:17 PM UTC, comment #5:

I remove most of CFLAGS and hardening things.

Autodetect dependencies is evil because:
- user can not chose what he need
- user can buld package on system with all deps but target system may not have all deps, this is hard to control.

For example I buld FreeBSD ports and make packages on my home workstation. Some installed port may require HARFBUZZ and install it. Then if I rebuild/update freetype - it may auto add HARFBUZZ as dep, and then this package will be installed on other system - app may fail to run because there is no HARFBUZZ.
I came here because this already happen with me.

rim <rim>
Sat 30 Mar 2019 05:01:16 PM UTC, comment #4:

I had a cursory look at the patch, what is the reason you enable all the `-f` features? I understand that hardening is desirable in general, but the flags aren't used for the default build system and tend to be distro-specific. This would result in a different build from what the default build system would produce. I am not sure that's a good thing. I suppose enabling `-fwrapv` might benefit a number-crunching library such as FreeType, but that has to be discussed with Werner.

Also, the patch tries to deliberately enable c11 or c99 mode, while the default build system goes for ansi for maximum compatibility. This may result in subtle behavioral (source) differences iirc. Also not sure this is a good idea.

What is bad about autodetection of dependencies?

Nikolaus Waxweiler <morksig>
Project Member
Sun 30 Dec 2018 03:21:11 AM UTC, comment #3:

Remove -fsanitize=safe-stack die to brokes some apps
add target_link_libraries() for static and dynamic libs

(file #45783)

rim <rim>
Fri 28 Dec 2018 11:10:27 PM UTC, comment #2:

1. Ok, reformat done.

2. Ok, long lines fixed.

3. I can not apply changes, I do not keep tons of unworked versios and do not use any cvs in process.

4. I will need help with documetation.

5. New files added because cmake have configure_file() function, witch expect @VAR_NAME@ to replace, istead %VAR_NAME% in current files. This saves ton of cmake code and make easy to read and understand CMakeFile.txt.
Also I made some small changes to these files.

6. Ok, I try :)

I take CMakeList.txt frome one mine projects and use it as template.
Next I add all staff from original CMakeList.txt.
Then I spent many time trying to fix pc file generetion, build static and shared lib same time, replacing vars in ftconfig.h and ftoption.h.

What new/differ:
1. freetype.pc file generated and installed

2. All staff that install autotool - now installed by cmake.

3. options:
FT_WITH_ZLIB - "Use system zlib instead of internal library."
FT_WITH_BZIP2 - "Support bzip2 compressed fonts."
FT_WITH_PNG - "Support PNG compressed OpenType embedded bitmaps."
FT_WITH_HARFBUZZ - "Improve auto-hinting of OpenType fonts."
FT_WITH_CONFIG - "Install freetype-config."
FT_DISABLE_MMAP - "do not check mmap() and do not use."
ENABLE_FULL_DEBUG - "Build with all possible debug."

4. All options OFF by default, no autodetect.

5. Use hardening things, if compiller/linker support:
"-D_FORTIFY_SOURCE=2"
"-fstack-protector-all"
"-fsanitize=safe-stack"
"-fsanitize=cfi"
"-mretpoline"
"-mfunction-return=thunk"
"-mindirect-branch=thunk"
"-fwrapv"
"-fPIE"
"-pie"
"-Wl,-z,retpolineplt"
"-Wl,-z,relro"
"-Wl,-z,now"
"-Wl,-z,noexecstack"

rim <rim>
Mon 17 Dec 2018 06:57:35 AM UTC, comment #1:

Thanks for your contribution.  However, there are very serious problems with your patch.

  • I very much dislike indentation by tabs.  AFAIK, this is not necessary for cmake, so please stay with two spaces per indentation level.
  • Similarly, you are introducing overlong lines, which I very much dislike, too.  If possible, please stay within a line length of 80 characters – yes, I'm old-fashioned and conservative :-)
  • To make a comparison with your version and the current state possible, please apply your changes incrementally.  I want to have a series of patches that introduce your added functionality in steps as small as possible – and formatting should be the last, optional patch in this series.
  • Documentation is completely lacking in case you are adding new features.  You must fix this.
  • I dislike that you add new files `freetype-config.cmake' and `freetype2.cmake'.  Why?  We already have similar files, and maintaining two such files is error-prone.  This alone is actually a reason to reject your patch...
  • I need ChangeLog entries for each step (these entries should not be part of the patch files but rather added separately in the preambles of those files).

Please retry.

Werner LEMBERG <wl>
Project Administrator
Mon 17 Dec 2018 02:50:38 AM UTC, original submission:

I have done update CMake files, now they can be used as main build system, at least for FreeBSD.

I try to keep all functionality from autotools and add some hardening and debug features.

rim <rim>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #47038:  0001-Update-CMake-build-scripts.patch added by podtelez (29KiB - application/octet-stream)
file #45763:  0001-Update-CMake-build-scripts.patch added by rim (30KiB - text/x-patch - patch update)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by podtelez (Updated the item)
  • -email is unavailable- added by morksig (Posted a comment)
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by rim (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-06-13 rim Attached File- => Added 0001-Update-CMake-build-scripts.patch, #47095
    2019-06-05 podtelez Attached File- => Added 0001-Update-CMake-build-scripts.patch, #47038
    2019-06-05 podtelez Attached File#47035 => Removed
    2019-06-05 rim Attached File- => Added 0001-Update-CMake-build-scripts.patch, #47035
    2019-06-04 rim Attached File- => Added 0001-Update-CMake-build-scripts.patch, #47030
    2019-03-30 wl SummaryCMake update build scripts to support all futures => CMake update build scripts to support all features
    2018-12-30 rim Attached File- => Added 0001-Update-CMake-build-scripts.patch, #45783
    2018-12-28 rim Attached File- => Added 0001-Update-CMake-build-scripts.patch, #45763
    2018-12-17 rim Attached File- => Added 0001-Update-CMake-build-scripts.patch, #45678

    Back to the top


    Powered by Savane 3.4