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] Revisit precedence of values assigned to requirements' options (ATM, hardcoded value in configure method cannot be overridden) #7786

Closed
memsharded opened this issue Oct 1, 2020 · 12 comments
Milestone

Comments

@memsharded
Copy link
Member

Right now a recipe can do:

def configure(self):
    self.options["mydep"].myoption = "value"

This has absolute precedence. It can never be overridden, even from command line doing -o mydep:myoption=othervalue

I would suggest to change this in Conan 2.0:

  • This has the same precedence as defining it in default_options, can be overriden by command line
  • Recipes will be able to validate(), post graph expansion and raise if they do not accept some upstream configuration.
@memsharded memsharded added this to the 2.0 milestone Oct 1, 2020
@jgsogo
Copy link
Contributor

jgsogo commented Oct 2, 2020

Link to the validate() (TBD) issue: #7591

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 2, 2020

I would like to notice that it will break some CCI recipes: gdal for example.

gdal depends on qhull. qhull has an option to build either a reentrant or a non-reentrant version, which are mutually exclusives. Currently the reentrant version is recommended by qhull's authors, so default option is reentrant=True. But gdal depends on non-reentrant qhull.
What does it mean? It means that gdal can't build if it doesn't override qhull option. If @memsharded suggestion is implemented, gdal will always fail unless consumer manually overrides qhull reentrant option.

So:

  • CCI will never be able to build any gdal binaries
  • For consumer, gdal build won't work out of the box, which is an unpleasant behaviour.

IMOO:

  • a recipe should work out of the box with any valid combination of its own options, without requiring that consumer has to dig into transitive dependencies to tweak options.

  • downstream recipes should be allowed to override upstream options, as you can override upstream version, if it's a hard requirement. A warning could be displayed to notice consumers that some options won't be honored.

  • downstream recipes should never override upstream options they don't care (best practice). For example, disabling of an upstream feature should be avoided.

  • [feature] Method to be run after the graph is resolved #7591 should be implemented to validate dependency graph

  • Currently version overriding and option overriding have different behaviour, it's counter intuitive:

    • downstream has precedence for version overriding
    • upstream has precedence for option overriding

    => I think that option overriding should behave like version overriding. So a consumer can override all options of the dependency graph, without fear that some recipe takes precedence. If he sets some options incompatible with some recipes, these recipes will raise an error in validate() upcoming method (and currently in build() method).

@jgsogo
Copy link
Contributor

jgsogo commented Oct 2, 2020

The proposal in this issue is to add more flexibility. Right now if you override an option in the configure() method it is not the same as if you were overriding it in the default_options. Overriding in the configure() option assigns a final value that cannot be changed: not even using the command line. This is somehow counterintuitive, it is harder to reason about and it is inconsistent: two mechanisms to do the same that have different behaviors.

With this change, gdal can override qhull option value as it is doing right now:

class GDAL(ConanFile):
    requires = "qhull/..."

    def configure():
        self.options['qhull'].reentrant = False

The difference would be for consumers and the command line. Right now the --option argument in this line has no effect:

conan install gdal/xxx --option qhull:reentrant=False

with the proposed change, the option in the command line will be taken into account... and yes, the recipe should implement some kind of validation (#7591) to raise if it is not able to build with that value.


This change can break existing builds that rely on this final assignment, that's the reason we cannot modify it before 2.0

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 2, 2020

I misunderstood, sorry, so this modification sounds good to me :)

@jgsogo
Copy link
Contributor

jgsogo commented Oct 2, 2020

No problem 😊 We are also wrong from time to time and we really appreciate all of you questioning new features and decisions.

@Minimonium
Copy link
Contributor

Minimonium commented Oct 2, 2020

Will this also allow the current recipe option overriding to be treated in the same manner as default_options? Currently it's not possible to have conditional default values for package options without tricks such as special auto option values.

Also, regarding the upstream package option overriding - #6408

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 2, 2020

AFAIK, you can conditionally set options values in config_options().
Last time I tried with libcurl recipe, I was able to override those values in command line. I can't remember if I tried with downstream's configure(), config_options() and default_options.

@Minimonium
Copy link
Contributor

Minimonium commented Oct 2, 2020

@SpaceIm It's explicitly forbidden to set option values in config_options:

config_options() is used to configure or constraint the available options in a package, before they are given a value. So when a value is tried to be assigned it will raise an error.

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 2, 2020

See libcurl recipe: https://github.com/conan-io/conan-center-index/blob/4ac63884d528d6abb2d4740acb88d5cbd3d6b76a/recipes/libcurl/all/conanfile.py#L85
It builds, and if you don't override options values, you'll see that values assigned in config_options() are honored.

@Minimonium
Copy link
Contributor

@SpaceIm Right now I don't recall exact specifics about when I was having problems with the feature, but here the discussion on which I based my current knowledge is: #3519
To my knowledge the behaviour in the libcurl's recipe is not guaranteed.

@jgsogo
Copy link
Contributor

jgsogo commented Oct 5, 2020

Unfortunately, Conan v1.x is not enforcing the scope for some attributes (access and modification) and it hasn't been exhaustively documented, so we feel like changing it (or being more strict) might be considered breaking. Conan v2 should enforce it and make available to each function only the attributes that can be modified. At some point in time we will be more strict in ConanCenter so we detect these errors and fix them before moving to Conan 2.0 (probably we will take advantage of CONAN_V2_MODE variable).

@jgsogo jgsogo changed the title [feature] Remove hardcoding of dependencies option in recipes [feature] Revisit precedence of values assigned to requirements' options (ATM, hardcoded value in configure method cannot be overridden) Oct 5, 2020
@memsharded
Copy link
Member Author

The precedence has already been modified for Conan 2.0.

Downstream options values will always have higher precedence.
The profiles and CLI options values will always have higher precedence than the recipes.

This enables things as settings options values conditionally in recipes methods (no need to conditionally define the defaults, as defining the values is good too, as downstream values still prevail).

So I think we can close this issue as solved in 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants