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

[feature] Separate settings/options attributes for build & host contexts #8292

Closed
1 task done
theodelrieu opened this issue Jan 6, 2021 · 5 comments
Closed
1 task done
Milestone

Comments

@theodelrieu
Copy link
Contributor

theodelrieu commented Jan 6, 2021

Hello,

I'm currently migrating all my recipes to be compatible with dual profiles, and I've encountered two issues with toolchain-like packages.

recipe-declared options and settings must be the target ones

This is a direct consequence of the fact that there is no way to specify options/settings for a specific context.
Therefore, options becomes the target ones, or rather the intersection of build_options/host_options/target_options (same for settings), and thus must be given a value, even if it will be unused in a context and even deleted in the package_id method.

Otherwise, Conan will complain: ERROR: darwin-toolchain/1.1.0: 'settings.arch' value not defined.

test_package becomes useless

Since test_package uses the same profile that was used to create the package, it cannot work for toolchain packages.

When creating the android_ndk_installer package with the macos-x86_64 profile, it will require android_ndk_installer with os=Macos, which makes no sense. Also, no compiler flags will be set, as settings_target are not defined:

    def package_info(self):
        # currently creating android_ndk_installer
        if not self._in_build_context:
            return
        # used as a build requirement, define C++ compiler flags et. al
        # ...

The only workaround I found is to completely remove the test_package folders from my toolchain recipes.

Proposed improvement

There should be a way to specify separate settings/options:

class DarwinToolchainConan(ConanFile):
    # arch is not needed when creating the package
    settings = { "os": ["Macos"], "build_type": ["ANY"], "compiler": ["apple-clang"] }
    options =  { "shared": [True, False],
                        "fPIC": [True, False],
                        # I clone LLVM and build libc++/libc++abi for every darwin arch/os variant,
                        # hence all these options 
                        "llvm_version": "ANY",
                        "ios_version_min": "ANY",
                        "macos_x86_64_version_min": "ANY",
                        "macos_armv8_version_min": "ANY"}
    settings_target = { "os": ["Macos", "iOS", "watchOS", "tvOS"], "arch": [...] }
    # no options_target needed in this case

To be extra specific and avoid confusion, settings could instead be named settings_host, that way settings would only be used by regular recipes.

New sections should be introduced in profiles as well, like @jgsogo mentioned in #7688 (comment).

Looking forward to your inputs!

PS:
I've talked with some peoples of the Conan team about the idea of having separate recipe types, since toolchains are quite different than regular libraries. This could be a feature introduced in Conan 2.0, and I think that this proposal is a building block of it.


@a4z
Copy link
Contributor

a4z commented Jan 6, 2021

That's a difficult one
lets look at the ios-cmake, for example.
All you need to know is that there is a cmake toolchain, you can test that even if you are not in the host context.
or, as the ios recipe does, only test if you run in the iOS context, because you can do that with it.

you can not do that with the Android NDK, but how to verify that the NDK works? you need to build various different packages, cmake, autoconf, makefile based, ......

for an other build dependency, like a code generator, I am just interested that the executable file is in the path, and there I do not need the host profile at all, just the build one.

So I do thing that there are different levels of testing

  • unit test
  • integration test
  • system test

The unit test for a build dependency package might be just some sanity checks,
and you might need an integration test, does it build some complicated packages (openssl, curl, something with cmake is my minimum test for ndk and ios tool chain for example).
The system test is, does it build all I need, and if that all works, than I can release it

However, #7688 is a valid use case, because something you need both, the host and the target to create a package. That's also difficult, but might lead in the right direction and provide a way to have 2 profiles available.

I have no idea how much should go into the test method, but I do fear that it becomes easy to make the test method requirements too complex and too costly.
And that this might prevent people form sending something to the cci, because you might have to live in fear you will get a lot of change requests for a PR that eats up your time that you do not have anyway, and then you prefer to keep something that works for you, since it passed your integration and system test, in you personal fork.
And the integration and system tests can never be done in the test method anyway, but now I start to repeat myself :-)

@memsharded
Copy link
Member

Hi @theodelrieu

There are too many different things in this issue, maybe we need to split them (not now, in the future) into separate discussions.

One tip: the constraint of settings in recipes like settings = { "os": ["Macos"], "build_type": ["ANY"], "compiler": ["apple-clang"] } will probably be deprecated, and forbiden values of settings should be handled in a validate() method.

We are aware of the limitations of test_package for testing build_requires, indeed this has to be addressed.

This is very good information, it will be useful for the new graph and cross building model, this is highlighting a need, it could be a new "recipe type" or maybe a new "relation type", and there could be a tool_requires, a test_requires, a build_requires... This is the kind of thing we need to sort out for Conan 2.0 new graph model.

@theodelrieu
Copy link
Contributor Author

@a4z

but how to verify that the NDK works?

I don't think this should be test_package role. To me, it's more of a safe-guard against bugs in package_info, e.g. wrong STL include paths, missing compiler flags and so on.

@memsharded

it will be useful for the new graph and cross building model

Glad to hear, looking forward to the discussions.

forbiden values of settings should be handled in a validate() method

Indeed, I'm only restricting settings like that in code samples ;)

@jgsogo
Copy link
Contributor

jgsogo commented Jan 7, 2021

About testing packages that are to be used as build_requires: #7132 For sure we need something, and it will be useful in the context of conan-io/conan-center-index.

@theodelrieu we did some early research about this topic here (https://github.com/jgsogo/conan-poc-graph-cpp), but we need to allocate more time to keep working on the conceptualization before proposing actual implementation. IMO, not only the role of the relation (which can only be fully declared by the consumer) but the visibility (interface, public, private) should be added to the self.requires(....) declaration. Then Conan can decide the information to propagate.

About options/settings related to target machine on your first commit: when you build the compiler-like package (android-ndk, gcc,...) you need information about the target (possible binaries that can be generated when using this compiler-like package) in order to grab/build/package libraries that will be used when using the compiler. At this moment, when building the compiler-like package you have only some constraints but not all the settings (it can build debug/release, different ios, different android API levels,...). You will only have all the settings-target defined when you are actually using the compiler-like package as a build-requires (and those settings will be the host ones).

Note to myself.- Right now I think that that information is only available in the package_info method via self.settings_target attribute, but we should consider adding it to the validate() function as it can centralize this check too and information is available because the graph is already resolved.

As you can see, we are abandoning the idea of propagating... in favor of validating, which is much easier to implement and probably to understand and code in the recipes.

But still no news about #7688 and how to match host profile with the cross-compiler when there are different packages (different package-id) of the cross-compiler for different host profiles... right now, it needs to be manually done by the user: the user provides matching information in the profiles and the recipe can raise if they didn't match. Does it worth the effort and complexity to propagate options to other contexts? ATM, we don't have a clear idea on how to do it (not even syntax).

@memsharded
Copy link
Member

Conan 2.0 has settled on the settings, settings_build and settings_target for this purpose.

There are some tests for these "toolchain" cases like android, libcxx, gcc in https://github.com/conan-io/conan/blob/develop2/conans/test/integration/build_requires/test_toolchain_packages.py. Some highlights:

  • settings_target can be used in build() method to input the build
  • settings_target can be selected (opt-in) to be part of the package_id, so we can make different binaries for different targets.

Closing this issue as solved in 2.0, feedback welcome.

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

No branches or pull requests

4 participants