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

Catch 2.13.4 contains feature from 3.0.0 pre-release #2142

Closed
uilianries opened this issue Dec 31, 2020 · 6 comments
Closed

Catch 2.13.4 contains feature from 3.0.0 pre-release #2142

uilianries opened this issue Dec 31, 2020 · 6 comments

Comments

@uilianries
Copy link
Contributor

uilianries commented Dec 31, 2020

Describe the bug
The patch version 2.13.4 provides libCatch2WithMain.a, which is listed to be included in Catch2 3.x

Expected behavior
Do not include new features in patch versions, only fixes. Keep it as same as 2.13.3. This change breaks users which don't expect using cmake for building, but only for installation. libCatch2WithMain should be included in a major version only. Or, if not possible, make it optional.

Reproduction steps

wget https://github.com/catchorg/Catch2/archive/v2.13.4.tar.gz
tar zxf v2.13.4.tar.gz
cd Catch2-2.13.4/
mkdir build
cd build/
cmake ..
cmake --build .

Platform information:

  • OS: Manjaro Linux 20.2
  • Compiler+version: GCC 10.2.0
  • Catch version: 2.13.4

Additional context
Build log (partial):

$ cmake --build .
Scanning dependencies of target Catch2WithMain
[  0%] Building CXX object CMakeFiles/Catch2WithMain.dir/src/catch_with_main.cpp.o
[  1%] Linking CXX static library libCatch2WithMain.a
[  1%] Built target Catch2WithMain

@blackliner
Copy link

blackliner commented Jan 2, 2021

How about an additional option(CATCH_MAIN_TARGET "Add a dedicated main target." OFF) ?

@AlexisWilke
Copy link

I too am running in various issues because of that library. Your .cmake files search for it in the wrong folder (i.e. /lib/... instead of the expected /usr/lib/...). So if it's a 3.x feature, we should not have it in 2.x at all. Plus it breaks your "all in the headers" scheme you were so proud about...

@blackliner
Copy link

blackliner commented Jan 9, 2021

I too am running in various issues because of that library. Your .cmake files search for it in the wrong folder (i.e. /lib/... instead of the expected /usr/lib/...). So if it's a 3.x feature, we should not have it in 2.x at all. Plus it breaks your "all in the headers" scheme you were so proud about...

What kind of issues? Can you describe this use case as a C++ file for the tests, or post it here? I can add an option for this experimental feature, that is off by default, as no one should have a problem when updating to the newest version.

@AlexisWilke
Copy link

I was able to fix my issues.

My first problem was the <arch_os> sub-folder which appears when compiling on Ubuntu 16.04 and does not appear when compiling for Ubuntu 18.04 or 20.04. But since this package is only for developers, I could bypass the architecture issue by copying the whole usr/lib folder.

However, this newer version of catch2 (2.13.4) first broke the use of the package. This is because I usually place all cmake files under /usr/share/cmake/... but your Catch2Targets.cmake actually searches for the libCatch2WithMain.a using a relative path from the Catch2Targets.cmake file location. So in other words, it has to be in the specific location you intended it to be (namely /usr/lib/[<arch_os>/]cmake/Catch2/...).

Now the search with a relative path threw me away because usually the .cmake files are expected to work on random systems. This is why, I think, the cmake environment offers commands such as find_library(). So it took me a while to understand that there was such a weird dependency on the file location.

My snapcatch2 package on launchpad works now. The source can be checked out on github. It has one dependency (our snapcmakemodules). I also add my own extra header so I don't have to re-implement the basic main() functionality I like.

Thank you for the prompt reply. Let me know if you need more info.

horenmar added a commit that referenced this issue Apr 5, 2021
The `Catch2WithMain` target was added experimentally for v2.13.4
to provide potentially better compilation (and link) times to users.
However, having it compiled by default causes worse experience for
people who do not use it, and for the v2 versions the single include
distribution is still the main one.

Closes #2142
@horenmar
Copy link
Member

horenmar commented Apr 6, 2021

Point of order: it is not a feature from v3, it is a somewhat different thing, that uses the same name for uniformity. 😃

I am also very surprised that the paths break like this. From quick glance, the paths are done in the same way that v3's static libraries are, and I do not know of there being problem with them, and Catch2Targets is autogenerated by CMake...

Anyway, I modified the target to be opt-in for v2.

@horenmar horenmar added the Resolved - pending review Issue waiting for feedback from the original author label Apr 6, 2021
@uilianries
Copy link
Contributor Author

Thanks @horenmar !

@horenmar horenmar removed the Resolved - pending review Issue waiting for feedback from the original author label Apr 13, 2021
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

4 participants