brew update: get the latest version of Ruby code before calling Ruby #47380

MikeMcQuaid · 2015-12-25T23:21:41Z

This splits brew update into two commands: brew update and brew report.

brew update handles:

git fetch the latest version of the code (in parallel) and then git merge (or git rebase) the latest version and call brew report

brew report handles:

everything involving Ruby code
reporting the actual status of what changed between the revision changes (if any)

To accomplish this we've added a basic framework to create Homebrew commands in Bash and pushed a bit of shared logic (e.g. setting HOMEBREW_PREFIX) into Bash to be consistent.

I appreciate the tests aren't all there yet and the coverage isn't great; I did this to do a line-by-line port for now so people could validate the approach and then will work on getting decent test coverage.

This provides some speedup by running git fetch in parallel but will also pave the way for further speedup by using ETags to only do the git fetch when it's actually necessary to do so.

xu-cheng · 2015-12-26T05:43:20Z

bin/brew

@@ -13,6 +13,8 @@ fi
 BREW_FILE_DIRECTORY="$(chdir "${0%/*}" && pwd -P)"
 HOMEBREW_BREW_FILE="$BREW_FILE_DIRECTORY/${0##*/}"

+export HOMEBREW_PREFIX=$(cd $(dirname $(dirname $HOMEBREW_BREW_FILE)) && pwd -P)


It should be chdir instead of cd

What’s the difference?

chdir ensures that the output is sent to /dev/null (in case of weird shell aliases/settings)

Thanks. Also note that you can avoid (weird) aliases by prepending a command with \; e.g.:

$ alias cd=echo $ cd foo foo $ \cd foo bash: cd: foo: No such file or directory

xu-cheng · 2015-12-26T05:51:07Z

Some comments:

I would suggest to put report.rb outside of cmd. Since it shouldn't be invoked directly by users.
I suggest to run bash script against spellcheck.
I'm kinda curious on what will happen if two brew update instances are running simultaneously. We could potenital require a lock if it would cause any trouble.

Some edge cases need to be addressed:

Several checks in brew.rb should be implemented in bash:
- Xcode/CLT related check, otherwise git may not work. brew.rb#L25-L49
- HOMEBREW_PREFIX location check, brew.rb#L51-L55
- Current directory existence check, brew.rb#L66
- Sudo check, brew.rb#L100-L109
- Help flag check, brew.rb#L135-L140
Current bash version of ensure git is installed will fail on non-Xcode/CLT system. Because /usr/bin/git stub will always exist.
Interrupt or any failure should restore repos to original states. i.e. the original reset_on_interrupt should be implemented. Another problem would be handling output in this situation.
We should avoid using brew some-command in bash script. Instead, it should be $HOMEBREW_BREW_FILE some-command. The reason is we want to avoid to pick up different brew instance on multiple installation environment.

Good work though, I gotta admit I was wrong last time. 🎄

xu-cheng · 2015-12-26T06:10:07Z

Library/Homebrew/cmd/update.sh

+  for i in $@
+  do
+    case "$i" in
+      --verbose) HOMEBREW_VERBOSE=1 ;;


We also accept -v. This can be a little complicated, because we should support flag like -dv, which means verbose and debug.

In addition, we need to set HOMEBREW_DEBUG, HOMEBREW_DEVELOPER and HOMEBREW_NO_COMPAT based on flags. They will be used in report script. Or we could pass all of flags to report script.

MikeMcQuaid · 2015-12-26T07:45:09Z

I would suggest to put report.rb outside of cmd. Since it shouldn't be invoked directly by users.

Yeh, makes sense. I was wondering about allowing it to take e.g. SHA1/tap parameters so you can view the output for that case (which would be useful for testing).

I suggest to run bash script against spellcheck.

You mean shellcheck or something here? Not quite sure I understand.

I'm kinda curious on what will happen if two brew update instances are running simultaneously. We could potenital require a lock if it would cause any trouble.

Because we've split out fetch it should be fine. That said, it's maybe worth locking anyway.

Several checks in brew.rb should be implemented in bash:

👍

Current bash version of ensure git is installed will fail on non-Xcode/CLT system. Because /usr/bin/git stub will always exist.

Yeh, I have been wondering about that case: don't we want to just encourage the users to install the CLT's git in that case rather than installing our own one?

Interrupt or any failure should restore repos to original states. i.e. the original reset_on_interrupt should be implemented.

Unfortunately I don't think it's possible to ignore interrupts but we can use a trap here I guess to do the cleanup.

We should avoid using brew some-command in bash script. Instead, it should be $HOMEBREW_BREW_FILE some-command. The reason is we want to avoid to pick up different brew instance on multiple installation environment.

👍

Good work though, I gotta admit I was wrong last time.

Thanks for the kind words 😊

xu-cheng · 2015-12-26T07:56:32Z

You mean shellcheck or something here? Not quite sure I understand.

Yeah, I mean shellcheck (the original one is hit by Apple's spell correction)

Yeh, I have been wondering about that case: don't we want to just encourage the users to install the CLT's git in that case rather than installing our own one?

I'd prefer to installing our own one. The thing is that installing CLT and accepting license requires sudo. And it's not friendly to automate script.

bfontaine · 2015-12-26T21:25:39Z

Library/Homebrew/cmd/update.sh

+fi
+
+git_init_if_necessary() {
+  if [ -z "$(ls .git 2>/dev/null)" ]


You could use [ -d .git ] to check if .git exists and is a directory.

MikeMcQuaid · 2015-12-26T21:43:21Z

Several checks in brew.rb should be implemented in bash:
Xcode/CLT related check, otherwise git may not work. brew.rb#L25-L49
HOMEBREW_PREFIX location check, brew.rb#L51-L55
Current directory existence check, brew.rb#L66
Sudo check, brew.rb#L100-L109
Help flag check, brew.rb#L135-L140

I believe I've now addressed everything except these points. @xu-cheng let me know what you think.

xu-cheng · 2015-12-28T12:41:31Z

Library/Homebrew/cmd/update.sh

+
+  if [ -z "$(which_git)" ]
+  then
+    brew install git


This should be $HOMEBREW_BREW_FILE install git

Wondering if we maybe even want to declare a function or something that means that brew install git will use the $HOMEBREW_BREW_FILE as expected.

Although this stayed the same it's now calling a function.

xu-cheng · 2015-12-28T12:55:50Z

I believe I've now addressed everything except these points

I still think these should be addressed. Especially, for sudo checker and Xcode/CLT license check.

MikeMcQuaid · 2015-12-28T12:58:07Z

I still think these should be addressed.

Yeh, sorry, I meant that I will be addressing them but was just running out of time so figured I'd get more review of what I'd changed.

xu-cheng · 2015-12-28T13:08:46Z

Library/Homebrew/cmd/update.sh

+  do
+    case "$i" in
+      update) shift ;;
+      --verbose|-v) HOMEBREW_VERBOSE=1 ;;


Handle -v may not enough. Because we treat -abcd as -a -b -c -d. You probably want:

--*) ;; -*) for x in $(echo "$i" | grep -o .) do case "$x" in v) HOMEBREW_VERBOSE=1 ;; *);; esac done

--verbose|-*v*) does what we want.

(and handles spaces)

How about --v which is an incorrect flag?

Also --another-flag-contains-v?

To get this right, it probably needs to be something like:

case "${i}" in update) shift ;; --verbose) HOMEBREW_VERBOSE=1 ;; --*) : "<reject or ignore>" ;; -*v*) HOMEBREW_VERBOSE=1 ;; *) : "<fail?>" ;; esac

One just has to be careful to put the cases in the right order and a bit more work has to be done if we want to check for other short flags other than -v.

xu-cheng · 2015-12-28T13:11:11Z

Per chat, brew update following with immediate interrupt will cause nasty output.

chdiza · 2015-12-29T05:49:48Z

Library/Homebrew/cmd/update.sh

+    then
+      which_git="$active_developer_dir/usr/bin/git"
+    fi
+    fi


One of these fi is indented too deeply. And it looks as though the indenting of then is inconsistent in this function.

Thanks @chdiza.

MikeMcQuaid · 2016-01-10T20:30:36Z

I've updated this with all the comments made. I'd like to know what tests should be made here before we push this (but I'd like to try and avoid "perfect being the enemy of good" given that this will address a lot of update issues for us and the earlier we ship this the easier it'll be to ship e.g. Ruby 2 support eventually).

UniqMartin · 2016-01-10T20:49:03Z

I agree that we don't have to be perfect here. But we should still be confident that this hasn't any major issues (like breaking updates completely) as those are difficult to fix once this reaches a significant number of users. I'll try to test drive this and provide a timely review, but given the extent of this change and somewhat limited time next week, don't expect that to happen before the weekend …

MikeMcQuaid · 2016-01-10T20:59:13Z

But we should still be confident that this hasn't any major issues (like breaking updates completely) as those are difficult to fix once this reaches a significant number of users. I'll try to test drive this and provide a timely review, but given the extent of this change and somewhat limited time next week, don't expect that to happen before the weekend …

Yep, that's fine. Definitely agree with not rushing through and breaking things. That's one of the reasons why this is (more-or-less) a line-by-line port of the Ruby code into Bash. Obviously the behaviour won't be identical but the flow at least should be.

xu-cheng · 2016-01-11T14:16:35Z

Library/Homebrew/cmd/update.sh

+your own risk.
+EOS
+  exit 1
+fi


I think we could add this check to bin/brew and discard the test in brew.rb.

Yeh, I guess I didn't want to handle the "check if a file is present in this array" but I guess that's not too much pain to do.

MikeMcQuaid · 2016-01-14T14:20:34Z

I've addressed all comments and suggestions and added tests to verify we don't accidentally have invalid Bash syntax anywhere.

This is ready for some testing and then 🚢ping. Another option for such a drastic change is we could bring back the old update.rb file and only run this update.sh if you have HOMEBREW_DEVELOPER set. Another another option is we push each commit here once-at-a-time and wait a bit for feedback.

Thoughts, @Homebrew/maintainers?

xu-cheng · 2016-01-15T20:25:31Z

Library/Homebrew/cmd/update-bash-report.rb

+require "descriptions"
+
+module Homebrew
+  def update_bash_report


For the time being, let's prevent users calling this command directly. i.e. adding homebrew developer check.

Also, we may want to name the command update-report or other name without the temporary bash part. Notice, we cannot change this command's name in the future.

That's true, we should call it update-report or something similar, but not use a temporary name as changing it afterwards won't be easy, if at all possible.

MikeMcQuaid · 2016-01-17T19:54:29Z

(Most) comments addressed, tested and pushed. Thanks for all the review, folks.

MikeMcQuaid added the maintainer feedback label Dec 25, 2015

xu-cheng reviewed Dec 26, 2015
View reviewed changes

bfontaine reviewed Dec 26, 2015
View reviewed changes

xu-cheng reviewed Dec 28, 2015
View reviewed changes

chdiza reviewed Dec 29, 2015
View reviewed changes

MikeMcQuaid mentioned this pull request Jan 9, 2016

Update formulae to use special rbenv dependency #47875

Closed

xu-cheng reviewed Jan 11, 2016
View reviewed changes

xu-cheng reviewed Jan 15, 2016
View reviewed changes

MikeMcQuaid closed this in 144c172 Jan 17, 2016

MikeMcQuaid deleted the update-bash branch January 17, 2016 19:54

Homebrew locked and limited conversation to collaborators Jul 10, 2016

brew update: get the latest version of Ruby code before calling Ruby #47380

brew update: get the latest version of Ruby code before calling Ruby #47380

Conversation

MikeMcQuaid commented Dec 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-cheng commented Dec 26, 2015

Choose a reason for hiding this comment

MikeMcQuaid commented Dec 26, 2015

xu-cheng commented Dec 26, 2015

Choose a reason for hiding this comment

MikeMcQuaid commented Dec 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-cheng commented Dec 28, 2015

MikeMcQuaid commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-cheng commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jan 10, 2016

UniqMartin commented Jan 10, 2016

MikeMcQuaid commented Jan 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jan 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jan 17, 2016