catch2 (2.x.x): support conan v2 #13794

paulharris · 2022-10-27T04:38:59Z

catch2 (2.x.x): support conan v2, based on work by SpaceIm that was done for 3.x.x
ie #13184

It currently doesn't build the test_package with with_prefix=True

It seems that the package_info() self.cpp_info.defines are not being seen by the test_package. I'm not sure what I'm doing wrong.

…one for 3.x.x

recipes/catch2/2.x.x/conanfile.py

uilianries · 2022-10-27T09:46:01Z

recipes/catch2/2.x.x/conanfile.py

+        tc.variables["BUILD_TESTING"] = False
+        tc.cache_variables["CATCH_INSTALL_DOCS"] = False
+        tc.cache_variables["CATCH_INSTALL_HELPERS"] = "ON"
+        tc.cache_variables["CATCH_BUILD_STATIC_LIBRARY"] = self.options.with_main   # FIXME cache_variables or just variables?


In case of doubt: conan-io/conan#11937 (comment)

Plus:

https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#cache-variables

https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#variables

There are two main cases to use cache_variables:

When a definition is configured before project()

When an option uses CACHE. Need to check project's CMakeLists.txt

The docs currently focus on the difference that CACHE is for "single config" compare to "multiple config",
and says for the majority of cases cache_variables is what you probably want to use ... so this seems to be a complex topic...
I see almost all recipes use variables, but it seems like if you really want the variable to be set, you should use cache_variables and there is less room for surprises?

In CMake, cache variables are how CMake and projects expose options to be set by consumers. Cache variables are meant to be able to be preemptible by consumers. If a consumer sets it before a project does, the project won't change the value. I think you're right, @paulharris, that we should prefer setting cache variables instead when possible, as this is how projects should expose the options we are often setting.

There are of course, tons of CMake projects that don't respect cache variables and set normal variables with the same name which causes all sorts of problems.... ugh. And the issue with setting most variables before the first project command is quite problematic.

Hopefully improvements for this are coming with a future CMake?

#13971 not the best but it's a start

Co-authored-by: Uilian Ries <[email protected]>

paulharris · 2022-10-28T02:11:38Z

Can someone please help me understand why this line is in the 3.x.x recipe:

conan-center-index/recipes/catch2/3.x.x/conanfile.py

Line 86 in b657409

tc.variables["CATCH_CONFIG_PREFIX_ALL"] = self.options.with_prefix

tc.variables["CATCH_CONFIG_PREFIX_ALL"] = self.options.with_prefix
As far as I can tell, this is not a CMake variable...
I see it here:

target_compile_definitions( PrefixedMacros PRIVATE CATCH_CONFIG_PREFIX_ALL CATCH_CONFIG_RUNTIME_STATIC_REQUIRE )
# Macro configuration does not touch the compiled parts, so we can link
# it against the main library
target_link_libraries( PrefixedMacros Catch2WithMain )

