cmd/pin: Update pinned formula messaging #16301

apainintheneck · 2023-12-06T07:24:42Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Resolves #16212

The hope is that this will be clearer and less annoying for users.

A user came to us a couple weeks ago stating that it was confusing that the brew upgrade command printed an error when a pinned formula had a new version available and didn't get upgraded. Not upgrading a package is the expected result of pinning it.

This PR changes that message to a warning from an error. While looking into this we found that there is another message that gets printed when a package dependent doesn't get upgraded because it is pinned and that got turned into a warning from a normal message. Honestly that should be more worrying for the user anyway; it could lead to a program not working correctly in the worst case scenario.

I also added a message to the brew pin command warning about potential unintended behavior if a dependency gets pinned and another package requires a newer version of it.

Lastly, I added a commented out deprecation notice for the brew upgrade --ignore-pinned command since it's now essentially the default.

Library/Homebrew/cmd/upgrade.rb

MikeMcQuaid

Looks good so far, thanks! My questions are:

does this indicates if a specific formula has not been upgraded because of the brew pin of a dependency?
does brew upgrade foo fail/error if foo is pinned?
same as 2) but if one of foo's dependencies are pinned.

apainintheneck · 2023-12-08T07:37:31Z

does this indicates if a specific formula has not been upgraded because of the brew pin of a dependency?

same as 2) but if one of foo's dependencies are pinned.

The answer to 1 and 3 is essentially the same.

If one of foo's dependencies is pinned and the current version is too old, the install will raise an error in the FormulaInstaller#check_install_sanity method (great method name by the way).

brew/Library/Homebrew/formula_installer.rb

Lines 348 to 352 in ab6a7ed

    
           return if pinned_unsatisfied_deps.empty? 
        
           raise CannotInstallFormulaError, 
        
                 "You must `brew unpin #{pinned_unsatisfied_deps * " "}` as installing " \ 
        
                 "#{formula.full_name} requires the latest version of pinned dependencies"

This error gets caught, printed and then we skip trying to install that formula. That seems to be the case when installing or upgrading formulae so it's even more complex than I originally thought.

brew/Library/Homebrew/install.rb

Lines 280 to 282 in ab6a7ed

    
           rescue CannotInstallFormulaError => e 
        
             ofail e.message 
        
             nil

brew/Library/Homebrew/upgrade.rb

Lines 110 to 112 in ab6a7ed

    
           rescue CannotInstallFormulaError => e 
        
             ofail e 
        
             nil

does brew upgrade foo fail/error if foo is pinned?

No, it would print a warning. This used to be an error message (ofail) and is the message that was brought up in the original issue. That means it won't return a non-zero exit code either anymore.

The hope is that this will be clearer and less annoying for users. A user came to us a couple weeks ago stating that it was confusing that the `brew upgrade` command printed an error when a pinned formula had a new version available and didn't get upgraded. This PR changes that message to a warning from an error. While looking into this we found that there is another message that gets printed when a package dependency doesn't get upgraded because it is pinned and that got turned into a warning from a normal message. Honestly, that should be more worrying for the user anyway; it could lead to a program not working correctly in the worst case. I also added a message to the `brew pin` command warning about potential unintended behavior if a dependency gets pinned and another package requires a newer version of it. Lastly, I added a commented out deprecation notice for the `brew upgrade --ignore-pinned` command since it's now the default.

MikeMcQuaid · 2023-12-11T15:38:53Z

Great, 1 and 3 sound right 👍🏻

No, it would print a warning. This used to be an error message (ofail) and is the message that was brought up in the original issue. That means it won't return a non-zero exit code either anymore.

I feel like brew upgrade shouldn't print an error or have non-zero exit code but brew upgrade foo should because you've specifically requested an upgrade of foo, one is available but you've pinned it. Thoughts?

apainintheneck · 2023-12-12T05:34:38Z

I feel like brew upgrade shouldn't print an error or have non-zero exit code but brew upgrade foo should because you've specifically requested an upgrade of foo, one is available but you've pinned it. Thoughts?

That sounds reasonable. In that case, should we still deprecate the --ingore-pinned option? This change probably removes the need for that option for most uses cases but not all.

MikeMcQuaid · 2023-12-12T08:48:35Z

In that case, should we still deprecate the --ignore-pinned option?

I don't feel strongly either way. brew upgrade foo --ignore-pinned when foo is pinned feels like a pretty weird invocation. If there's other cases outside of that: I'm happy to leave it. Could always leave it for now and see if we end up iterating post-merge based on more feedback?

The idea here is that it should be expected that `brew upgrade` will not upgrade pinned packages but will attempt to upgrade everything else. For that reason, it will only show a warning about pinned packages in that case. If, on the other hand, you pass the name of a pinned package explicitly to the upgrade command, like in `brew upgrade PINNED`, we want to show an error since we cannot upgrade that package until it gets unpinned.

Library/Homebrew/cmd/upgrade.rb

apainintheneck · 2023-12-13T08:23:55Z

I don't feel strongly either way. brew upgrade foo --ignore-pinned when foo is pinned feels like a pretty weird invocation.

Me thoughts exactly. I'll leave the deprecation in.

Library/Homebrew/cmd/upgrade.rb

MikeMcQuaid

Thanks for all the work here @apainintheneck and sorry for the amount of back and forth needed!

Moulick · 2023-12-14T10:44:57Z

In that case, should we still deprecate the --ignore-pinned option?

I don't feel strongly either way. brew upgrade foo --ignore-pinned when foo is pinned feels like a pretty weird invocation. If there's other cases outside of that: I'm happy to leave it. Could always leave it for now and see if we end up iterating post-merge based on more feedback?

Does feel weird, the one case that comes to mind is that if foo is pinned and you want to upgrade foo only this time but leave it pinned for the next time, without having to unpin, upgrade, re-pin

apainintheneck · 2023-12-16T18:21:25Z

Does feel weird, the one case that comes to mind is that if foo is pinned and you want to upgrade foo only this time but leave it pinned for the next time, without having to unpin, upgrade, re-pin

@Moulick I don't believe that was possible before this change either. This strictly changes messaging not how brew install behaves when a formula is pinned.

Moulick · 2023-12-19T10:29:21Z

Does feel weird, the one case that comes to mind is that if foo is pinned and you want to upgrade foo only this time but leave it pinned for the next time, without having to unpin, upgrade, re-pin

@Moulick I don't believe that was possible before this change either. This strictly changes messaging not how brew install behaves when a formula is pinned.

Sure, I get that this is just a message change, not arguing that fact at all 😄. From a power users' perspective, looking at the command brew upgrade foo --ignore-pinned, logically I'd think it would upgrade foo ignoring the fact that it is pinned or not, even though it does not do that. Anyways, I don't think we really need it anyways, it's more of a convenience that would help if someone wanted to upgrade without having to unpin, but for now a user can unpin, upgrade, re-pin if they so wanted. (Like I do for a couple of dependencies) ¯\_(ツ)_/¯

apainintheneck requested a review from MikeMcQuaid December 6, 2023 07:24

Rylan12 reviewed Dec 6, 2023

View reviewed changes

Library/Homebrew/cmd/upgrade.rb Outdated Show resolved Hide resolved

Library/Homebrew/cmd/upgrade.rb Outdated Show resolved Hide resolved

MikeMcQuaid reviewed Dec 6, 2023

View reviewed changes

apainintheneck force-pushed the update-pinned-formula-messaging branch from bb36203 to b3a6e59 Compare December 8, 2023 07:43

apainintheneck commented Dec 13, 2023

View reviewed changes

Library/Homebrew/cmd/upgrade.rb Show resolved Hide resolved

MikeMcQuaid reviewed Dec 13, 2023

View reviewed changes

Library/Homebrew/cmd/upgrade.rb Outdated Show resolved Hide resolved

cmd/upgrade: deprecate --ignore-pinned.

d0da6bd

MikeMcQuaid approved these changes Dec 13, 2023

View reviewed changes

MikeMcQuaid enabled auto-merge December 13, 2023 12:04

MikeMcQuaid merged commit cd8a9c0 into Homebrew:master Dec 13, 2023
25 checks passed

github-actions bot added the outdated PR was locked due to age label Jan 19, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/pin: Update pinned formula messaging #16301

cmd/pin: Update pinned formula messaging #16301

apainintheneck commented Dec 6, 2023 •

edited

Loading

MikeMcQuaid left a comment

apainintheneck commented Dec 8, 2023 •

edited

Loading

MikeMcQuaid commented Dec 11, 2023

apainintheneck commented Dec 12, 2023

MikeMcQuaid commented Dec 12, 2023

apainintheneck commented Dec 13, 2023

MikeMcQuaid left a comment

Moulick commented Dec 14, 2023

apainintheneck commented Dec 16, 2023

Moulick commented Dec 19, 2023 •

edited

Loading

cmd/pin: Update pinned formula messaging #16301

cmd/pin: Update pinned formula messaging #16301

Conversation

apainintheneck commented Dec 6, 2023 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

apainintheneck commented Dec 8, 2023 • edited Loading

MikeMcQuaid commented Dec 11, 2023

apainintheneck commented Dec 12, 2023

MikeMcQuaid commented Dec 12, 2023

apainintheneck commented Dec 13, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Moulick commented Dec 14, 2023

apainintheneck commented Dec 16, 2023

Moulick commented Dec 19, 2023 • edited Loading

apainintheneck commented Dec 6, 2023 •

edited

Loading

apainintheneck commented Dec 8, 2023 •

edited

Loading

Moulick commented Dec 19, 2023 •

edited

Loading