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

Assuming os_build and arch_build as settings #4061

Merged
merged 3 commits into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@kolrami
Copy link
Contributor

commented Dec 4, 2018

Changelog: Bugfix: Take into account os_build and arch_build for search queries.
Docs: omit

Accoding to issue #4060 os_build and arch_build are not handled as options. This tries to fix it, but maybe further things are needed?

Close #4060

@CLAassistant

This comment has been minimized.

Copy link

commented Dec 4, 2018

CLA assistant check
All committers have signed the CLA.

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

Hi @kolrami

Thanks for detecting and fixing this issue. It would be great to add a unit test to make sure it is covered and doesn't break in the future. Do you want to try it yourself, would you need help?

@kolrami

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

Hi @memsharded, yes I will give it a try :)

@kolrami

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Hi @memsharded, I now have added some unit tests for the _build settings. Maybe you can have a look? Thank you very much.

@kolrami

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

Hi @memsharded, while this fixes the problem for now, a question for the future could be if we need this hard-coded list at all. What do you think of a more generic way:

  • First look into the info_settings. If the given prop_name is found we compare against it.
  • If prop_name is not found in info_settings, we look into the options.
  • To still have settings and options with the same name (maybe we do not want that at all) allow something like settings. and options. in a query to explicitly specify if we want to look for a setting or an option?
@memsharded

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

Nice job! You even detected a duplicate test. And your tests are also very good, perfect, many thanks!

@memsharded memsharded merged commit 922e890 into conan-io:develop Dec 6, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Dec 6, 2018

@memsharded memsharded added this to the 1.11 milestone Dec 6, 2018

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

I also agree with your view, and that it could be improved in the future for using different settings.
The truth is that we would like to improve the settings model for cross-building. While this might take a while, it is in our roadmap. It will be a good moment to revisit this issue. Thanks for the feedback!

@kolrami

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

Sounds cool :) Thank you for the support!

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Merge pull request conan-io#4061 from kolrami/feature/query-build-set…
…tings

Assuming os_build and arch_build as settings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.