conan-io / conan

Conan - The open-source C and C++ package manager
https://conan.io
MIT License
8.31k stars 986 forks source link

[bug] Conan package option conflicts do not lead to error #17190

Closed stevn closed 1 week ago

stevn commented 1 month ago

Describe the bug

Environment: MacOS 14, apple-clang 16, conan 2.8.0

[settings] arch=x86_64 build_type=Release compiler=apple-clang compiler.cppstd=gnu17 compiler.libcxx=libc++ compiler.version=16 os=Macos

Description:

Conan options handling is flawed:

Hard-coded package options may be overridden silently without the user noticing that something bad is happening under the hood.

Conflicting options should lead to an error instead of silently being modified.

Example scenario: https://github.com/stevn/conan2_options_test

Introduction

In this reproduction scenario a library package stuff/1.0.0 is required by an application package stufftool/1.0.0.

There is a (transitive) dependency on libtiff:

In this example, let's assume that we want to disable the jbig option of libtiff. The JBIG package is very problematic for closed-source software because it is licensed under GPL.

Therefore the stuff library makes sure to disable the jbig option by hard-coding the option to False in the configure() function of its conanfile.py recipe.

Build + Reproduce

On Unix-like operating systems run:

./create_all.sh

This currently produces this error:

CMake Error: Could not create named generator DummyGeneratorToPreventBuildingOfJBIG

Here the build of jbig has been made to fail on purpose by setting a non-existent CMake generator name DummyGeneratorToPreventBuildingOfJBIG for jbig in global.conf. This shall stop the build process as soon as Conan attempts to automatically build JBIG, because we do not want that.

Error summary

When building stufftool, its opencv dependency defaults to libtiff with jbig=True option. This leads to its stuff dependency silently becoming "corrupted" in a certain way.

Even though it configures a really important option, the hard-coded option value jbig=False is ignored and the stuff library is rebuilt with jbig=True! The stuff library with JBIG enabled should never exist, as it is explicitly disabled in its recipe.

Conan apparently automatically tries to "make things work" by only setting the options of a package once and then ignoring the later configure() calls. That seems problematic.

Expected behavior

I would have expected Conan to throw an error message stating that a package options conflict has been found in the dependency tree.

How to reproduce it

Run these commands:

git clone https://github.com/stevn/conan2_options_test
cd conan2_options_test
./create_all.sh
memsharded commented 1 month ago

Hi @stevn

Thanks for your feedback and for your reproducible code.

However, I am afraid that this is expected behavior. It was this way in Conan 1, and it was changed in Conan 2 because of being too problematic.

The new behavior is documented in

https://docs.conan.io/2/tutorial/versioning/conflicts.html#overriding-options

With warnings:

Defining options values in recipes does not have strong guarantees, please check this FAQ about options values for dependencies. The recommended way to define options values is in profile files.

And best practices

As a general rule, avoid modifying or defining values for dependencies options in consumers conanfile.py. The declared options defaults should be good for the majority of cases, and variations from those defaults can be defined better in profiles better.

And also https://docs.conan.io/2/knowledge/faq.html#defining-options-for-dependencies-in-conanfile-py-recipes-doesn-t-work

There there are recommendations like:

In general, it is more recommended to define options values in profile files, not in recipes. Recipe defined options always have precedence over options defined in profiles.

memsharded commented 4 weeks ago

Hi @stevn

Any further question or issue here? If not maybe we can close the ticket? Thanks for your feedback.

stevn commented 4 weeks ago

Hi @memsharded,

I am not sure what to say. I have noticed that the Conan client has been pushing a lot of different settings and package-specifics into the Conan profile. The package options is just one example.

This sounds like a wrong design decision to me. (But I must admit I am probably not aware of all of the background details that led to that decision...)

To me there should be a separation of concerns. The Conan profile should be concerned about things like compiler version, operating system and ABI. So if we for example have a project that targets Linux, Windows and MacOS on ARM64 and x86_64, I would expect to find exactly these profiles. So maybe something like 6 profiles. Maybe with address sanitizer profiles you would get a slightly higher count, but overall still easily manageable. That makes a lot of sense to me.

On the other hand, having profiles that define all options of all dependencies that ever existed doesn't make sense to me.

If I have software that supports let's say 10 different feature flags, I wouldn't expect to need to deal with handling all combinations of OS, CPU and compiler version with all combinations of possible feature flags. That would result in a situation which kind of "explodes", producing hundreds of different profiles, where each profile needs to know the exact dependency tree and all options of all packages therein. That sounds a bit crazy. The profile cannot know such things, they are too complicated. I would describe them as internals of the packages. There should be no need to extract these internals and store them in the profile.

Instead I would expect it to behave dynamically, where each dependency's recipe only knows its own feature flags (which resemble a package's configuration interface in a way) and its own dependencies and their feature flags.

For example, lets assume there is an image processing project called imgproc that can be built with or without TIFF support. It will therefore need to have a Conan package option to configure this feature flag, let's call it feature_tiff. When the Conan package is being built with TIFF support enabled, we set imgproc/*:feature_tiff=True on the Conan create command line. The command line defines the top level configuration: the package to create, the target OS and CPU type, compiler version and feature flags of that package. At this point it is not possible to know all the internals of all dependencies of that package. Now, when building its own source files, the imgproc recipe will enable that feature in its own compile definitions (e.g. via CMake: target_compile_definitions(imgproc PRIVATE FEATURE_TIFF=1)). It will also enable TIFF support in OpenCV via self.options["opencv"].with_tiff=True. This is an easy example, but in a complex project these are details that are easy to know on the recipe level, but impossible to know on the top-level command line, which is where the Conan profile lives.

In my understanding the Conan profile should be completely unrelated to those things and should just define some CPU arch, compiler version etc.

I am aware that these Conan options can lead to a conflict. In the case of a conflict, I would much rather be notified of the conflict and deal with it appropriately than have something undefined "magically" compile and link. A package option conflict when left unresolved in my perspective leads to undefined behavior, the danger of which should not be underestimated.

I am not sure how to continue here. As you said, Conan devs decided differently. A decision I cannot understand.

Maybe there is a path forward by introducing some global config to error on package options conflict being detected? I'd be happy to hardcode that to true in all of my scripts.

But in the end I believe it is actually a more fundamental question of how a package manager should behave.

memsharded commented 4 weeks ago

To me there should be a separation of concerns. The Conan profile should be concerned about things like compiler version, operating system and ABI. So if we for example have a project that targets Linux, Windows and MacOS on ARM64 and x86_64, I would expect to find exactly these profiles. So maybe something like 6 profiles. Maybe with address sanitizer profiles you would get a slightly higher count, but overall still easily manageable. That makes a lot of sense to me.

Our learnings is that this is not a complete view. The options you define, how you use your dependencies is too often equally critical, some examples:

This is why we encourage to try to keep the recipes defaults as much as possible, and not assign different options values from recipes, to align as much as possible with the users expectations. You could be building and application and assuming a default static linkage of your dependencies (most common default in Conan and other tools) but some recipe in the middle of the graph could be deciding otherwise with a *:shared=True, and your application will depend on shared libraries.

It is not that the end-users should be aware and define absolutely all options values of its dependencies, but the opposite. Defining values for options of dependencies in recipes actually increases the amount of issues and the need of the end user profiles to define the value of options they want. The idea is that the recipes defaults should be good for majority of cases, and those cases which those defaults are not enough, they should be explicit in user profiles, as them being hidden inside dependencies recipes.

Regarding your imgproc example, maybe the package should define a consistent with_tiff option, so the approach to define it from the consumer is *:with_tiff=True. Having the same feature toggle with different names in different packages and having there conditional logic to propagate it sounds mostly as an anti-pattern. It is true that the "feature toggles" is often different to other options usages, and probably the one with less requirements from the end-consumer. But the other options cases are even more common, so they also kind of weighted the design in that direction.

That was mostly the rationale, and so far and given the users feedback that moved to Conan 2 already, we are quite happy with the decision.

