[CMake] Modernize CMake integration #281

vicroms · 2019-05-25T00:32:37Z

I'm a maintainer for vcpkg.

A vcpkg contributor (@NNemec) and I are trying to port bdwgc to vcpkg, see: microsoft/vcpkg#6405.

While doing so, I've made some changes to the CMakeLists.txt file.

The goal is to match the output of the CMake build to the files distributed in the Debian package (which, I assume is what most downstream consumers use).

Using vcpkg`s CI, I can confirm that this results in successful builds for:

Windows 32-bit
Windows 64-bit
Windows UWP (Windows 10 Store apps)
Windows for ARM
x64-Linux (tested with Ubuntu)

The existing CMakeLists.txt only produced succesful builds for Windows 32- and 64-bit.

Both the existing version and the modified version fail to build correctly on Mac OS X, but that's something that can be added in a future PR.

Note: this only produces C++ libraries.

codecov · 2019-05-27T07:31:15Z

Codecov Report

Merging #281 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #281     +/-   ##
=========================================
+ Coverage   71.07%   71.17%   +0.1%     
=========================================
  Files          54       54             
  Lines        9051     9052      +1     
=========================================
+ Hits         6433     6443     +10     
+ Misses       2618     2609      -9

Impacted Files	Coverage Δ
allchblk.c	`74.93% <0%> (-0.28%)`	⬇️
tests/disclaim_test.c	`89.32% <0%> (+0.1%)`	⬆️
alloc.c	`67.39% <0%> (+0.16%)`	⬆️
pthread_support.c	`77.03% <0%> (+0.66%)`	⬆️
typd_mlc.c	`56.69% <0%> (+1.76%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17885ab...25a4cb1. Read the comment docs.

ivmai · 2019-05-27T07:33:15Z

OK, looking into it now. Seems to be good.
The thing I worry about is the style of keywords (upper/lower case)- I want to keep one for all CMake files. I don't have a preference, currently uppercase is used. If you think that lowercase is more readable or more common then it is OK to make a refactory commit.

ivmai · 2019-05-27T07:46:16Z

lower-case seems to be more common

ivmai · 2019-05-27T07:47:26Z

How about moving cord-specific part to Cmakefile in cord?

NNemec · 2019-05-27T09:06:58Z

Sounds reasonable, but it would be good if you could do that as a separate effort to avoid blocking this pull-request. Generally, I would even suggest to deactivate the build of cord by default. My guess is that only a negligible fraction of gc users would be interested in a highly specialized string handling library.

ivmai · 2019-05-27T20:21:24Z

it would be good if you could do that as a separate effort to avoid blocking this pull-request.

OK.

Generally, I would even suggest to deactivate the build of cord by default. My guess is that only a negligible fraction of gc users would be interested in a highly specialized string handling library.

For now, I'm not going to deactivate it, though I agree that it is of little usage. Currently it is on primarily for gc testing.

ivmai · 2019-05-27T20:21:56Z

I will review and merge your patch in 2-3 days

vicroms · 2019-05-28T19:16:37Z

I've solved the conflicts with master and added back the add_subdirectory(tests) which I had removed for the purposes of building the vcpkg port.

CMakeLists.txt

ivmai · 2019-05-29T08:25:30Z

I will reply to your questions later.

What's wrong with https://ci.appveyor.com/project/ivmai/bdwgc/build/job/j1450qvf94bqjary ?

vicroms · 2019-05-30T18:07:50Z

What's wrong with https://ci.appveyor.com/project/ivmai/bdwgc/build/job/j1450qvf94bqjary ?

I'm not too familiar with how testing works, but it looks like the tests are not linking gc properly because they reference the old target name gc-lib.

I'll update the tests/CMakeLists.txt to use the new target name.

…-install-targets

Issue #281 (bdwgc). * CMakeLists.txt (CMAKE_LEGACY_CYGWIN_WIN32): Remove (do not set). * CMakeLists.txt (VERSION): Change cmake_minimum_required from 2.6 to 3.1.

(code refactoring) Issue #281 (bdwgc). * CMakeLists.txt (option): Group all items together close to the beginning of the file; reorder items to match that of configure.

ivmai · 2019-05-30T22:12:12Z

I'm continuing review of your 2 patches in this PR.

CMakeLists.txt

…-install-targets

ivmai · 2019-05-31T18:18:39Z

This pull request introduces 11 alerts when merging af50acc into 2b70037 - view on LGTM.com

new alerts:

10 for FIXME comment
1 for Missing header guard

Comment posted by LGTM.com

vicroms · 2019-06-03T20:41:26Z

I noticed that one of the CI tests fails with the following error:

libtool: link: 
gcc 
  -DSTATICROOTSLIB2 
  -DGC_VISIBILITY_HIDDEN_SET -fvisibility=hidden 
  -Wall -Wextra -Wpedantic -Wno-long-long -g 
  -O2 -fno-strict-aliasing -D GCTEST_PRINT_VERBOSE 
  -o .libs/staticrootstest.exe 
  tests/staticrootstest-staticrootstest.o  
  ./.libs/libstaticrootslib_test.dll.a 
  ./.libs/libstaticrootslib2_test.dll.a 
  /cygdrive/c/projects/bdwgc/.libs/libgc.dll.a 
  ./.libs/libgc.dll.a 
  -L/nowhere 
  -L/usr/local/lib

It is liking to the old binary names libgcc.dll.a, while this CMakeLists should produce:

bin/gc.dll, and
lib/gc.lib

on Windows.

ivmai · 2019-06-04T08:18:04Z

What does it mean "install tests"? Why do we need an option for it?

vicroms · 2019-06-04T08:23:29Z

It is ON by default, if it is set to OFF then it skips building the tests. It is useful in vcpkg so that only the library is installed without any other extra binary (like cord for example).

ivmai · 2019-06-05T08:56:10Z

Review is still in progress.

Is it OK if I rename install_test, install_cord to build_test/cord ?

Issue #281 (bdwgc). * CMakeLists.txt (enable_sigrt_signals): Add option (default to OFF). * CMakeLists.txt [CMAKE_USE_PTHREADS_INIT && enable_sigrt_signals]: Define GC_USESIGRT_SIGNALS macro.

Issue #281 (bdwgc). build_cord and build_tests options are introduced in the CMake script. * CMakeLists.txt (build_cord): Add option (on by default); add comment. * CMakeLists.txt (build_tests): Add option (off by default). * CMakeLists.txt (cord): Specify add_subdirectory only if build_cord. * CMakeLists.txt (tests): Specify add_subdirectory only if build_tests. * cord/CMakeLists.txt: Skip cordtest and de tests unless build_tests.

CMakeLists.txt

…-install-targets

nickwanninger · 2019-06-06T23:19:39Z

I mentioned in #275, that if you run cmake .. -DCMAKE_OSX_ARCHITECTURES=x86_64, it seems to work with macOS, ignoring a warning:

CMake Warning (dev):
  Policy CMP0042 is not set: MACOSX_RPATH is enabled by default.  Run "cmake
  --help-policy CMP0042" for policy details.  Use the cmake_policy command to
  set the policy and suppress this warning.

  MACOSX_RPATH is not specified for the following targets:

   gcmt-dll

This warning is for project developers.  Use -Wno-dev to suppress it.

But other than that, this PR works on macOS with that flag

Issue #281 (bdwgc). Now, "cmake --target install" copies the public header files (to the installation destination) unless -Dinstall_headers=OFF is passed to cmake. * CMakeLists.txt (install_headers): New option (on by default). * CMakeLists.txt [install_headers] (include/gc.h, include/gc_backptr.h, include/gc_config_macros.h, include/gc_gcj.h, include/gc_inline.h, include/gc_mark.h, include/gc_pthread_redirects.h, include/gc_tiny_fl.h, include/gc_typed.h, include/gc_version.h, include/javaxfc.h, include/leak_detector.h): Specify install to "include/gc" folder. * CMakeLists.txt [install_headers && enable_cplusplus] (include/gc_allocator.h, include/gc_cpp.h): Likewise. * CMakeLists.txt [install_headers && enable_disclaim] (include/gc_disclaim.h): Likewise. * CMakeLists.txt [install_headers] (include/extra/gc.h): Specify install to "include" folder. * CMakeLists.txt [install_headers && enable_cplusplus] (include/extra/gc_cpp.h): Likewise.

(fix of commit f5006fc) Issue #281 (bdwgc). * CMakeLists.txt (add_definitions): Define _CRT_SECURE_NO_DEPRECATE only if MSVC (instead of WIN32).

Issue #281 (bdwgc). * CMakeLists.txt (enable_single_obj_compilation): New option (off by default). * CMakeLists.txt [enable_single_obj_compilation] (SRC): Override value to extra/gc.c. * CMakeLists.txt [enable_single_obj_compilation && CMAKE_USE_PTHREADS_INIT]: Define GC_PTHREAD_START_STANDALONE macro. * CMakeLists.txt [enable_single_obj_compilation && CMAKE_USE_PTHREADS_INIT] (SRC): Append pthread_start.c (in addition to extra/gc.c).

…-install-targets

ivmai · 2019-06-13T07:28:31Z

I have an issue with tests execution when shared library is built (at least on Windows): the tests are created and executed in tests folder while the library is created in base folder, so the tests fail because the library is not found.
I see 3 ways to fix it:

move the content of tests/CMakeLists.txt to base CMakeLists.txt
tell cmake somehow that the test executables should be put to the base folder
tell cmake/ctest somehow (working_directory?) that the tests should be launched from the base folder

Ideal way is 2 but I failed to do it; neither way 3. It could be fixed with way 1 but I'd like to keep tests/CMakeLists.txt.

Do you know how to fix it in way 2 or 3?

(fix of commit 17885ab) Issue #281 (bdwgc). * CMakeLists.txt [enable_cplusplus] (SRC): Add gc_cpp.cc after SRC overridden to extra/gc.c (not before) if enable_single_obj_compilation.

Issue #281 (bdwgc). * CMakeLists.txt (enable_cplusplus): Move option to above project command. * CMakeLists.txt [!enable_cplusplus] (project): Specify C as the only project language.

Issue #281 (bdwgc). Now only gc-lib is produced. * CMakeLists.txt (gcmt-dll): Remove add_library command.

wangp · 2019-06-14T08:02:00Z

Can you use add_test(... WORKING_DIRECTORY ...) or set the PATH environment variable?
https://cmake.org/pipermail/cmake/2015-November/061978.html
(not tested, sorry)

Issue #281 (bdwgc). Also specify that libgc.a is used privately in test executables. * CMakeLists.txt (gc-lib): Rename library to gc in add_library and set_target_properties. * cord/CMakeLists.txt [build_tests] (cordtest, de): Rename gc-lib to gc and add PRIVATE in target_link_libraries. * tests/CMakeLists.txt (gctest, hugetest, leaktest, middletest, realloc_test, smashtest): Likewise. * tests/CMakeLists.txt [enable_gc_debug] (tracetest): Likewise. * tests/CMakeLists.txt [enable_disclaim] (disclaim_bench, disclaim_test, disclaim_weakmap_test): Likewise.

Issue #281 (bdwgc). This is done by moving the content of cord/CMakeLists.txt and tests/CMakeLists.txt files to the base CMakeLists.txt. * CMakeLists.txt [build_tests && build_cord]: Move content from cord/CMakeLists.txt. * CMakeLists.txt [build_tests && build_cord] (cordtest, de): Add "cord/" to .c files in add_executable. * CMakeLists.txt [build_tests]: Move content from tests/CMakeLists.txt. * CMakeLists.txt [build_tests] (gctest, hugetest, leaktest, middletest, realloc_test, smashtest, tracetest, test_cpp, disclaim_bench, disclaim_test, disclaim_weakmap_test, disclaim_weakmap_test): Add "tests/" to .c and .cc files in add_executable. * Makefile.am (EXTRA_DIST): Remove cord/CMakeLists.txt, tests/CMakeLists.txt. * doc/README.cmake: Update info about CMakeLists.txt files (there is just one file now). * cord/CMakeLists.txt: Remove file. * tests/CMakeLists.txt: Likewise.

Issue #281 (bdwgc). * CMakeLists.txt [enable_cplusplus] (test_cpp): Uncomment add_executable, target_link_libraries, add_test; remove TODO.

vicroms · 2019-06-17T19:26:23Z

Sorry, I haven't had time to look into the issues.
Any updates so far?

ivmai · 2019-06-18T09:56:46Z

In progress. I'll finish this week

Issue #281 (bdwgc). * CMakeLists.txt (BUILD_SHARED_LIBS): Document option and set default to ON. * CMakeLists.txt: Remove commented out ADD_LIBRARY for atomic_ops. * CMakeLists.txt [BUILD_SHARED_LIBS] (SRC): Override to extra/gc.c. * CMakeLists.txt [BUILD_SHARED_LIBS] (GC_DLL): Define macro. * CMakeLists.txt [!BUILD_SHARED_LIBS] (GC_NOT_DLL): Define macro not only for tests but also for gc library. * CMakeLists.txt (gc): Remove STATIC in add_library specification.

Issue #281 (bdwgc). * CMakeLists.txt [build_cord] (CORD_SRC): Set variable. * CMakeLists.txt [build_cord] (cord): Specify add_library and target_link_libraries; remove TODO. * CMakeLists.txt [install_headers && build_cord] (cord/cord.h, cord/cord_pos.h, cord/ec.h): Specify install. * CMakeLists.txt [build_tests && build_cord] (cordtest, de): Do not compile cord/cordbscs.c, cord/cordprnt.c, cord/cordxtra.c; add cord to target_link_libraries specification.

(fix of commit 78c4589) Issue #281 (bdwgc). * CMakeLists.txt [build_tests && enable_cplusplus] (test_cpp): Add WIN32 in add_executable specification.

Issue #281 (bdwgc). This matches the behavior of the Automake-based build script. * CMakeLists.txt [enable_cplusplus] (SRC): Do not add gc_cpp.cc. * CMakeLists.txt [enable_cplusplus] (gccpp): Specify add_library and target_link_libraries. * CMakeLists.txt [build_tests && enable_cplusplus] (test_cpp): Add gccpp to target_link_libraries.

Issue #281 (bdwgc). * CMakeLists.txt [build_cord] (cord): Specify install(TARGETS). * CMakeLists.txt (gc): Likewise. * CMakeLists.txt [enable_cplusplus] (gccpp): Likewise.

ivmai · 2019-06-19T23:23:52Z

Why do we need target_include_directories ?

I have pushed all the rest of the changes your requested. Please check it.

Issue #281 (bdwgc). * CMakeLists.txt (enable_werror): New option (OFF by default). * CMakeLists.txt: Specify add_compile_options to report all warnings; add TODO about pedantic. * CMakeLists.txt [enable_werror]: Specify add_compile_options to treat all warnings as errors. * CMakeLists.txt [enable_werror && APPLE]: Specify add_compile_options to ignore deprecated declarations, e.g. _dyld_bind_fully_image_containing_address.

ivmai · 2021-10-07T22:08:36Z

Related PR: microsoft/vcpkg#20580 (update package to v8.2.0)

[CMake] Match output to distributed Debian package

4c3ffa1

This was referenced May 25, 2019

[bdwgc] New port microsoft/vcpkg#6405

Merged

Fix cmake build on MacOS #275

Closed

Add tests

ee16a47

vicroms commented May 28, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

vicroms commented May 28, 2019

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

vicroms added 2 commits May 30, 2019 12:26

Update tests/CMakeLists.txt

8f5d0fe

Merge branch 'master' of https://github.com/ivmai/bdwgc into pr/cmake…

b32b25b

…-install-targets

ivmai pushed a commit that referenced this pull request May 30, 2019

Upgrade cmake_minimum_required(version) to 3.1

e7dd50d

Issue #281 (bdwgc). * CMakeLists.txt (CMAKE_LEGACY_CYGWIN_WIN32): Remove (do not set). * CMakeLists.txt (VERSION): Change cmake_minimum_required from 2.6 to 3.1.

ivmai pushed a commit that referenced this pull request May 30, 2019

Group all options near the top of CMake script

4d48691

(code refactoring) Issue #281 (bdwgc). * CMakeLists.txt (option): Group all items together close to the beginning of the file; reorder items to match that of configure.

ivmai reviewed May 31, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/ivmai/bdwgc into pr/cmake…

af50acc

…-install-targets

Fix option(install_headers)

6407141

Remove unsupported Mac architecture

ab1732d

ivmai reviewed Jun 6, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/ivmai/bdwgc into pr/cmake…

56db5f0

…-install-targets

ivmai pushed a commit that referenced this pull request Jun 7, 2019

Define _CRT_SECURE_NO_DEPRECATE macro in CMake script only for MS VC

0c24cb6

(fix of commit f5006fc) Issue #281 (bdwgc). * CMakeLists.txt (add_definitions): Define _CRT_SECURE_NO_DEPRECATE only if MSVC (instead of WIN32).

Merge branch 'master' of https://github.com/ivmai/bdwgc into pr/cmake…

25a4cb1

…-install-targets

ivmai added a commit that referenced this pull request Jun 14, 2019

Remove gcmt-dll generation by CMake

5551994

Issue #281 (bdwgc). Now only gc-lib is produced. * CMakeLists.txt (gcmt-dll): Remove add_library command.

ivmai added a commit that referenced this pull request Jun 17, 2019

Enable test_cpp in CMake script

78c4589

Issue #281 (bdwgc). * CMakeLists.txt [enable_cplusplus] (test_cpp): Uncomment add_executable, target_link_libraries, add_test; remove TODO.

ivmai closed this Jun 21, 2019

ivmai mentioned this pull request Jul 8, 2019

Add more test executables to CMake script #104

Closed

ivmai mentioned this pull request Jul 4, 2021

Various CMake fixes. #356

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMake] Modernize CMake integration #281

