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

[4/n] [xbuild] [ci] Get the value for 'xxx_build' and 'xxx_target' from corresponding profiles if available #6916

Merged
merged 17 commits into from Apr 30, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Apr 27, 2020

Changelog: Feature: Consume settings_build to get the value of the OS and arch from the build machine (only when --profile:build is provided).
Docs: conan-io/docs#1678

On top of #6769

Retrieve the values for os_build and arch_build from:

  • settings_build.os and settings_build.arch if the profile for the build context is being used.
  • keep working as it was before if no profile for build context has been provided.

This PR also modifies the signatures of the tools cross_building and vcvars_command (non-breaking, backward compatible), now they prefer a conanfile as the first argument. This cannot be enforced due to backward compatibility, but it makes it impossible to get the settings_build from the conanfile for those recipes where the user is already passing the conanfile.settings as the first argument.

#TAGS: slow


closes jgsogo#37
closes #6778 (supersedes)

@jgsogo jgsogo marked this pull request as ready for review Apr 27, 2020
@jgsogo jgsogo requested a review from memsharded Apr 27, 2020
@jgsogo jgsogo requested a review from danimtb Apr 27, 2020
Copy link
Member

@memsharded memsharded left a comment

Looking good, almost there.
I would like if the pattern of reading the os/arch from build or fallback would be a bit more straightforward, but I understand why it is like that, and I am ok with it.

if os_detected is None or arch_detected is None or self._arch is None or self._os is None:
settings_build = getattr(self._conanfile, 'settings_build', None)
if settings_build:
os_build, arch_build = get_build_os_arch(self._conanfile)
Copy link
Member

@memsharded memsharded Apr 29, 2020

Choose a reason for hiding this comment

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

Maybe don't call again the method that knows how to fallback, because you already checked that you have settings_build.

Copy link
Member Author

@jgsogo jgsogo Apr 29, 2020

Choose a reason for hiding this comment

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

I need to check here if a have settings_build because the values returned by the fallback are not the ones expected here.

Probably this is a bug in the existing code: this function doesn't consider settings.os_build or settings.arch_build, only those detected or returned by platform.xxx().

self.conanfile.settings.get_safe("os"))
flags.extend(rpath_flags(the_os, self.compiler, self._deps_build_info.lib_paths))
os_build, _ = get_build_os_arch(self.conanfile)
if not hasattr(self.conanfile, 'settings_build'):
Copy link
Member

@memsharded memsharded Apr 29, 2020

Choose a reason for hiding this comment

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

This is also a bit complicated to read, you already checked the settings_build in the get_build_os_arch() call.

Copy link
Member Author

@jgsogo jgsogo Apr 29, 2020

Choose a reason for hiding this comment

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

totally agree, but here in the existing code we are using settings.os as a fallback. With the previous model, it could be a valid assumption that settings.os is a valid fallback-value for the _build contexts (I think it is not ok, but it was already there), but if you provide a profile_build this is no longer a valid assumption.

conans/client/tools/oss.py Show resolved Hide resolved
settings_build = getattr(conanfile, 'settings_build', None)
if settings_build:
return settings_build.get_safe('os'), settings_build.get_safe('arch')
if hasattr(conanfile, 'settings_build'):
Copy link
Member

@memsharded memsharded Apr 30, 2020

Choose a reason for hiding this comment

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

I still think that this is a bit of a secondary effect: no build_profile -> no settings_build -> fallback to old os_build.

So if we decided to make them None as default for every conanfile, this would break.
But I am ok with it, lets keep it as-is

@jgsogo jgsogo merged commit 63b6a28 into conan-io:develop Apr 30, 2020
2 checks passed
@jgsogo jgsogo deleted the feat/xbuild-profiles-cross_building2 branch Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants