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

[3/n] [xbuild] Populate 'settings_build' and 'settings_target' in conanfile #6769

Merged
merged 14 commits into from Apr 29, 2020

Conversation

jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 31, 2020

Changelog: Feature: Populate settings_build and settings_target in conanfile (only if provided --profile:build).
Docs: conan-io/docs#1678

  • the ancestors related modifications: move them to a different PR, probably a set with (name, context) is enough, no need for the NodeOrderedDict.

  • How to disambiguate --build? It is building everything that matches the name in the host and build contexts. Is it the best/only approach to use --build:host and --build:build?

  • Test: if a recipe declares itself as a build_requires, it should be detected as a loop.

Repo to work with this PR: https://github.com/jgsogo/conan-xbuild#cross-building

Next step in the xbuild feature is to provide settings from other contexts to the conanfile to make actual cross building possible. Now we already have conanfile.settings attribute, this PR adds conanfile.settings_build and conanfile.settings_target:

image

In the previous graph there are three different kinds of recipes regarding their context:

  • Recipes in the host context (app->gtest) will have:

    • settings: equals to the profile provided in --profile/--profile:host
    • settings_build equal to the settings that will apply to the build context (this should substitute os_build and arch_build completely).
    • settings_target equals None, there aren't settings available for it. If we are compiling a cross compiler we will need to inform about the target using an option.
  • Recipes in the build context (protoc->gtest/protobuf) will have:

    • settings: equals to the profile provided in --profile:build
    • settings_build will be equal to those provided in the profile_build
    • settings_target will be equal to the settings corresponding to the profile_host (this should substitute os_target and arch_target completely)
  • Recipes in the build context that are build_requires(build) of recipes in the build context (cmake):

    • settings: equals to the profile provided in --profile:build
    • settings_build will be equal to those in the profile_build
    • settings_target will be equal to those in the profile_build, these build_requires are targeting packages that are already in the build context.

Closes #6881

@jgsogo jgsogo added this to the 1.25 milestone Mar 31, 2020
@uilianries
Copy link
Member

@uilianries uilianries commented Mar 31, 2020

I have few questions:

  • Why do you need GTest in the Context build?
  • settings_build is a real attribute and can replace the usage for profile_build. Is that correct?
  • Why did you name settings_target instead of settings_host, as it's related to the host? I know there is the triplet logic behind, but don't you think this can confuse the users?

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Mar 31, 2020

Hi, @uilianries Packages in the graph (and the test in the PR) are just an example

  • Why do you need GTest in the Context build?

Just imagine that the protoc recipe has that build_requires and we are building all the graph from sources.

  • settings_build is a real attribute and can replace the usage for profile_build. Is that correct?

I'll add this to the PR description: settings_build and settings_target are intended to be a conanfile attribute like current self.settings.

  • Why did you name settings_target instead of settings_host, as it's related to the host? I know there is the triplet logic behind, but don't you think this can confuse the users?

Depending on the context the settings_target can match the profile_host or the profile_build, I've enumerated three types of recipes depending on the context and they have three different colors in the graph.
cmake is a build_requires-build of protoc, which is a build_requires-build of app

@uilianries
Copy link
Member

@uilianries uilianries commented Mar 31, 2020

I'll add this to the PR description: settings_build and settings_target are intended to be a conanfile attribute like current self.settings.

Could they be a sub-settings attribute? self.settings.build self.settings.target

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Apr 1, 2020

I'll add this to the PR description: settings_build and settings_target are intended to be a conanfile attribute like current self.settings.

Could they be a sub-settings attribute? self.settings.build self.settings.target

I don't think so. Current self.settings has the structure of the settings.yml file (constrained to the settings declared in the recipe). IMO, adding those fields as sub-settings would be counterintuitive. OTH, if those are sub-settings we wouldn't need to change the signature of many tools, with this PR-feature many of them will be changed to my_function(settings, settings_build, settings_target,....) or to my_function(conanfile,...).

@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Apr 8, 2020

Running examples of actual cross building using this PR: https://github.com/jgsogo/conan-xbuild#cross-building

  • Up to Canadian Cross: cross compiling a cross compiler

🎉

@jgsogo jgsogo requested a review from danimtb Apr 17, 2020
Copy link
Member

@memsharded memsharded left a comment

This is looking good. Both concept and implementation looks fine to me.

Please have a look to the minor comments (the ancestors is the only one that could have some issue)

I am going to review the other PR (4/n), but this could be taken out of draft, and almost good to merge.

conans/client/graph/graph_manager.py Outdated Show resolved Hide resolved
conans/client/graph/graph.py Show resolved Hide resolved
conans/client/graph/graph_builder.py Outdated Show resolved Hide resolved
@jgsogo jgsogo marked this pull request as ready for review Apr 22, 2020
@jgsogo
Copy link
Member Author

@jgsogo jgsogo commented Apr 28, 2020

Ok, now it is not populating the settings_target attribute for a build_requires that doesn't imply a context_switch

@memsharded memsharded merged commit e17f052 into conan-io:develop Apr 29, 2020
2 checks passed
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.

3 participants