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

[question] Force option values on requirements #15519

Closed
1 task done
frogarian opened this issue Jan 24, 2024 · 10 comments · Fixed by #15571
Closed
1 task done

[question] Force option values on requirements #15519

frogarian opened this issue Jan 24, 2024 · 10 comments · Fixed by #15571
Assignees

Comments

@frogarian
Copy link

What is your question?

The Documentation of the configure() method (https://docs.conan.io/2/reference/conanfile/methods/configure.html) states the following:

The definition of options values is only a “suggestion" [...]

Is there any chance to force an option onto a dependency. Or at least automatically check if the set option was actually used?

In our case we have several libraries, all depending on Qt. Some of them need specific modules of Qt, so they configure those in their recipe. But this only seems to have an effect in some cases, totally not obvious when.

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded memsharded self-assigned this Jan 24, 2024
@memsharded
Copy link
Member

memsharded commented Jan 24, 2024

Hi @frogarian

Thanks for your question.

Is there any chance to force an option onto a dependency.
To clarify, this is forcing the dependency option, except in some cases:

  • If some other more downstream consumer decides to pass a different option value, the more downstream always have precedence (imaging your consumer conanfile.py setting up dependencies options to some values and just being ignored because some intermediate dependency is deciding otherwise)
  • If the user profile or command line options define otherwise (same principle, the most downstream always have precedence)

Or at least automatically check if the set option was actually used?

Yes, packages can validate that the options of their dependencies are the correct ones in their validate() methods, and raise if the values are not valid

In general the recommendation are:

  • Define that default_options in the source package (in this case qt), to be what you need
  • Define the options that you need in your profile files. This is the recommendation in most cases.
  • Define the options that you need in conanfile files. This can be messy, because there can be multiple conanfiles setting different values, conflicting, etc, so not recommended in most cases

@frogarian
Copy link
Author

If some other more downstream consumer decides to pass a different option value, the more downstream always have precedence (imaging your consumer conanfile.py setting up dependencies options to some values and just being ignored because some intermediate dependency is deciding otherwise)

I understand this point when the more downstream consumer actually sets a different value for the option.
But in our case it seems like more downstream consumer also override the value with the default value when they do NOT explicitly set it.
This behavior seems a little strange to me. Wouldn't it be nice to at least have the chance to force some option into the graph?

@memsharded
Copy link
Member

But in our case it seems like more downstream consumer also override the value with the default value when they do NOT explicitly set it.

Yes, it is true, the first one requiring the package will define its set of options.
We kind of had that in Conan 1.X and it was a neverending stream of issues related to settings options, bugs, confusions... really complicated. So we decided in 2.0 to simplify, and recommend to avoid defining options in recipes as much as possible, and approach it by defining the right default in the dependency or in profiles.

This behavior seems a little strange to me. Wouldn't it be nice to at least have the chance to force some option into the graph?

We have this ticket to investigate this: #14764. I think it would be mostly what you are asking for here, so maybe we can close this ticket and follow up there.

The main issue is that it is not evident. While it seems that it is desired to be able that some recipe "forces" some option of the dependency, this also brings extreme confusion and frustration when users try to define the value of that option in their command line or in their conanfile and it seems that Conan is failing at that, and they will report a bug against Conan.

@frogarian
Copy link
Author

I get your point but to me the way it is now in Conan 2 might also lead to confusion. To the user it is not evident which package is the first to require some other package and thereby define its options. And since defining them is just a suggestion, it seems more or less useless in those cases where a dependency is injected from multiple packages.

If on the other hand a user is able to force the option into the graph you might be able to output this to the user and it is at the very least visible in the recipe.

Regarding your feature request I'm not sure if this actually covers my problem.

In our case for example we have, amongst others, two packages A and B, which both have a dependency on Qt. Where package B has a strict requirement for the qt websockets submodule, which in conan is modeled as option. But if now package A for some reason is the 'first' to require Qt in the graph, package B will be left with a Qt package it cannot use due to the missing qt websockets module. And since we provide only those libraries A and B as part of a bigger bundle of packages we don't have a toplevel recipe to pin down some options and the users of our stack must define lot's of options we could have provided them.

Hope this example demonstrates our dilemma a little better. I'm not sure if this is unique for our use case or why this didn't come up already, but sadly for our case this is really a step back compared to Conan v1.

@memsharded
Copy link
Member

memsharded commented Jan 25, 2024

Hope this example demonstrates our dilemma a little better. I'm not sure if this is unique for our use case or why this didn't come up already, but sadly for our case this is really a step back compared to Conan v1.

The problem is that Conan 1.X had some important bugs in this regard with the graph expansion. Maybe they were not hitting you, as it worked well for some cases, but it was a nightmare in many others, it was mostly flawed. Any package that had conditional requirements based on options and these options would be defined from different consumer packages would easily lead to broken dependency graphs, but in so subtle ways that it could takes tons of time to debug and understand what was happening.

Let me put a real example in form of a failing test in Conan 1.X to demonstrate one of the issues:

def test_qt():
    c = TestClient()
    qt = textwrap.dedent("""
        from conan import ConanFile
        class Qt(ConanFile):
            name = "qt"
            version = "0.1"
            options = {"ssl": [False, True]}
            default_options = {"ssl": False}
            def requirements(self):
                if self.options.ssl:
                    self.requires("openssl/0.1")
            """)
    c.save({"openssl/conanfile.py": GenConanfile("openssl", "0.1"),
            "qt/conanfile.py": qt,
            "sub1/conanfile.py": GenConanfile("sub1", "0.1").with_requires("qt/0.1"),
            "sub2/conanfile.py": GenConanfile("sub2", "0.1").with_requires("qt/0.1")
                                                            .with_default_option("qt*:ssl", True),
            "app/conanfile.py": GenConanfile("app", "0.1").with_requires("sub1/0.1", "sub2/0.1")
            })
    c.run("create openssl")
    c.run("export qt")
    c.run("export sub1")
    c.run("export sub2")
    c.run("install app --build=missing")
    print(c.out)
    assert "openssl" in c.out  # This fails, openssl not there!

Even if sub2 package is explicitly defining qt:ssl=True option, the graph is being resolved without openssl. Conan 2 is not really a step back, it was not working in Conan 1.X either.

The only approach that we found that can be managed easily and consistently is defining the optionality in one place. Either in the upstream recipe, in this case Qt or in the downstream common profile file. The correctness of optionality for specific packages can be easily enforced checking in validate().

@frogarian
Copy link
Author

But isn't this test case failing in Conan v2 as well?

@memsharded
Copy link
Member

Sure. The recommendation for defining options is to centralize it in the dependency recipe or in profiles, both in 1.X and 2.0. Trying to distribute options definitions among multiple recipes in the graph is problematic both in 1.X and 2.0, it is an intrinsic problem of decentralization of information in something that doesn't evaluate linearly, like a graph.

@frogarian
Copy link
Author

Ok, so it seems that until now with Conan v1 we just got lucky that it somehow worked out for us and with Conan v2 we finally have to stick to those rules.

Thanks for clarifying this.

@memsharded
Copy link
Member

I am submitting #15571, that is marking this question to be closed.
In that PR I am proposing a new "strong" definition of options that will have more priority than downstream or profile regular options (though downstream or profile "strong" options will still have precedence).

This addresses some of the issues related to this question. But I have been revisiting the "diamond" problem with different options being assigned in different branches, and it is really a challenging problem, that would require a completely different graph computation algorithm, way more costly and slower (requires back-tracking and constant re-evaluation of the graph, increasing from linear to at least polynomial). So the above recommendations are still valid.

Thanks for the feedback!

@memsharded
Copy link
Member

Closed by #15571, it will be included in next release, but it is possible that initially as undocumented, to see if it could be breaking something first, then document in later future releases.

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 a pull request may close this issue.

2 participants