But said that, I also agree with you that it would be good to have some better diagnosis when dependencies recipes do change their dependencies options defaults, and those changes are not prioritized in the presence of other packages that didn't use the same defaults. Even if the recommendation would be to raise ConanInvalidConfiguration when the lack of a given option value will really produce an error (and otherwise, it would mean: "it is not really important the dependency option value, it was not critical"), I think some default reporting of mismatches in options values when there are diamonds in the graph with different values in the branches could be worth it. Not sure if a hard "conflict" error should be done here, but if we add something as a warning, then the warn-as-error feature could make it. I'll have a look to see what can be done.

Thanks for your feedback!

memsharded commented 4 weeks ago

I was checking this, and I see that Conan is already reporting when these conflicts are happening, with something like:

Options conflicts
        liba/0.1:myoption=1 (current value)
             libc/0.1->myoption=2
        It is recommended to define options values in profiles, not in recipes

Do you see that information in your output? If you see it, I think this feature would be quite easy, we just need to add a warning to the output saying, "there are options conflicts in your dependency graph", and then the warn-as-error will do the rest, what do you think?

stevn commented 3 weeks ago

Package options

Shared option and the star pattern match

:shared=True/False some recipe in the middle of the graph could be deciding otherwise with a `:shared=True`

I do not use the star character (*) to pattern-match all packages in the dependency tree. Especially doing so in a recipe seems very bad indeed. Even on the command line that seems a bit error-prone to me.

Not every package necessarily has the option shared. So without knowing all the internals of the complete package tree you cannot use this pattern and expect it to always work. But I must admit I am not entirely sure what would happen. Will Conan ignore the option value set by star pattern-match if it doesn't exist on a package in the dependency tree?

Anyway, I only seldomly set an option of a package in the middle of the dependency tree on the command line. If I know a specific package supports being built as shared or static version and that the depender also handles that correctly, then it makes sense to be able to decide this on the top-level (e.g. on the command line). But then I would set that option only on that specific package. I would expect this to fail with an options conflict though, if the direct depender of that package only supports using that package with one specific configuration.

Here is an example:

In this example scenario I would recommend that:

However, in this example scenario I would expect these commands to work OK:

conan create myproductionapp # defaults to mylib with shared=False
conan create myproductionapp -o "mylib/*:shared=False" # same as above, but explicit
conan create myproductionapp -o "mylib/*:shared=True"

In this example scenario I would like to get an error by default if I use such a command line because the experimental app would otherwise break:

conan create myexperimentalapp -o "mylib/*:shared=True"

However, if I really know what I am doing and know that in fact this works OK, I would then like to be able to override that error and proceed anyway by ignoring package option conflicts. When using this (theoretical) --ignore-option-conflicts I am aware that I am entering an experimental world because I am overriding something in the recipe in a direction for which it wasn't intended, that wasn't tested and therefore is unlikely to work.

conan create myexperimentalapp -o "mylib/*:shared=True" --ignore-option-conflicts

critical architectural decision

That is correct. These options have a lot of different implications. It is therefore very important to get this right. I would argue that this is the reason why hardcoded option values in recipes should not be ignored.

I would make the distinction between defaulted option values and explicitly configured option values. If I configure something explicitly in my recipe I want that to be honored by default. If it is not being honored, then what is the point of setting it in the first place?

Unnecessary dependencies

Bringing additional unnecessary components to the consumer applications, that might overlink. Often Conan optional components in packages are activated via options, like with_zlib. If packages decide to change that default, that will bring your extra dependencies and functionality that your consumer applications might not need at all.

Yeah I am not sure how to deal with that. It seems like these packages should not be changing the defaults if they don't really need that stuff.

Some very important features, like enabling SSL are very often controlled by a with_ssl option. This is again a very "architectural" decision, not something to let some intermediate package in the graph to change the defaults, and for example disable SSL support. keep the recipes defaults as much as possible

I agree, the intermediate packages should not change defaults for everyone else.

Validation

If there is a package that cannot work at all with some of its dependencies defaults, it can raise a ConanInvalidConfiguration" in its validate() method.

Yes, but doing so with every Conan package option that I set in my recipe is very tedious and error-prone. I would like to keep it simple.

Validation example

https://docs.conan.io/2/tutorial/versioning/conflicts.html#overriding-options

In this documentation an intro package is mentioned that defaults to using a matrix dependency as shared lib:

class Intro(ConanFile):
    name = "intro"
    version = "1.0"
    default_options = {"matrix*:shared": True}

    def requirements(self):
        self.requires("matrix/1.0")

    def validate(self):
        if not self.dependencies["matrix"].options.shared:
            raise ConanInvalidConfiguration("Intro package doesn't work with static matrix library")

This makes sense to me. The default_options are well-defined. At this point it looks like the shared option of matrix defaults to True, but could also work with False. Then later in the validate() method we error out when someone tries to do that.

My first thought on this is that maybe there should be a more elegant way to declare all the configurations that you are compatible with on a recipe level. But this "scripting" approach will of course work as well.

Side note: I would recommend that "matrix*:shared" should be written as "matrix/*:shared" instead to avoid conflicts with a package that might also start with "matrix", e.g. matrixhelper or so.

Unnecessary validation example

The above example is fine, but doesn't really represent the problematic use case, for example:

class Intro(ConanFile):
    name = "intro"
    version = "1.0"
    options = {"imgio": [False, True]}
    default_options = {"imgio": True}

    def requirements(self):
        self.requires("opencv/4.10.0")

    def configure(self):
        self.options["opencv"].with_tiff = bool(self.options.imgio)
        self.options["opencv"].with_jpeg = bool(self.options.imgio)
        self.options["opencv"].with_png = bool(self.options.imgio)

    def validate(self):
        if  (self.dependencies["opencv"].options.with_tiff != bool(self.options.imgio)) or
            (self.dependencies["opencv"].options.with_jpeg != bool(self.options.imgio)) or
            (self.dependencies["opencv"].options.with_png != bool(self.options.imgio)):
            raise ConanInvalidConfiguration("Explicitly set conan option was ignored by Conan itself and overridden somewhere magically for some unknown reason.")

It seems unnecessary and clumsy to have to check everything I set in the configure() method in the validate() method.

But it currently looks like this might be something I have to do for now. I am currently thinking about putting this in my python_requires base class, storing a dictionary of options that I really do want to apply, setting these in configure() and later checking these in the validate() method.

class Intro(ConanFile):
    name = "intro"
    version = "1.0"
    options = {"imgio": [False, True]}
    default_options = {"imgio": True}

    # BaseRecipe has a `_forced_options = {}`
    python_requires = "baserecipe/1.0.0"
    python_requires_extend = "baserecipe.BaseRecipe"

    def requirements(self):
        self.requires("opencv/4.10.0")

    def configure(self):
        # Configure all options on dependencies and
        # store an internal dictionary of options (self._forced_options)
        # to be validated against later.
        self._set_dep_option("opencv", "with_tiff", bool(self.options.imgio))
        self._set_dep_option("opencv", "with_jpeg", bool(self.options.imgio))
        self._set_dep_option("opencv", "with_png", bool(self.options.imgio))

    def validate(self):
        # Enforce that all options in self._forced_options are really applied
        # and not overridden magically by Conan.
        self._validate()

I haven't tried if this works yet... Any thoughts?

Further points

Defining values for options of dependencies in recipes actually increases the amount of issues

I would agree that recipes should not touch options they don't care about.

I would argue that each recipe must set exactly those options of dependencies that they really depend on. If an option conflict then is the result of this, this is the kind of error I want to deal with myself and resolve explicitly.

The idea is that the recipes defaults should be good for majority of cases, and those cases which those defaults are not enough, they should be explicit in user profiles, as them being hidden inside dependencies recipes.

I don't agree here. They should be explicit in the recipe, not in user profiles, because the recipe is the only place where the package can know what it really depends on. In some cases the user profile might actually know that it wants to tweak some option of some intermediate package. But in most cases the user profile doesn't know all these recipe internals and therefore should not accumulate these.

consistent with_tiff option, so the approach to define it from the consumer is *:with_tiff=True. Having the same feature toggle with different names in different packages and having there conditional logic to propagate it sounds mostly as an anti-pattern

Yeah this was a bad example. I used a different name (feature_tiff) for that option on purpose to try to make it distinct and not be confused with the with_tiff option of the OpenCV package.

Checks

I was checking this, and I see that Conan is already reporting when these conflicts are happening: It is recommended to define options values in profiles, not in recipes Do you see that information in your output?

Yes I remember seeing something like that somewhere in the logs after being confused for several days about how Conan options work and why my code is not compiling or sometimes behaving strangely etc.

Warn-as-error

if we add something as a warning, then the warn-as-error feature could make it. warn-as-error will do the rest, what do you think?

I personally have bad experiences using the warn-as-error concept across different contexts. There are cases where warn-as-error helps write good code and there are cases where it becomes a nightmare. Basically when I write new code with a certain compiler, warn-as-error helps me use the features of that compiler version properly. But when I want to use that code a year later with a different compiler, I cannot spend lots of hours just to fix some irrelevant warnings of a mature package (and likely introduce new bugs while doing so).

I feel like the same principle will apply to Conan recipes and the Conan client. Warn-as-error is nice to have while writing recipes, but in the end everyone will be disabling warn-as-error in their other daily tasks, which might lead to this kind of warning being overlooked.

In the end I think it is important to make the right decision what really must be treated as an error and warn-as-error doesn't help with that.

memsharded commented 3 weeks ago

I do not use the star character (*) to pattern-match all packages in the dependency tree. Especially doing so in a recipe seems very bad indeed. Even on the command line that seems a bit error-prone to me.

Not every package necessarily has the option shared. So without knowing all the internals of the complete package tree you cannot use this pattern and expect it to always work. But I must admit I am not entirely sure what would happen. Will Conan ignore the option value set by star pattern-match if it doesn't exist on a package in the dependency tree?

Yes, Conan will ignore the option value if a given package does not declare it, as long as it is a pattern like *. If using an exact match, then it will complain if the recipe doesn't define the option.

I think our experience tell otherwise, specially with the shared option. Having all the dependencies as static or having all dependencies as shared libraries work in general reasonably well (modulo having some care with runtime location of shared libs), and those are also very popular configurations. On the contrary, selecting some dependencies in the dependency graph as shared=True while leaving the default of shared=False (=static) for other libraries can be more challenging like for example having way larger total binary size because of multiple linkage of the same static libraries into different shared libraries. And not only the size, but this can also bring other issues like duplicated global objects intended to be unique.

We had that model in ConanCenter for Conan 1, and it was truly problematic. We have been building ConanCenter with Conan 2 with *:shared=True and *:shared=False for 2 years, and this has proven to be a much better approach.

conan create myexperimentalapp -o "mylib/*:shared=True"

But that is a different problem than a conflict. A conflict only happens when there are 2 different branches of a diamond structure in the graph with different options. This definition from the command line or profile (it is the same, totally equivalent) is known in Conan as the "the consumer should always have priority" design criteria. That means that if a user wants to be able to define some option for the dependencies, it will always have priority and define the value without being a conflict.

I think the feature you might be looking for is the "important option values". Please check the https://docs.conan.io/2/reference/conanfile/attributes.html#default-options section, and the paragraphs about option! important options. It allows the fine grain control that you want, respecting by default the recipe change while allowing to force it from the command line with the ! qualifier.

I feel like the same principle will apply to Conan recipes and the Conan client. Warn-as-error is nice to have while writing recipes, but in the end everyone will be disabling warn-as-error in their other daily tasks, which might lead to this kind of warning being overlooked.

But that applies exactly the same to any other config that we create. If we define a -cc core.graph:options_conflict=True as a way to raise errors for this condition, then developers are perfectly capable to define -cc core.graph:options_conflict=False and disable it. So it would be exactly the same situation as -cc core:warnings_as_errors approach. Adding a specific new conf doesn't seem to have any advantage here. And changing the current default and raising errors by default it is not possible, it would be breaking behavior (and likely to be quite massive breaking behavior). So I think the best possible way to do it is a core:warning_as_errors=["options_conflict"] that you can set in your profiles.

stevn commented 3 weeks ago

Hi @memsharded! Thanks for the quick feedback, this is valuable information you are providing here.

Build all shared

Having all the dependencies as static or having all dependencies as shared libraries work in general reasonably well.

Sounds interesting, I will try this out.

And not only the size, but this can also bring other issues like duplicated global objects intended to be unique.

Yes this is something to look out for. I don't use that very often, but it is necessary sometimes. It only works reliably if you know which interfaces and symbols exist where in your complete dependency tree.

Experimental API

But that is a different problem than a conflict. A conflict only happens when there are 2 different branches of a diamond structure in the graph with different options.

Yes, you are right. I had forgotten that I need to construct a diamond structure to trigger the problem.

Anyway, this "experimental API" example just shows that overriding options, including the shared option has different side effects.

I would expect an error to occur if:

Consumer is always right

This definition from the command line or profile (it is the same, totally equivalent) is known in Conan as the "the consumer should always have priority" design criteria. That means that if a user wants to be able to define some option for the dependencies, it will always have priority and define the value without being a conflict.

Interesting, good to know. That's a different concept than I had in mind.

It seems that this can be dangerous, because in my experience in many cases the user isn't fully aware of what is happening. Most of the time the person writing the recipe for a package knows more about the package than the user of the package.

So catching errors early (in the recipe) and providing a good error message when the user tries to do something wrong seems important. It seems to me that the way to achieve this in general is to use the validate() method. I guess I'll take a closer look at that soon.

From my perspective explicitly set (maybe marked as important?) package options aren't something that anyone should be able to override. There should be an easy way for the writer of a recipe to force an option and to error out if that is somehow different.

You mention the "important options" feature. This might be what I am looking for, but I haven't fully understood that yet and will look into this approach next.

Option states

After thinking about the options handling for a bit, it seems to me that there should be multiple states of an option in a recipe:

And then that state could be overridden:

I guess "higher authority" here would mean the user, which probably could be a depender package or a Conan profile / command line. But I am not sure if these are considered equivalent in this context?

Important options

I think the feature you might be looking for is the "important option values".

I saw that but didn't know how to set this in the configure() method. Is that supported?

def configure(self):
    # How to make this option "important"?
    self.options["opencv"].with_tiff = bool(self.options.imgio)

The important! syntax is still slightly confusing to me. But I will try this out soon.

Will two conflicting important! options lead to an error? If not, it doesn't solve my problem.

Warn-as-error vs new core config

But that applies exactly the same to any other config that we create. If we define a -cc core.graph:options_conflict=True as a way to raise errors for this condition, then developers are perfectly capable to define -cc core.graph:options_conflict=False and disable it. So it would be exactly the same situation as -cc core:warnings_as_errors approach. Adding a specific new conf doesn't seem to have any advantage here.

You are right, this is basically equivalent. I am afraid to enable warn-as-error for all warnings, which would likely lead to lots of errors quickly, which in turn would lead to people disabling warn-as-error again. But enabling warn-as-error only for this specific options-conflict warning would probably work OK.

Conclusion

I think setting core:warning_as_errors=["options_conflict"] should be fine. Thanks for clarifying this!

I am also interested in finding out more about important! options syntax and semantics to see if this will solve the problem. It seems that my problem could be solved if Conan would raise an error if two important! options conflict and if I had a way of marking an option as important! in the configure() method of the recipe.

memsharded commented 1 week ago

It seems that this can be dangerous, because in my experience in many cases the user isn't fully aware of what is happening. Most of the time the person writing the recipe for a package knows more about the package than the user of the package.

Not really. Our experience is exactly the opposite. Consumers that tried to change some option in Conan 1 and Conan were ignoring them were really upset and frustrated, complaining and blaming Conan for not allowing them to configure some option (aimed to be a configuration point). Imagine saying somedep:shared=True and Conan ignoring it or producing error because some recipe wanted to use somedep as static and defined somedep:shared=False.

So there is no good solution that satisfies all use cases, specially when different users want completely opposite and incompatible behaviors.

The solution of "consumer is always right" and has priority has been working quite well. It assumes part of the Python Zen of given full control to users (not really private members). Yes, downstream consumers might not be always right, but they should still have full power to do what they want.

Will two conflicting important! options lead to an error? If not, it doesn't solve my problem.

They will give the warning too.

I have done https://github.com/conan-io/conan/pull/17366 that will close this ticket, that add a warning for options conflicts, and categorized it as "risk", so it can use the warn-as-error to raise an error when these options conflicts happen.

Thanks for the feedback!