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

Removing some unecessary settings #3486

Closed
wants to merge 1 commit into from
Closed

Removing some unecessary settings #3486

wants to merge 1 commit into from

Conversation

anagno
Copy link
Contributor

@anagno anagno commented Nov 11, 2020

Coming from question #3482 in the issues. Since Eigen is a header only library, the other settings are not necessary

Specify library name and version: Eigen/

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

Coming from question #3482 in the issues. Since Eigen is a header only library, the other settings are not necessary
@conan-center-bot
Copy link
Collaborator

All green in build 1 (3829e8f1bb7439438fe0a16cc48f88cc96d7b8ea)! 😊

@madebr
Copy link
Contributor

madebr commented Nov 11, 2020

I think these were added to make sure cmake detects the correct compiler toolchain.
On the other hand, eigen does not contain a CMakeLists.txt wrapper to check the compiler or uses the cmake generator, which is safer to do (imho).
Its cost is negligible.

@Croydon
Copy link
Contributor

Croydon commented Nov 11, 2020

In the context of #3157 and what we decided to do for installer packages, it might be a lot easier to always have the full set of settings and remove on a per-recipe basis from the package ID, which is done here by self.info.header_only()

🤔

@SSE4 SSE4 requested review from uilianries and danimtb November 11, 2020 18:42
@SSE4
Copy link
Contributor

SSE4 commented Nov 11, 2020

I think it goes back to #1957

@anagno
Copy link
Contributor Author

anagno commented Nov 11, 2020

Because I got lost in the referenced issues, then we do nothing. self.info.header_only() is more than enough. The current suggestion for the header libraries is that we specify all the settings and then we use the self.info.header_only() to delete them. Right ? Should that be mentioned in the documentation?

@SSE4
Copy link
Contributor

SSE4 commented Nov 12, 2020

@anagno I think yes, this is more or less common practice for CMake-based header-only projects:

  1. have all settings defined (os, arch, compiler, build_type)
  2. clear settings in package_info

I believe we should add this to the FAQ, maybe also to the hooks (unsure).

why do we need them?
without specifying some settings, CMake may struggle to correctly generate the project or detect a compiler.
in the end, for consumer it's the same - you still get one package and on package id for all settings combinations, but it solves issues for many users.

@anagno anagno closed this Nov 12, 2020
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

Successfully merging this pull request may close these issues.

5 participants