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

boost-1.78 fix cmake auto configuration for windows #8526

Merged
merged 5 commits into from
Jan 24, 2022

Conversation

pgeler
Copy link
Contributor

@pgeler pgeler commented Dec 23, 2021

closes: #8550
Specify library name and version: boost-1.78

cmake auto configuration failing with boost-1.78:

1> [CMake] 	"toolset": "-{}".format(self._toolset_tag),
1> [CMake] while calling '_toolset_tag', line 1294
1> [CMake] 	if self.settings.compiler in ("msvc", "Visual Studio"):
1> [CMake] 	ConanException: Invalid setting 'msvc' is not a valid 'settings.compiler' value.
1> [CMake] Possible values are ['Visual Studio', 'apple-clang', 'clang', 'gcc', 'intel', 'qcc', 'sun-cc']

  • 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.

@CLAassistant
Copy link

CLAassistant commented Dec 23, 2021

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link

ghost commented Dec 23, 2021

@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor

SpaceIm commented Dec 23, 2021

Specify library name and version: boost-1.78

cmake auto configuration failing with boost-1.78:

1> [CMake] 	"toolset": "-{}".format(self._toolset_tag),
1> [CMake] while calling '_toolset_tag', line 1294
1> [CMake] 	if self.settings.compiler in ("msvc", "Visual Studio"):
1> [CMake] 	ConanException: Invalid setting 'msvc' is not a valid 'settings.compiler' value.
1> [CMake] Possible values are ['Visual Studio', 'apple-clang', 'clang', 'gcc', 'intel', 'qcc', 'sun-cc']
* [ ]  I've read the [guidelines](https://github.com/conan-io/conan-center-index/blob/master/docs/how_to_add_packages.md) for contributing.

* [ ]  I've followed the [PEP8](https://www.python.org/dev/peps/pep-0008/) style guides for Python code in the recipes.

* [ ]  I've used the [latest](https://github.com/conan-io/conan/releases/latest) Conan client version.

* [ ]  I've tried at least one configuration locally with the
  [conan-center hook](https://github.com/conan-io/hooks.git) activated.

Your settings.yml is outdated.

@conan-center-bot

This comment has been minimized.

@DDoSolitary
Copy link

I encountered the same problem, but I think swapping the strings isn't a real fix. It only works if your compiler is Visual Studio.

Your settings.yml is outdated.

@SpaceIm I don't think it's a good idea to require an up-to-date settings.yml file. The recipe is breaking existing projects running on a somewhat old version of Conan. Such breaking change without any warning is really annoying.

I would suggest using a similar approach as conan-io/conan#9954, i.e. reading the compiler value with self.settings.get_safe('compiler') before comparision.

@ericLemanissier
Copy link
Contributor

@SpaceIm
Copy link
Contributor

SpaceIm commented Dec 25, 2021

The last revision of boost recipe already requires conan 1.33.0:

required_conan_version = ">=1.33.0"

And msvc has been added to settings.yml in conan 1.33.0 https://github.com/conan-io/conan/releases/tag/1.33.0

@SSE4 SSE4 requested a review from uilianries December 27, 2021 07:28
@conan-center-bot conan-center-bot requested a review from SSE4 January 3, 2022 12:01
@pgeler
Copy link
Contributor Author

pgeler commented Jan 4, 2022

The last revision of boost recipe already requires conan 1.33.0:

required_conan_version = ">=1.33.0"

And msvc has been added to settings.yml in conan 1.33.0 https://github.com/conan-io/conan/releases/tag/1.33.0

the problem in the way conan upgrade the configurations, changes in seettings.yml especially when it was all the way "default" is a painful to push through entire organization especially when people using it time to time...

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 4, 2022

self.settings.compiler in ("msvc", "Visual Studio") raises ConanException when msvc is not in settings.yml, it's a conan client issue I believe, not a recipe issue.
We should be able to compare self.settings.compiler with any string, it should never raise (return False instead). Indeed, recipes have no control on consumers settings.yml.

Does it work with str(self.settings.compiler) in ("msvc", "Visual Studio")?

(also related to #6155)

@pgeler
Copy link
Contributor Author

pgeler commented Jan 4, 2022

self.settings.compiler in ("msvc", "Visual Studio") raises ConanException when msvc is not in settings.yml, it's a conan client issue I believe, not a recipe issue. We should be able to compare self.settings.compiler with any string, it should never raise (return False instead). Indeed, recipes have no control on consumers settings.yml.

Does it work with str(self.settings.compiler) in ("msvc", "Visual Studio")?

(also related to #6155)

yes, it does work, in fact that was original solution. but then found this ("order") hack in other receipts

@pgeler
Copy link
Contributor Author

pgeler commented Jan 4, 2022

I think it will be more straight forward to change to condition to self.settings.get_safe("compiler"): @SpaceIm, @SSE4 thoughts?

@ericLemanissier
Copy link
Contributor

get_safe does not help here. Compiler always exists

@pgeler
Copy link
Contributor Author

pgeler commented Jan 4, 2022

self.settings.get_safe("compiler")

it does help

well, another way I may think of is to use local variable in condition compiler == 'vc'

@SpaceIm
Copy link
Contributor

SpaceIm commented Jan 4, 2022

str(self.settings.compiler) seems to be the most robust approach.

@pgeler
Copy link
Contributor Author

pgeler commented Jan 4, 2022

str(self.settings.compiler) seems to be the most robust approach.

✔️ sold

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this change because it introduces a weakness to typos. It's necessary to update settings.yml for other reasons anyway. If user really want to use an outdated settings.yml, they can use older revisions of recipes.

@pgeler
Copy link
Contributor Author

pgeler commented Jan 4, 2022

I don't like this change because it introduces a weakness to typos. It's necessary to update settings.yml for other reasons anyway. If user really want to use an outdated settings.yml, they can use older revisions of recipes.

outdated settings.yml was not intentional, the very first time I actually noticed it even exists was @SpaceIm comment above. The main problem I view is in strategy conan uses for upgrading it. But I'm confident that the strategy was thoughtful and don't want to dealt with changing it... I'm just trying to simplify upgrade for my organization and other people who is not to investigate each issue with every update. Otherwise the package manager does not remove, but add complexity to their day by day work...

@ghost ghost mentioned this pull request Jan 10, 2022
4 tasks
@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

simplify upgrade for my organization and other people who is not to investigate each issue with every update.

Former Conan enterprise user, what worked best for us was using the https://docs.conan.io/en/latest/reference/commands/consumer/config.html install option with a repo with our settings (but we needed custom settings and had to semi-automatic upgrade)

There's a super awesome feature

The conan config install can be periodically executed, before any command, when config_install_interval is configured in conan.conf. Conan runs it based on config_install.json, including the timestamp of the last change.

Hopefully this helps

@conan-center-bot

This comment has been minimized.

@prince-chrismc
Copy link
Contributor

You'll need to retrigger CI, close the pr wait 10s and then re-open it 🔁

@conan-center-bot
Copy link
Collaborator

All green in build 7 (acf372e3191696d8fe38780936caf8a3bfe1306e):

  • boost/1.69.0@:
    All packages built successfully! (All logs)

  • boost/1.70.0@:
    All packages built successfully! (All logs)

  • boost/1.71.0@:
    All packages built successfully! (All logs)

  • boost/1.72.0@:
    All packages built successfully! (All logs)

  • boost/1.76.0@:
    All packages built successfully! (All logs)

  • boost/1.73.0@:
    All packages built successfully! (All logs)

  • boost/1.78.0@:
    All packages built successfully! (All logs)

  • boost/1.74.0@:
    All packages built successfully! (All logs)

  • boost/1.77.0@:
    All packages built successfully! (All logs)

  • boost/1.75.0@:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit 073927b into conan-io:master Jan 24, 2022
@pgeler pgeler deleted the fix-boost-1.78-cmake-windows branch January 24, 2022 16:48
Sahnvour pushed a commit to Sahnvour/conan-center-index that referenced this pull request Jan 30, 2022
* fix cmake auto configuration for windows

* fix

* fix
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.

[package] boost/1.78.0: Failing install on Windows using Visual Studio compiler
9 participants