add_test(NAME CATCH_CONFIG_PREFIX_ALL COMMAND PrefixedMacros -s)
set_tests_properties(
    CATCH_CONFIG_PREFIX_ALL
  PROPERTIES
    PASS_REGULAR_EXPRESSION "CATCH_"
...

But that is defining a test name(?) not a variable...

recipes/catch2/2.x.x/conanfile.py

paulharris · 2022-10-28T04:20:56Z

recipes/catch2/3.x.x/conanfile.py

@@ -46,7 +46,7 @@ def _compilers_minimum_version(self):

    @property
    def _default_reporter_str(self):
-        return '"{}"'.format(str(self.options.default_reporter).strip('"'))
+        return str(self.options.default_reporter).strip('"')


@SpaceIm as noted in main comment - please check this too

paulharris · 2022-10-28T04:21:25Z

I've fixed up the prefix behaviour, looks like there was some errors in the 3.x.x recipe.

@SpaceIm can you please check the highlighted lines especially, as I'm not sure why the magic was required for the quotes. conan/cmake is quoting those values, so they end up double-quoted in conan_toolchain.cmake if the quotes are added here.

recipes/catch2/2.x.x/conanfile.py

ghost · 2022-11-09T16:22:25Z

I detected other pull requests that are modifying catch2/3.x.x recipe:

catch2: add console_width parameter #14475

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prsso don't hesitate to report issues/improvements there.

uilianries · 2022-11-10T12:47:08Z

recipes/catch2/2.x.x/test_package/conanfile.py

+        # note: this is required as self.dependencies is not available in test()
+        self._tests_todo.append("test_package")
+        if catch_opts.with_main:
+            self._tests_todo.append("standalone")
+        if not catch_opts.with_prefix and catch_opts.with_main and catch_opts.with_benchmark:
+            self._tests_todo.append("benchmark")


Suggested change

# note: this is required as self.dependencies is not available in test()

self._tests_todo.append("test_package")

if catch_opts.with_main:

self._tests_todo.append("standalone")

if not catch_opts.with_prefix and catch_opts.with_main and catch_opts.with_benchmark:

self._tests_todo.append("benchmark")

# note: this is required as self.dependencies is not available in test()

self._tests_todo.append("test_package")

if catch_opts.with_main:

self._tests_todo.append("standalone")

if not catch_opts.with_prefix and catch_opts.with_main and catch_opts.with_benchmark:

self._tests_todo.append("benchmark")

It does not work. Conan parse twice the recipe folder and _tests_todo will be empty as soon as test() is called.

You can create a temporary file and and those values. More information: conan-io/conan#12411

I did this in 332eb40

I cherry-picked your commit and fixed it up slightly

recipes/catch2/2.x.x/conanfile.py

prince-chrismc · 2022-11-14T23:37:10Z

Whoops I pushed to the wrong repo 🙈

I got this working + applied a few of the comments

This reverts commit 332eb40.

Co-authored-by: Chris Mc <[email protected]>

recipes/catch2/2.x.x/conanfile.py

paulharris · 2022-11-28T14:35:56Z

Thanks @prince-chrismc, I included your work and fixed it up slightly

Co-authored-by: SpaceIm <[email protected]>

uilianries

LGTM

conan-center-bot · 2022-12-02T17:06:21Z

Conan v1 pipeline

All green in build 19 (bf47652e73f1abbc916f65c40cce6490cc182b49):

catch2/2.13.9@:
All packages built successfully! (All logs)
catch2/2.11.3@:
All packages built successfully! (All logs)
catch2/2.13.8@:
All packages built successfully! (All logs)
catch2/2.12.4@:
All packages built successfully! (All logs)
catch2/3.1.0@:
All packages built successfully! (All logs)
catch2/2.13.7@:
All packages built successfully! (All logs)
catch2/3.2.0@:
All packages built successfully! (All logs)
catch2/3.0.1@:
All packages built successfully! (All logs)

paulharris added 2 commits October 27, 2022 12:08

catch2 (2.x.x): support conan v2, based on work by SpaceIm that was d…

c22397f

…one for 3.x.x

Some minor tweaks

d2502b3

paulharris changed the title ~~[catch2 (2.x.x): support conan v2~~ catch2 (2.x.x): support conan v2 Oct 27, 2022

github-actions bot mentioned this pull request Oct 27, 2022

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot added the Failed label Oct 27, 2022

This comment has been minimized.

Sign in to view

Linter fixes

6fe08f1

uilianries reviewed Oct 27, 2022

View reviewed changes

Update recipes/catch2/2.x.x/conanfile.py

3083bfd

Co-authored-by: Uilian Ries <[email protected]>

conan-center-bot added Service Under Maintenance and removed Failed labels Oct 27, 2022

This comment has been minimized.

Sign in to view

paulharris added 2 commits October 28, 2022 12:16

Fixed CATCH_CONFIG_PREFIX_ALL behaviour

0e8effe

Don't quote default_reporter - 3.x.x already changed

c67dd63

paulharris commented Oct 28, 2022

View reviewed changes

recipes/catch2/2.x.x/conanfile.py Show resolved Hide resolved

paulharris commented Oct 28, 2022

View reviewed changes

conan-center-bot added Failed and removed Service Under Maintenance labels Oct 28, 2022

This comment has been minimized.

Sign in to view

Fix for conan 1.52

a5170e5

conan-center-bot removed the Failed label Oct 28, 2022

This comment has been minimized.

Sign in to view

prince-chrismc reviewed Oct 30, 2022

View reviewed changes

recipes/catch2/2.x.x/conanfile.py Outdated Show resolved Hide resolved

prince-chrismc previously approved these changes Oct 30, 2022

View reviewed changes

Fixup error message

44f0fe6

paulharris dismissed prince-chrismc’s stale review via 44f0fe6 November 2, 2022 06:34

This comment has been minimized.

Sign in to view

prince-chrismc mentioned this pull request Nov 2, 2022

docs: when to use CMakeToolchain's variable choices prince-chrismc/conan-center-index#39

Closed

uilianries reviewed Nov 10, 2022

View reviewed changes

prince-chrismc reviewed Nov 14, 2022

View reviewed changes

recipes/catch2/2.x.x/conanfile.py Outdated Show resolved Hide resolved

v2 fixes

332eb40

conan-center-bot added the Failed label Nov 14, 2022

This comment has been minimized.

Sign in to view

Revert "v2 fixes"

506f373

This reverts commit 332eb40.

conan-center-bot removed the Failed label Nov 15, 2022

This comment has been minimized.

Sign in to view

danimtb previously approved these changes Nov 15, 2022

View reviewed changes

conan-center-bot requested a review from franramirez688 November 15, 2022 12:00

prince-chrismc mentioned this pull request Nov 17, 2022

[develop2] Try to revert check_min_cppstd to use self.settings instead of self.info.settings conan-io/conan#12555

Merged

ghost mentioned this pull request Nov 18, 2022

catch2: add version 3.2.0 #14265

Merged

4 tasks

paulharris and others added 2 commits November 28, 2022 20:51

Merge remote-tracking branch 'origin/master' into catch2-conan-v2

a0227c3

Update recipes/catch2/2.x.x/conanfile.py

8e4afe4

Co-authored-by: Chris Mc <[email protected]>

paulharris dismissed danimtb’s stale review via 8e4afe4 November 28, 2022 12:51

Christopher McArthur and others added 2 commits November 28, 2022 22:15

v2 fixes

16532b8

Fixup todos filename for test_package

a0a5794

SpaceIm reviewed Nov 28, 2022

View reviewed changes

recipes/catch2/2.x.x/conanfile.py Outdated Show resolved Hide resolved

Update recipes/catch2/2.x.x/conanfile.py

bf47652

Co-authored-by: SpaceIm <[email protected]>

prince-chrismc approved these changes Nov 28, 2022

View reviewed changes

ghost mentioned this pull request Nov 28, 2022

catch2: add console_width parameter #14475

Merged

4 tasks

uilianries approved these changes Nov 29, 2022

View reviewed changes

prince-chrismc closed this Dec 1, 2022

prince-chrismc reopened this Dec 1, 2022

conan-center-bot merged commit 8c1c300 into conan-io:master Dec 2, 2022

paulharris deleted the catch2-conan-v2 branch December 5, 2022 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

catch2 (2.x.x): support conan v2 #13794

catch2 (2.x.x): support conan v2 #13794

paulharris commented Oct 27, 2022

This comment has been minimized.

uilianries Oct 27, 2022

paulharris Oct 27, 2022

jwillikers Oct 28, 2022 •

edited

Loading

paulharris Nov 2, 2022

prince-chrismc Nov 2, 2022

This comment has been minimized.

paulharris commented Oct 28, 2022

paulharris Oct 28, 2022

paulharris commented Oct 28, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ghost commented Nov 9, 2022 •

edited by ghost

Loading

uilianries Nov 10, 2022

prince-chrismc Nov 14, 2022

paulharris Nov 28, 2022

prince-chrismc commented Nov 14, 2022

This comment has been minimized.

This comment has been minimized.

paulharris commented Nov 28, 2022

uilianries left a comment

conan-center-bot commented Dec 2, 2022

catch2 (2.x.x): support conan v2 #13794

catch2 (2.x.x): support conan v2 #13794

Conversation

paulharris commented Oct 27, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwillikers Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

paulharris commented Oct 28, 2022

Choose a reason for hiding this comment

paulharris commented Oct 28, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ghost commented Nov 9, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince-chrismc commented Nov 14, 2022

This comment has been minimized.

This comment has been minimized.

paulharris commented Nov 28, 2022

uilianries left a comment

Choose a reason for hiding this comment

conan-center-bot commented Dec 2, 2022

Conan v1 pipeline

jwillikers Oct 28, 2022 •

edited

Loading

ghost commented Nov 9, 2022 •

edited by ghost

Loading