-
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
ios-cmake: migrate to Conan v2, improve test_package #21532
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
6174a8e
to
cdb0111
Compare
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit cdb0111ios-cmake/3.1.2@#579c560cc6c3cdaf7f6fabc8f2d9c11e
|
cdb0111
to
f2d764e
Compare
This comment has been minimized.
This comment has been minimized.
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.
Some minor comments, otherwise I like how this looks :)
"WATCHOSCOMBINED", "SIMULATOR_WATCHOS", | ||
"MAC", "MAC_ARM64", "MAC_CATALYST", "MAC_CATALYST_ARM64"] | ||
"toolchain_target": [ | ||
"auto", |
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.
I think we can use the fact that we're migrating this to Conan 2 to remove the auto value altogether so we don't need to check for it later? wdyt @valgur?
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.
I don't think that would work. The logic in config_options()
sets the default value to an appropriate one based on self.settings.os
and falls back to auto
if it cannot be determined. The self._default_toolchain_target
property does not cover several Apple OS-s, notably Macos
.
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.
Oh so then the idea is that if it's not able to be guessed, we fail later in the validate if it's still set to auto? My only concern is that then running conan create ... -o="&:toolchain_target=auto"
will always fail, the CLI value is overriding the decision in config_options
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.
Oh I see - I've pushed a minor change to let users pass auto and have it be still auto-guessed, let me know if it works for you :)
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.
Uhm... self.options
always report their default value in configure()
, even if the user overrides them. But only if packge_type = "build-scripts"
for some reason.
Second, it always fails with:
ConanException: Incorrect attempt to modify option 'toolchain_target' from 'auto' to 'xyz'
It would be great if the Conan client got rid of that artificial limitation. :/
The VTK PR (#10776) actually manages to work around that limitation through some black magic, basically doing:
def configure(self):
overrides = {}
if self.options.toolchain_target == "auto":
overrides["toolchain_target"] = self._default_toolchain_target
self.options.update(self.options.possible_values, dict(self.options.items(), **overrides))
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.
The build-scripts
package type is super-buggy or simply inconsistent with the options handling overall, for some reason. The user-supplied option values are simply ignored and not even validated to be one of the allowed values.
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.
Oooh... the options actually work, but only if I use -o:b toolchain_target=...
. This could maybe be documented better.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This reverts commit 45c3127.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Failure in build 5 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping Failure in build 5 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
@valgur are you still intending to work on this PR? This is package is a sticking point for my work's internal Conan 2 migration, so we can probably pick it up if you don't have time (although it doesn't look like it needs much work to drive it forwards? Maybe just rebasing to set off another CI run?). |
I'd be happy to help, but honestly, it's probably more likely to be merged if you open a new PR based off this one. I am no longer able to trigger the PRs without the team since to the newly-imposed limit on the number of allowed open PRs, and new PRs tend to get more immediate attention as well, it seems. |
No description provided.