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] [2.0] Options are frozen when calling configure #14528

Closed
PengZheng opened this issue Aug 19, 2023 · 7 comments
Closed

[question] [2.0] Options are frozen when calling configure #14528

PengZheng opened this issue Aug 19, 2023 · 7 comments
Assignees

Comments

@PengZheng
Copy link

PengZheng commented Aug 19, 2023

This is expected. The manipulation of options needs to be done in the config_options() method. That was always the purpose of this method, though the capability was leaking into configure() in Conan 1.X. Please try that.

I manipulate option values in configure() for a different purpose

    def configure(self):
        if self.options.build_framework:
            self.options.build_utils = True

framework and utils are two different components in the same conan package.
When user wants framework, utils should be built automatically.
For more context, see this PR.

This automatic intra-package dependency deduction is important when using complex middleware like Celix in large scale.

But if you are going to either set or remove with_stacktrace_backtrace, then, why giving it a default_options value?

Note that all these build_ options default to false so that users don't get what they don't need.

How to support it in Conan 2.0?
Any recommendation specific to the Celix project can be given in this issue:
apache/celix#483

Originally posted by @PengZheng in #10104 (comment)

@memsharded
Copy link
Member

Hi @PengZheng

Thanks for your report.

However, I am not sure what would be an issue. I can't reproduce it. We have a test that validates this is possible in https://github.com/conan-io/conan/blob/6f6481dbf6ae5adfd8ddeee126dc419c2249f8b3/conans/test/integration/options/options_test.py#L154C1-L154C1. Could you please double check, or try to provide a more complete reproducible case that we could reproduce? Are you sure you are at latest 2.0.9?

Regarding

self.requires("celix/2.3.0")
self.options['celix'].build_pushstreams = True

This is not possible, as requires need to happen in the requirements() method, and the definition of options need to be done in configure(). But you can use self.requires("celix/2.3.0", options={"build_pushstreams": True})

@PengZheng
Copy link
Author

PengZheng commented Aug 20, 2023

Could you please double check, or try to provide a more complete reproducible case that we could reproduce? Are you sure you are at latest 2.0.9?

My situation is similar to the following code snippet. Note that with_stacktrace_backtrace now has default set.

                options = {"without_stacktrace": [True, False],
                           "with_stacktrace_backtrace": [True, False]}
                # with_stacktrace_backtrace has default False
                default_options = {"without_stacktrace": True, "with_stacktrace_backtrace": False}
                def configure(self):
                    if self.options.without_stacktrace:
                        del self.options.with_stacktrace_backtrace 
                    else:
                        # Incorrect attempt to modify options 'with_stacktrace_backtrace'
                        self.options.with_stacktrace_backtrace = True

build_pushstreams defaults to False so that it (and its dependencies) won't be built if not explicitly requested.
But another feature with option build_cxx_rsa_integration depends on build_pushstreams so we have something like the following:

        if self.options.build_cxx_rsa_integration:
            self.options.build_cxx_remote_service_admin = True
            # Incorrect attempt to modify options 'build_pushstreams'
            self.options.build_pushstreams = True

Now turning build_cxx_rsa_integration on will lead to "incorrect attempt to modify options" error.

This is not possible, as requires need to happen in the requirements() method, and the definition of options need to be done in configure(). But you can use self.requires("celix/2.3.0", options={"build_pushstreams": True})

Yes, this is indeed an issue.The solution you suggested is concise and clean. Thanks.

@memsharded
Copy link
Member

memsharded commented Aug 20, 2023

build_pushstreams defaults to False so that it (and its dependencies) won't be built if not explicitly requested.

Yes, that is true. If you plan to define the default dynamically in the configure() method, you cannot give a default in default_options, they are incompatible. Please do not define the value in default_options and you can assign that False value in configure() instead when the other conditionals are not met (the key is that the option default value can only be assigned once)

@PengZheng
Copy link
Author

PengZheng commented Aug 20, 2023

I tried the following, but it does not work in Conan 1 (the option is ignored):

    def requirements(self):
        if self.options.build_utils:
            # self.requires("libzip/[>=1.7.3 <2.0.0]")
            # self.options['libzip'].shared = True
            self.requires("libzip/[>=1.7.3 <2.0.0]", options={"shared": True})

Is this expected? I found no such usage in conan-center-index grep -nR "self.requires" recipes/ | grep options.
I cannot migrate to Conan 2 right away, thus the conanfile has to work with 1.x.

I have updated the project to use Conan2's generators:
https://github.com/apache/celix/tree/feature/483-conan-2-support

The remaining issues:

  • Implement automatic intra-package dependency deduction in a way supported by Conan 2
  • Specify dependency option in requirements in a way supported both by Conan 1 and Conan 2

I will update if I made any progress.

@memsharded
Copy link
Member

Is this expected? I found no such usage in conan-center-index

Yes, this is expected, it is a Conan 2.0 feature only, it doesn't work in Conan 1.X, and it is not planned to add it to 1.X.
The Conan 1.X and 2.0 compatible way of doing it is in the configure() method

If the question has been responded and defining values in configure() work as expected (without defining the default_option value), I'd prefer if you close the issue, and create new issues for any new open issue or question. Thanks!

PengZheng added a commit to apache/celix that referenced this issue Aug 21, 2023
Dynamic defaults is unsupported by Conan2 according to conan-io/conan#14528
@PengZheng
Copy link
Author

PengZheng commented Aug 21, 2023

Dynamic defaults have been implemented and dependency options are set in configure(): https://github.com/apache/celix/blob/6d25aa626335d7ccaa4a83520289b0760f8aa8e3/conanfile.py#L217-L220

But I encountered version conflicts caused by indirect dependencies. I'll create a separate issue for this.

@memsharded
Copy link
Member

Great, thanks for the feedback!

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

2 participants