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

[package] catch2/2.9.2: Expected CMake scripts to be included in the package #252

Closed
pleroux0 opened this issue Oct 30, 2019 · 11 comments · Fixed by #253
Closed

[package] catch2/2.9.2: Expected CMake scripts to be included in the package #252

pleroux0 opened this issue Oct 30, 2019 · 11 comments · Fixed by #253
Assignees
Labels
bug Something isn't working priority: high

Comments

@pleroux0
Copy link
Contributor

Package and Environment Details (include every applicable attribute)

  • Package Name/Version: catch2/2.9.2

I expected to have access to cmake scripts that are installed with Catch2.

The helper scripts are set to be installed.

cmake.definitions["CATCH_INSTALL_HELPERS"] = "ON"

Then they are deleted during packaging.

tools.rmdir(os.path.join(self.package_folder, "lib", "cmake"))

Currently, I am using the older bincrafters package (catch2/2.5.0@bincrafters/stable) which still includes the CMake scripts. I would need to maintain my own conan package to use the newer version of Catch2.

@pleroux0 pleroux0 added the bug Something isn't working label Oct 30, 2019
@danimtb
Copy link
Member

danimtb commented Oct 30, 2019

See discussion here #209

@danimtb danimtb removed the bug Something isn't working label Oct 30, 2019
@Morwenn
Copy link
Contributor

Morwenn commented Oct 30, 2019

Catch2 provides more than just install scripts though: some of the provided CMake scripts such as CatchAddTests.cmake allow to discover Catch tests in the source and register them as CTest tests. It's notably not the Config.cmake files and yet they seem to be removed to from the package.

Those scripts should be kept since they're not meant for dependency discovery.

@danimtb
Copy link
Member

danimtb commented Oct 30, 2019

In that case, it is totally fine to have them packaged. Thanks for the info @Morwenn

@pleroux0
Copy link
Contributor Author

Thanks for the response

@Morwenn That is exactly my use case. I use catch_discover_tests.

@danimtb I'll keep updated with conan-io/conan#5798 and #209 for now. For now, I'll keep using the old package. Do you want to close this issue or keep it open to specifically track the catch2 package?

@Morwenn
Copy link
Contributor

Morwenn commented Oct 30, 2019

If I understand correctly this issue is independent of the ones you linked to: they try to solve the problems with packages that provide more than the basic dependency mechanisms in Find*.cmake and *Config.cmake files. In the test of Catch2 the relevant files are separate from the dependency-finding ones, so we should update the Catch2 recipe to not get rid of them.

@pleroux0
Copy link
Contributor Author

pleroux0 commented Oct 30, 2019

The CMake scripts require the config file to be loaded so that they are in the path. The first bullet point of conan-io/conan#5798 (comment) should fix this.

Edit: Fixed typo

uilianries added a commit to uilianries/conan-center-index that referenced this issue Oct 30, 2019
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries self-assigned this Oct 30, 2019
@uilianries uilianries added bug Something isn't working priority: high labels Oct 30, 2019
uilianries added a commit to uilianries/conan-center-index that referenced this issue Oct 31, 2019
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@pleroux0
Copy link
Contributor Author

pleroux0 commented Nov 6, 2019

This issue is not fixed as CatchAddTests.cmake is still missing from the package, which is required by catch_discover_tests.

@pleroux0
Copy link
Contributor Author

pleroux0 commented Nov 6, 2019

@danimtb Can you please re-open this issue?

uilianries added a commit to uilianries/conan-center-index that referenced this issue Nov 6, 2019
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member

@pleroux0 I gonna submit a new PR with a fix. Thanks for reporting

@Morwenn
Copy link
Contributor

Morwenn commented Nov 12, 2019

Can this issue be closed?

@danimtb
Copy link
Member

danimtb commented Nov 12, 2019

Closed by #292. Thanks @Morwenn!

@danimtb danimtb closed this as completed Nov 12, 2019
@theodelrieu theodelrieu mentioned this issue Nov 18, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority: high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants