Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recent CMake changes in this project have broken builds of dependent projects #430

Closed
claremacrae opened this issue Mar 1, 2021 · 13 comments

Comments

@claremacrae
Copy link
Contributor

Expected Behavior

1. boost.ut target seems to no longer be created

That the target boost.ut is still provided, to link against, when this project is obtained via add_subdirectory(), FetchContent() or similar...

2. include file no longer found: #include <boost/ut.hpp>

Example code - this is how I obtain the boost.ut project - UtVersion is master:

https://github.com/claremacrae/ApprovalTests.cpp.CMakeSamples/blob/main/dev_approvals_fetch_content/CMakeLists.txt#L130-L133

FetchContent_Declare(boost.ut
        GIT_REPOSITORY https://github.com/boost-ext/ut.git
        GIT_TAG ${UtVersion})
FetchContent_MakeAvailable(boost.ut)

The repo goes on to use the target boost.ut.

This directory and GitHub action exists to help the ApprovalTests.cpp project have early detection of any changes in test frameworks that might affect our users, when we do our next release.

This has been working for some months, and was still working on the 24th of Feb.

Actual Behavior

The next build, on 27th Feb at 01:10 GMT gave these errors

1. in clang builds:

Fetching boost.ut...
CMake Error at CMakeLists.txt:137 (target_compile_options):
  Cannot specify compile options for target "boost.ut" which is not built by
  this project.

2. in gcc builds:

Building CXX object approvaltests.cpp_build/tests/UT_Tests/CMakeFiles/UT_Tests.dir/UTApprovalTestTests.cpp.o
In file included from /home/runner/work/ApprovalTests.cpp.CMakeSamples/ApprovalTests.cpp.CMakeSamples/ApprovalTests.cpp/ApprovalTests/ApprovalTests.hpp:68,
                 from /home/runner/work/ApprovalTests.cpp.CMakeSamples/ApprovalTests.cpp.CMakeSamples/ApprovalTests.cpp/tests/UT_Tests/UTApprovalTestTests.cpp:3:
/home/runner/work/ApprovalTests.cpp.CMakeSamples/ApprovalTests.cpp.CMakeSamples/ApprovalTests.cpp/ApprovalTests/../ApprovalTests/integrations/ut/UTApprovals.h:14:10: fatal error: boost/ut.hpp: No such file or directory
   14 | #include <boost/ut.hpp>
      |          ^~~~~~~~~~~~~~
compilation terminated.
approvaltests.cpp_build/tests/UT_Tests/CMakeFiles/UT_Tests.dir/build.make:81: recipe for target 'approvaltests.cpp_build/tests/UT_Tests/CMakeFiles/UT_Tests.dir/UTApprovalTestTests.cpp.o' failed

Steps to Reproduce the Problem

Run this on a unix box with a recent version of either linux or gcc installed:

git clone https://github.com/claremacrae/ApprovalTests.cpp.CMakeSamples
cd ApprovalTests.cpp.CMakeSamples
git clone https://github.com/approvals/ApprovalTests.cpp.git
cd dev_approvals_fetch_content
./build.sh

Wait until all the dependencies have been cloned, and then one of the errors will occur - depending on whether you are using gcc or clang.

Specifications

The failure was detected in a GitHub Actions build. The failing builds can be seen here:

https://github.com/claremacrae/ApprovalTests.cpp.CMakeSamples/actions/runs/604457152

  • Linux Clang 6, 8, 9, and 10
  • Linux gcc 9 and 10
  • macOS Xcode 10.3, 11.7 and 12.

It's likely that the ones that passed are skipping the building of boost.ut

@ClausKlein
Copy link
Contributor

That is caused by my changes, I have noted to it during review!

the project name was changed to ut.
the imported target to Boost::ut

see

# Create target Boost::ut and install target

@claremacrae
Copy link
Contributor Author

claremacrae commented Mar 1, 2021

That is caused by my changes, I have noted to it during review!

the project name was changed to ut.
the imported target to Boost::ut

see

# Create target Boost::ut and install target

Thanks for the reply.

There's nearly 500 stars and more than 40 forks of this project, so that's a lot of people, many of whom have to update their builds.

And those of us that maintain libraries that provide interfaces to this project now have to decide whether to drop supporting the older versions, or not support newer versions....

Please consider maintaining aliases for backwards compatibility, to prevent this breaking change.

@ClausKlein
Copy link
Contributor

And those of us that maintain libraries that provide interfaces to this project now have to decide whether to drop supporting the older versions, or not support newer versions....

Please consider maintaining aliases for backwards compatibility, to prevent this breaking change.

one problem was: the config version.cmake file were not installed.
so the find-package(...) does not realy work!

and other Boost components are all exported as i.e. Boost::filesystem

Is it possible to do add_library(boost.ut ALIAS Boost::ut) after FetchContent_MakeAvailable()?

@claremacrae
Copy link
Contributor Author

and other Boost components are all exported as i.e. Boost::filesystem

That's a really interesting point...

I really agree with the name chosen for this project in Conan-center-index, which is boost-ext-ut - the reason I agree is that this project isn't part of boost, and calling its target Boost.ut creates a cause for confusion, I feel... The name boost-ext-ut feels fairer - suggesting external.

https://github.com/conan-io/conan-center-index/tree/master/recipes/boost-ext-ut

Is it possible to do add_library(boost.ut ALIAS Boost::ut) after FetchContent_MakeAvailable()?

I think it's not that simple... it would have to be something like this pseudo-code:

if target Boost::ut exists 
    add_library(boost.ut ALIAS Boost::ut)
endif

