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

[POC] Feature: crossbuilding model #5202

Draft
wants to merge 120 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@jgsogo
Copy link
Member

commented May 22, 2019

Changelog: (Feature | Fix | Bugfix): Describe here your pull request
Docs: https://github.com/conan-io/docs/pull/XXXX

Proposal for a new cross-building model, this PR includes all the components:

  • Changes in the graph to support context switch between host and build
  • Command line interface:
    • Previous arguments without explicit context are assigned to host
    • Host context is specified using :h or :host suffixes to arguments: --profile:host, -pr:h, --env:host,...
    • Build context is specified using :b or :build suffixes: --options:build, -s:b,...
  • Adds a ConanFile::default_build_options to be able to hardcode in the conanfile.py option values for the build requires (build context)
  • Deprecate the arch_build and os_build settings: those are no longer included in the autodetected profile and a message is shown to the user if these settings are intentionally used.
  • Make host/build settings accessible from conanfile, some tools or helper will need it (Meson build helper, cross-compiling tool,...). Members conanfile.settings_host (just a property returning conanfile.settings) and conanfile.settings_build are proposed.

Possible breaking changes

  1. By default a build_requires uses build context, but command line inputs (settings, options,...) go to the host context, so existing build_requires won't get the settings/options from the command line.
    • ⚠️ Solution: change the default to host, before final review
  2. Deprecation of arch_build and os_build, how to deal with it?

Test that fail:

  • If there is a test_package, the options are not propagated upstreams
  • If we are requiring the same package from two different contexts (different package_ids) we have to sets of cpp_info, env_info and user_info that we have to mix/override/... The proposal is to have both of them: self.deps_cpp_info and self.deps_cpp_info_build and the BuildHelpers or Generators will decide which one to use.

Think about

  • Problem about reusing (do not removing the cache folder) the source directory: it was already a problem as the source method can access to options and settings, now with a xbuild model the problem could be even bigger.

Examples

Very basic examples and recipes to work with xbuilding: https://github.com/jgsogo/conan-xbuild-recipes

jgsogo added some commits May 14, 2019

add build and host to inputs related to environment, profile, options…
… and settings (old ones refers to build machine)
@lasote

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

Deprecate the arch_build and os_build settings: those are no longer included in the autodetected profile and a message is shown to the user if these settings are intentionally used.

That is breaking.

@lasote

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

By default a build_requires uses build context, but command line inputs (settings, options,...) go to the host context, so existing build_requires won't get the settings/options from the command line.
Solution: change the default to host

Mhh, but if no build input is provided, the build is a "copy" of the host, right?

build_requires = "first/0.0.0@lasote/stable"
def build_requirements(self):
self.build_requires("first/0.0.0@lasote/stable", context="host")

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

I left a comment in the general PR related to this. If the "install" is not specifying build profile or build options explicitly, shouldn't the "build" context be considered the "host" one? like a copy. Otherwise, I think we will be breaking many things, not only this usage.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

It is a little bit different (logic around conan_api.py#L1187):

  • Non-scoped settings/env/options go to host. I mean, those without the :host or :build suffixes, go to host.
  • Then, build profile will take explicit values or the default profile
  • And host profile will take explicit values or be equal to build (by default we generate native binaries).

Examples:

  • conan ... <no args> will generate binaries using the default profile for host and build.
  • conan ... -s os=Linux will generate binaries for Linux (composition rules for profile and arguments apply), using the default profile as build.
  • conan ... -s:h os=Linux, same as above
  • conan ... -s:b build_type=Release will generate binaries for the default profile, but will use Release ones for build (composition rules for profiles apply).
  • conan ... -pr:h=host -pr:b=build will use profile build to build and profile host for generated artifacts...

The breaking change that I need to revert is that, by default, build_requires use the build profile and this is breaking for released behavior (but I'm keeping it by now as it helps me to notice about tests that are using profiles and arguments).

This comment has been minimized.

Copy link
@lasote

lasote Jun 19, 2019

Contributor

I need to understand why you think it should behave that way.

The idea I always had is:

  • The build profile is None by default (if no build profile is specified, that means no explicit value with the :build scope, syntax that I like, by the way)
  • If no build profile is available, there is no "scope change" for the build requires.
  • By default a build require scope is the "build" one, but if there is no "build" scope, then there is no change (no breaking)
  • The old xxx_build settings keep working exactly the same. While we don't remove it from the settings.yml, they will be present in the build scope when specified (even if it doesn't make any sense)
  • Everything should work exactly the same, no tests changed, nothing, until the user specify any build input.

It is likely that I don't fully understand the reasons behind your design, so please, tell me.

@@ -236,5 +263,9 @@ def test_profile_requires(self):
cmake
gcc
"""
self.client.save({"profile.txt": profile, "conanfile.txt": reuse}, clean_first=True)
self.client.run("install . --profile ./profile.txt --build missing")
self.client.save({"profile_host.txt": profile_host,

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

My concern when I see a modified test is: Or we are breaking the behavior or we are not testing something anymore. I think this is case 2). Shouldn't we provide new tests but keeping the classic --profile=whatever?. Am I missing something?

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

Some tests are changed because of the breaking change (1) explained in the PR notes, before the final release that change has to be reverted and many tests will have no changes: for the release, build_requires should use the host profile too unless the user explicitly says the opposite.

But right now we are far from thinking about the release and these tests show how the host and build profiles behave for different graph scenarios, it is important to check that the context switch is done and every package is getting the expected settings, options and environment variables.

@@ -200,7 +200,7 @@ def system_requirements(self):
self.output.info("Running system requirements!!")
"""})
client.run("create . Pkg/0.1@lasote/testing")
self.assertIn("Configuration:[settings]", "".join(str(client.out).splitlines()))
self.assertIn("Configuration (host machine):[settings]", "".join(str(client.out).splitlines()))

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