[CMake] Modernize CMake integration #281

vicroms commented May 25, 2019 •

edited

Loading

codecov bot commented May 27, 2019 •

edited

Loading

ivmai commented May 27, 2019

ivmai commented May 27, 2019

ivmai commented May 27, 2019

NNemec commented May 27, 2019

ivmai commented May 27, 2019

ivmai commented May 27, 2019

vicroms commented May 28, 2019

ivmai commented May 29, 2019

vicroms commented May 30, 2019

ivmai commented May 30, 2019

ivmai commented May 31, 2019

vicroms commented Jun 3, 2019 •

edited

Loading

ivmai commented Jun 4, 2019

vicroms commented Jun 4, 2019

ivmai commented Jun 5, 2019

nickwanninger commented Jun 6, 2019

ivmai commented Jun 13, 2019

wangp commented Jun 14, 2019

vicroms commented Jun 17, 2019

ivmai commented Jun 18, 2019

ivmai commented Jun 19, 2019

ivmai commented Oct 7, 2021

[CMake] Modernize CMake integration #281

[CMake] Modernize CMake integration #281

Conversation

vicroms commented May 25, 2019 • edited Loading

codecov bot commented May 27, 2019 • edited Loading

Codecov Report

ivmai commented May 27, 2019

ivmai commented May 27, 2019

ivmai commented May 27, 2019

NNemec commented May 27, 2019

ivmai commented May 27, 2019

ivmai commented May 27, 2019

vicroms commented May 28, 2019

ivmai commented May 29, 2019

vicroms commented May 30, 2019

ivmai commented May 30, 2019

ivmai commented May 31, 2019

vicroms commented Jun 3, 2019 • edited Loading

ivmai commented Jun 4, 2019

vicroms commented Jun 4, 2019

ivmai commented Jun 5, 2019

nickwanninger commented Jun 6, 2019

ivmai commented Jun 13, 2019

wangp commented Jun 14, 2019

vicroms commented Jun 17, 2019

ivmai commented Jun 18, 2019

ivmai commented Jun 19, 2019

ivmai commented Oct 7, 2021

vicroms commented May 25, 2019 •

edited

Loading

codecov bot commented May 27, 2019 •

edited

Loading

vicroms commented Jun 3, 2019 •

edited

Loading