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

How to deprecate a recipe option #5020

Merged

Conversation

uilianries
Copy link
Member

@uilianries uilianries commented Mar 23, 2021

Sometimes we need to change recipes options, one of most intuitive action is removing deprecated options, but it could cause a break change if you are using that option in your project. Instead, on CCI we follow an undocumented protocol. Let's add it on FAQ.

/cc @SpaceIm @ericLemanissier

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries requested review from SSE4, jgsogo and danimtb March 23, 2021 19:05
@conan-center-bot
Copy link
Collaborator

Updating docs!

@Croydon
Copy link
Contributor

Croydon commented Mar 23, 2021

We only had brief discussions about this in some pull requests. Does the majority of core contributors think that this is the best way?

Personally, I think that we break recipes in so many ways so often, that this way of deprecating options might have a good intention, but is ultimately almost pointless. The only way people can prevent the risk of breakages is by using revisions and if you use them, no option can suddenly disappear. If you then try to update revisions Conan will inform you that the option does not exist.

Removing old options from recipes would help to keep them cleaner.

Also, if consumers set options they expect that they actually have an effect. If recipes just output some warning they might be easily overlooked in several hundred or thousand lines of log output. Assuming that options have an effect, getting no error from Conan but then noticing maybe much later that the options had no effect is IMHO the much worse experience.

@conan-center-bot
Copy link
Collaborator

Updating docs!

@prince-chrismc
Copy link
Contributor

we break recipes in so many ways so often [...] the only way people can prevent the risk of breakages is by using revisions

💯

Any consumer who needs consistency is

  • cloning all the recipes and packages
  • using revisions

and if you use them, no option can suddenly disappear

My only mute counter is it makes it that much harder to upgrade. However if deprecating an option is a breaking change then it's no worse.

docs/faqs.md Outdated Show resolved Hide resolved
@SSE4
Copy link
Contributor

SSE4 commented Mar 24, 2021

I am not sure if I like it...

  1. it's so much extra code
  2. we keep an option which has no effect
  3. we only print a warning, and usually attention is not paid for them

also I don't get it. you say:

No. Changing any option will result in a different package ID and may result a different behavior, the result can break users.

but then you suggest:

def package_id(self):
    del self.info.options.foobar

it means if I had -o foobar=True in my profile, package id will change anyway for that configuration, right?

in general I would say, use revisions to avoid breakage. and clarify our policy that CCI doesn't provide backward compatibility for recipe options.

@uilianries
Copy link
Member Author

it means if I had -o foobar=True in my profile, package id will change anyway for that configuration, right?

Yes. Actually, the real problem is when you that option and we remove it from a recipe. When updating it, or consuming a fresh remote copy, Conan command will break because that option is no longer available.

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot
Copy link
Collaborator

Updating docs!

@uilianries
Copy link
Member Author

I see many comments on this PR, which is excellent, more people giving their opinion.

Deprecating options is a suggestion, not a rule, but as we are discussing now, we can provide cases where we should remove or not an option.

docs/faqs.md Outdated

## Can I remove an option from recipe

No. Changing any option will result in a different package ID and may result a different behavior, the result can break users.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove that argument, as adding del self.info.options.foobar causes the same effect as removing option (different package ID), so there is no advantage for that case

docs/faqs.md Outdated Show resolved Hide resolved
@conan-center-bot
Copy link
Collaborator

Updating docs!

prince-chrismc
prince-chrismc previously approved these changes Mar 24, 2021
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to merge our current policy but I'd love to see reform in the future

@ericLemanissier
Copy link
Contributor

ericLemanissier commented Mar 25, 2021

disabling the effect of an option instead of removing the option from the recipe, is choosing a silent breakage (I compiled my package successfully, but it does not have the feature I asked for), over a loud breakage (conan signals from the start that there is a problem). This does not seem to be the better choice to me either.

@SSE4 SSE4 mentioned this pull request Mar 25, 2021
4 tasks
@uilianries
Copy link
Member Author

Thank you all for your comments! After discussing with Conan Team about this topic, we see removing options is better, because we are faking when add "deprecated", and no all people read that warning message. I'll update this PR, following the opposite side.

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 25, 2021

Don't you remember what happened with libcurl or jasper?

#2883
#3206

@uilianries
Copy link
Member Author

Yes, I do.

Deprecating options can be a remedy for cases like libcurl, but also we are lying for users. We patched an option which is no longer. The recipe is turning a patchwork.

Conan Center Index can be improved, rebuilding what is needed to avoid missing package IDs after removing options.

Lockfiles are the key for behavior and reproducibility. So even if we update a package, a Lockfile will keep the same package.

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 25, 2021

Deprecating options can be a remedy for cases like libcurl, but also we are lying for users. We patched an option which is no longer.

We don't lie, the old options are working as expected.

Lockfiles are the key for behavior and reproducibility. So even if we update a package, a Lockfile will keep the same package.

I agree, but how many consumers are using lockfiles? Theory vs reality?

@uilianries
Copy link
Member Author

We don't lie, the old options are working as expected.

Options are working for old revisions only. After deprecating we can pass same old values, but there is no effect for recipe.

I agree, but how many consumers are using lockfiles? Theory vs reality?

Companies are strongly recommended to use Lockfiles. Regular users can update their package revisions, as soon as Conan Center regenerates all packages affected by an option change.

@SpaceIm
Copy link
Contributor

SpaceIm commented Mar 25, 2021

We don't lie, the old options are working as expected.

Options are working for old revisions only. After deprecating we can pass same old values, but there is no effect for recipe.

I tried to carefully map the deprecated options values to the corresponding new option values:

        # Deprecated options
        # ===============================
        if (any(deprecated_option != "deprecated" for deprecated_option in [self.options.with_openssl, self.options.with_wolfssl, self.options.with_winssl, self.options.darwin_ssl])):
            self.output.warn("with_openssl, with_winssl, darwin_ssl and with_wolfssl options are deprecated. Use with_ssl option instead.")
            if tools.is_apple_os(self.settings.os) and self.options.with_ssl == "darwinssl":
                if self.options.darwin_ssl == True:
                    self.options.with_ssl = "darwinssl"
                elif self.options.with_openssl == True:
                    self.options.with_ssl = "openssl"
                elif self.options.with_wolfssl == True:
                    self.options.with_ssl = "wolfssl"
                else:
                    self.options.with_ssl = False
            if not tools.is_apple_os(self.settings.os) and self.options.with_ssl == "openssl":
                if self.settings.os == "Windows" and self.options.with_winssl == True:
                    self.options.with_ssl = "schannel"
                elif self.options.with_openssl == True:
                    self.options.with_ssl = "openssl"
                elif self.options.with_wolfssl == True:
                    self.options.with_ssl = "wolfssl"
                else:
                    self.options.with_ssl = False
        # ===============================

@prince-chrismc
Copy link
Contributor

I think libcurl is likely an exception, we changed from many settings to one... but the "setting" still exists. For that use case I think "deprecated" was the right call.

I think it much more common we have "broken" or settings that do not work, removed from upstream, etc.
For example I recently did this...

if self.options.multiple_headers != "deprecated":
self.output.warn("The {} option 'multiple_headers' has been deprecated. Both formats are in the same package.")

but the setting never worked, no one was using it nor was it's logic maintained.... it could have been removed.

@jgsogo
Copy link
Contributor

jgsogo commented Mar 26, 2021

I think we should prefer the loud breakage over the silent one:

disabling the effect of an option instead of removing the option from the recipe, is choosing a silent breakage (I compiled my package successfully, but it does not have the feature I asked for), over a loud breakage (conan signals from the start that there is a problem). This does not seem to be the better choice to me either.

It can be very misleading if you are setting an option in your profile or command line and it has no effect... it can lead to some hours of wtf debugging and investigation 😅

It is also true that a deprecation like the one did by @SpaceIm would be the better approach, but sometimes it adds lot's of complexity to a recipe that we will want to remove at some point in time. Maybe the policy should follow these lines:

  • Prefer the depreation path with a mapping from old options to new ones.
  • If logic is too complex (this is subjective and depends on the PR author) then just remove the option.
  • After some time (comment with a date?), we will welcome a PR removing the option that was deprecated.

I'm not a friend of deferring the inevitable, but I'll be ok with the two steps deprecation as well.

@prince-chrismc
Copy link
Contributor

The only thing I would suggest changing is

-this is subjective and depends on the PR author
+this is subjective and depends on the Conan team

a4z
a4z previously approved these changes Mar 27, 2021
SSE4
SSE4 previously approved these changes Mar 30, 2021
Signed-off-by: Uilian Ries <uilianries@gmail.com>
…n-center-index into docs/options-deprecation
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries dismissed stale reviews from SSE4, a4z, and prince-chrismc via 372e848 March 30, 2021 18:48
@conan-center-bot
Copy link
Collaborator

Updating docs!

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@conan-center-bot
Copy link
Collaborator

Updating docs!

@uilianries
Copy link
Member Author

Thank you all for your comments! I made a small update, I've included Javier's lines, now deprecation is not mandatory, but a strong recommendation. An option can be removed after 1 month of deprecation. Of course, we need to check possible gaps (downstream breakage) before merging anything.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor editorial on the double negation

docs/faqs.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
@conan-center-bot
Copy link
Collaborator

Updating docs!

@conan-center-bot conan-center-bot merged commit 94e3073 into conan-io:master Apr 5, 2021
@prince-chrismc prince-chrismc mentioned this pull request Apr 28, 2021
4 tasks
AlvaroFS pushed a commit to AlvaroFS/conan-center-index that referenced this pull request May 7, 2021
* How to deprecate an option

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add examples for option deprecation

* Update docs/faqs.md

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

* Update docs/faqs.md

* Options can be removed, but not recommended

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Improve remove option faq

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Grammar fix

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Update docs/faqs.md

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>

Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants