-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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: remove b2-default
value of layout
option
#20336
boost: remove b2-default
value of layout
option
#20336
Conversation
logic of this value in configure() can't work in conan v2, it raises a ConanException. If folks want default layout for Windows, they can set layout to versioned instead of this removed b2-layout.
🤖 Beep Boop! This pull request is making changes to 'recipes/boost//'. 👋 @grafikrobot @Hopobcn @jwillikers you might be interested. 😉 |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit 1c83094boost/1.83.0@#082c96117dbdc7dc78f45b601268d362
boost/1.82.0@#3347635f6da76465b8d39c7488155bce
boost/1.81.0@#ab9dabb21c28432bc1092ff1bc6564a3
boost/1.77.0@#11e87e9713334fa1c71a880a92937321
boost/1.78.0@#d5d46c13120b0855257347f7e4aad0fc
boost/1.73.0@#c02bfd577bd6ffdc57ac1fb494c7979e
boost/1.80.0@#1e94d944acfd3cf5e6502897e95cff72
boost/1.79.0@#e3ec245bc13e2959c921a8302b7e2f34
boost/1.74.0@#9465cd6359a614fcd019431ecd662b4b
boost/1.71.0@#533591956da0d6942b73824ac6897bdc
boost/1.76.0@#ecbc80552085153998b139d6df689d42
boost/1.72.0@#f5e7adccb9f001fbf3ac9b440eb5532e
boost/1.75.0@#281c8db52d169067eb9fa114896b5270
|
See also conan-io/conan#14868 for explanations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to change moving forward - We'll need to validate the options instead of forcing them, as per your conversation in the Conan repo issue, this is no longer valid in v2
I don't understand. As explained, this value was a mistake from the beginning, just an ugly way to provide default layout irrespective of host machine. If we really want the correct default layout, we should do conditional in config_options for layout option depending on os. But it may break many pre-build shared packages of downstream recipes for Windows. So I've kept default value of layout option, and anyway consumers can change layout to versioned if they want... Therefore this b2-default was useless. I've linked conan-io/conan#14868 because custom logic of boost recipe regarding b2-default was somewhat similar, but the main problem of conan-io/conan#14868 is related to deprecated options actually, and this b2-default was not a deprecation mechanism. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that looks like it's effectively a dead value for that option.
@RubenRBS can you unblock? from my point of view, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, considering:
The b2-default
is a virtual auto value for Boost project, the same does not exist in Boost project, only system
, versioned
and tagged
are available: https://github.com/boostorg/boost/blob/e4b2e2d6a11c9596a022ddfaa49c7a3c6261e297/boostcpp.jam#L64
It's true in Windows, Boost uses versioned
by default, but in this recipe we have been enforced the system
by default to any platform.
Plus, I'm not confident in auto values, as the upstream may change in future and we miss it when reviewing.
I just talked to @AbrilRBS first, so I'll wait her thoughts first.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 2 (
Conan v2 pipeline ✔️
All green in build 2 (
|
Hooks produced the following warnings for commit 1c83094boost/1.83.0@#f8c3d7363db580d9c19a9cfe8dc72691
boost/1.79.0@#5c44343f74b3b5956f807ae62e47c321
boost/1.85.0@#0146e46a0e32fbb44e7189f25c193dae
boost/1.74.0@#a8c6bb7cfaa5bc06be5d25798c225cf2
boost/1.84.0@#0782f5013c2e065b1a69f2a0397ae9b7
boost/1.75.0@#ab276f7d6b38f8bbc6b2e6355407c754
boost/1.73.0@#2c9fe41f6f6e7de7c863b47ebca19045
boost/1.81.0@#929f1e244f121f10d11a82009c466cd3
boost/1.78.0@#8ebf75b5c55dc6000202a1b1f0eb5992
boost/1.82.0@#20a883942beee1ebdda43efb5a49b0a1
boost/1.72.0@#dc2b851dcdc6ff5ba6546154a62513a9
boost/1.71.0@#cd795842a22331fb9eb4093a12cf4f8c
boost/1.80.0@#98c121475a82553038f2ad8c9623e47c
boost/1.77.0@#8ac7825236e6f9f156175d9241f6dfbc
boost/1.76.0@#3363ee59fe1aed3374493cac0501d61d
|
logic of this value in configure() can't work in conan v2, it raises a ConanException. If folks want default layout for Windows, they can set layout to versioned instead of this removed b2-layout.
logic of this value in configure() can't work in conan v2, it raises a ConanException (see https://cpplang.slack.com/archives/C41CWV9HA/p1696374024080289)
If folks want default layout for Windows, they can set layout to versioned instead of this removed b2-layout, so this value was more or less useless.