I really think that when we are not using cross build profiles everything should work the same, including that message. Maybe I don't have any idea about cross-building or (very likely) don't about gnu triplet jargon and (host-machines) means nothing to me.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

I can remove the (host machine) note when both profiles are equal, it is not a big deal (simple change in manager.py#L56)

@@ -264,6 +266,7 @@ class OpenSSLConan(ConanFile):
386: False
no_asm: False
shared: False
default_build_options: None

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

Same, I don't know what are the "build options".

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

From the notes in the PR:

Adds a ConanFile::default_build_options to be able to hardcode in the conanfile.py option values for the build requires (build context)

These are options that apply to the build_requires of a recipe that are in the build context. It is the equivalent to ConanFile::default_options that apply to the requires and build_requires in the host context.

@@ -125,6 +125,7 @@ def build(self):
self.assertIn("CC/1.0@private_user/channel: Error in build() method, line 36",
my_json["installed"][0]["packages"][0]["error"]["description"])

@unittest.expectedFailure # TODO: Need CLI in the cross-building feature to fix it

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

CLI?

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

CLI ~ Command line interface.

This PR has the command line interface already merged, so it should be easy to make the needed changes to make it pass.

@@ -120,7 +120,6 @@ def profile_update_and_get_test(self):
# Now try the remove

client.run("profile remove settings.os ./MyProfile")
client.run("profile remove settings.os_build ./MyProfile")

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

breaking symptom

@@ -37,11 +37,17 @@ def get_conaninfo(info):
return load(os.path.join(folder, "conaninfo.txt"))

settings = ["compiler=Visual Studio", "compiler.version=15", "build_type=Release"]
info = api.create(".", user="conan", channel="stable", settings=settings)
profile_host = ProfileInfo(None, settings, None, None)
profile_build = ProfileInfo(None, None, None, None)

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

I think profile_build should be None (defaulted)

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

I think it is better to keep it non defaulted to None. Here in the test we need this kind of initialization, but in the production code we always have a value and there is no need to introduce and additional if-clause to check if it is None.

@@ -64,7 +64,7 @@ def test_profile_relative_cwd(self):
self.client.save({"conanfile.txt": "", "sub/sub/profile": ""})
self.client.current_folder = os.path.join(self.client.current_folder, "sub")
self.client.run("install .. -pr=sub/profile2", assert_error=True)
self.assertIn("ERROR: Profile not found: sub/profile2", self.client.out)
self.assertIn("ERROR: Host profile: Profile not found: sub/profile2", self.client.out)

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

Same as commented before. The message should be the same in the general usage without cross-build.

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

We can add an if to check if the build profile has any value and remove the extra Host profile: from the error message (conan_api.py#L1203)

@@ -0,0 +1,149 @@
# coding=utf-8

This comment has been minimized.

Copy link
@lasote

lasote Jun 17, 2019

Contributor

typo in the file name

This comment has been minimized.

Copy link
@jgsogo

jgsogo Jun 18, 2019

Author Member

It is because values are contrained using the values_contrained function (that function exists in the codebase) 😆😆

@jgsogo

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

Before making any commit I'll wait for you to finish the review (I think it is very annoying to see changing sources during a review).


About the deprecation of arch_build and os_build we need to define how to make it without breaking things (I'll add it to the PR notes). Probably there is no problem if we keep populating the autodetected profile with these values, but I think that the build profile should take precedence over them.

Scenarios to implement/think:

  • conan ... -s os_build=Linux. To keep things working, although this setting should go to the host profile (no scope), its intention is to apply to the build one. Should we make this assumption?
  • conan .... -s:h os_build=Linux. Keep this value in the host profile just in case any recipe use os_build?
  • conan ... -s:b os_build=Linux. Keep this value in the build profile just in case any recipe use os_build?
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.