Why impose that on users, and more importantly, the time taken to research the problem when they get the failure, as happened to me today - when it could be a one-liner inside the project?

@ClausKlein
Copy link
Contributor

you should be happy, it works now without your config hacks:

bash-3.2$ git diff
diff --git a/dev_approvals_fetch_content/CMakeLists.txt b/dev_approvals_fetch_content/CMakeLists.txt
index 8e2eaa9..bf562b8 100644
--- a/dev_approvals_fetch_content/CMakeLists.txt
+++ b/dev_approvals_fetch_content/CMakeLists.txt
@@ -132,14 +132,16 @@ FetchContent_Declare(boost.ut
         GIT_TAG ${UtVersion})
 FetchContent_MakeAvailable(boost.ut)
 
-if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
-    # Turn off some checks off for boost.ut
-    target_compile_options(boost.ut INTERFACE
-            -Wno-c99-extensions # Needed for Boost.ut, at least in v1.1.6
-            -Wno-documentation-unknown-command # unknown command tag name \userguide
-            -Wno-weak-vtables
-            -Wno-comma # See https://github.com/boost-ext/ut/issues/398
-            )
+if(TARGET boost.ut)
+    if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
+        # Turn off some checks off for boost.ut
+        target_compile_options(boost.ut INTERFACE
+                -Wno-c99-extensions # Needed for Boost.ut, at least in v1.1.6
+                -Wno-documentation-unknown-command # unknown command tag name \userguide
+                -Wno-weak-vtables
+                -Wno-comma # See https://github.com/boost-ext/ut/issues/398
+                )
+    endif()
 endif()
 
 # -------------------------------------------------------------------
bash-3.2$ 

@ClausKlein
Copy link
Contributor

@claremacrae one note: IMHO the fetchcontents is very slow and not very flexible to use.

would it be an option to try https://github.com/cpm-cmake/CPM.cmake

with local cache enable, it is really fast!

@claremacrae
Copy link
Contributor Author

claremacrae commented Mar 1, 2021

you should be happy, it works now without your config hacks:

....

Thanks - I appreciate the suggestion....

One of the benefits of the ApprovalTests.cpp.CMakeSamples project is for me to test out code against multiple different versions of Boost.ut, when we get bug reports from ApprovalTests users, such as approvals/ApprovalTests.cpp#157

Hence my desire to be able to keep building against a variety of Boost.ut versions...

@claremacrae
Copy link
Contributor Author

claremacrae commented Mar 1, 2021

@claremacrae one note: IMHO the fetchcontents is very slow and not very flexible to use.

would it be an option to try https://github.com/cpm-cmake/CPM.cmake

with local cache enable, it is really fast!

Thanks for that suggestion too...

Another of the benefits - in fact purposes - of ApprovalTests.cpp.CMakeSamples is to provide tested CMake code that we can embed in to the ApprovalTests.cpp docs, at these two pages:

What I'm after is simple, standard things that work with CMake out of the box - and Conan out of the box - so that I can give new users at least one simple thing to try for them...

So there's no advantage to me in having a layer on top to speed things up, as it one more thing to document and test....

For that kind of documentation - and the target audience of people getting started - simpler is better...

Anyone who already knows about CPM or similar is of course free to disregard the pointers we give, and make their own way...

@ClausKlein
Copy link
Contributor

Fine, I like this: https://github.com/approvals/ApprovalTests.cpp/blob/master/CMake/WarningsAsErrors.cmake

@claremacrae claremacrae changed the title Recent CMake changes in this project appear to have broken builds of dependent projects Recent CMake changes in this project have broken builds of dependent projects Mar 20, 2021
@claremacrae
Copy link
Contributor Author

Is it possible to do add_library(boost.ut ALIAS Boost::ut) after FetchContent_MakeAvailable()?

Just to help anyone else who comes here, the above gives an error about Boost::ut already being an alias.

This works:

add_library(boost.ut ALIAS ut)

claremacrae added a commit to claremacrae/ApprovalTests.cpp.CMakeSamples that referenced this issue Mar 20, 2021
@kris-jusiak
Copy link
Contributor

@claremacrae would the following be okay?

@claremacrae
Copy link
Contributor Author

@claremacrae would the following be okay?

Thanks! I've answered in that ticket...

I worked around the issue before you kindly added that, but I think it's likely that the change will affect others as well.

Also for others here, the suggestion in #430 (comment) didn't work for me with clang9, with the latest boost.ut. I still got lots of warnings about C99 code...

This is what I needed, to work with Clang 9 on Linux:

message(NOTICE "Fetching boost.ut...")
set(BUILD_BENCHMARKS OFF CACHE BOOL "")
set(BUILD_EXAMPLES OFF CACHE BOOL "")
set(BUILD_TESTS OFF CACHE BOOL "")
FetchContent_Declare(boost.ut
        GIT_REPOSITORY https://github.com/boost-ext/ut.git
        GIT_TAG ${UtVersion})
FetchContent_MakeAvailable(boost.ut)

if(TARGET Boost::ut)
    add_library(boost.ut ALIAS ut)
endif()

if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
    # Turn off some checks off for boost.ut
    target_compile_options(ut INTERFACE
            -Wno-c99-extensions # Needed for Boost.ut, at least in v1.1.6
            -Wno-documentation-unknown-command # unknown command tag name \userguide
            -Wno-weak-vtables
            -Wno-comma # See https://github.com/boost-ext/ut/issues/398
            )
endif()

@claremacrae
Copy link
Contributor Author

Closing, since based on the comments, it seems unlikely that it's worth leaving open...

I've added notes above, in the hope of helping others out....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants