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

Auto-update when running brew install/upgrade. #50

Merged
merged 1 commit into from
Apr 11, 2016
Merged

Auto-update when running brew install/upgrade. #50

merged 1 commit into from
Apr 11, 2016

Conversation

MikeMcQuaid
Copy link
Member

  • 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 successfully ran brew tests with your changes locally?

Follows on from Homebrew/legacy-homebrew#50192.

The main changes from there:

  • I've fixed the issue if brew.sh was updated while running
  • I've fixed the brew edit workflow for when HOMEBREW_DEVELOPER is set
  • Only enable this behaviour if HOMEBREW_AUTO_UPDATE is set (so I can test it locally and see how annoying it is to e.g. run on every command).

CC @UniqMartin @xu-cheng @bfontaine @apjanke on what would be required for this to be merged as-is (disabled by default).

@ilovezfs
Copy link
Contributor

ilovezfs commented Apr 8, 2016

brew update currently gives a summary of what happened. Are we OK with that info now getting spit out at the top of random installations, and quite possibly off screen almost immediately? Should we just suppress it entirely now?

@MikeMcQuaid
Copy link
Member Author

Are we OK with that info now getting spit out at the top of random installations, and quite possibly off screen almost immediately? Should we just suppress it entirely now?

Yes, I'm OK with that. Something I forgot to do is to add an additional header letting people know that we've autoupdated if there are any changes.

@ilovezfs
Copy link
Contributor

ilovezfs commented Apr 9, 2016

Would you save the update summary and dump it at the end of the install or just leave it at the top?

@MikeMcQuaid
Copy link
Member Author

Leave it at the top.

# shellcheck source=/dev/null
source "$HOMEBREW_LIBRARY/Homebrew/cmd/update.sh"
HOMEBREW_PRECOMMAND="homebrew-update --preinstall"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be a bit cleaner to package this as a function like so:

update-preinstall() {
  [[ -n "$HOMEBREW_AUTO_UPDATE" ]] || return

  if [[ "$HOMEBREW_COMMAND" = install || "$HOMEBREW_COMMAND" = upgrade ]]
  then
    # Hide shellcheck complaint:
    # shellcheck source=/dev/null
    source "$HOMEBREW_LIBRARY/Homebrew/cmd/update.sh"
    homebrew-update --preinstall
  fi
}

Then use that further down below, e.g.:

{ update-preinstall; "homebrew-$HOMEBREW_COMMAND" "$@"; exit $?; }

To me that not only looks cleaner, but also clearer. It also avoids an unquoted variable expansion.

Copy link
Member Author

Choose a reason for hiding this comment

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

@UniqMartin Sounds good. Does that still avoid the "file updated under it" issue, yeh?

Copy link
Contributor

Choose a reason for hiding this comment

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

Does that still avoid the "file updated under it" issue, yeh?

Yes, it does. The function has already been completely parsed by the time it is executed.

@MikeMcQuaid
Copy link
Member Author

Addressed comments.

@MikeMcQuaid
Copy link
Member Author

Will merge this tomorrow if there are no objections.

@chdiza
Copy link
Contributor

chdiza commented Apr 10, 2016

Just for my own clarity, what is the knob that prevents an auto-update from happening? (In the legacy thread from which this arose, there seemed to be general agreement that such a knob should exist.)

@UniqMartin
Copy link
Contributor

@chdiza The current state of the code is that this is an opt-in feature. Once this is merged, you would have to set HOMEBREW_AUTO_UPDATE=1 in your environment to enable the auto update.

@chdiza
Copy link
Contributor

chdiza commented Apr 10, 2016

@UniqMartin Thanks. I had scanned the code looking for the knob, and my eyes must have skipped right over that shellvar.

@MikeMcQuaid
Copy link
Member Author

I'll add HOMEBREW_NO_AUTO_UPDATE in here just so it's one less thing to remove when we ship this.

Also, slightly tweak the behavior of `brew update` in this case so that
it doesn't print annoying output and still allows the `brew edit` flow
for people with `HOMEBREW_DEVELOPER` set.
@MikeMcQuaid
Copy link
Member Author

Added the HOMEBREW_NO_AUTO_UPDATE, 🚢ing.

@MikeMcQuaid MikeMcQuaid merged commit 4a7cd16 into Homebrew:master Apr 11, 2016
@MikeMcQuaid MikeMcQuaid deleted the auto-update branch April 11, 2016 08:31
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants