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

vendor ruby #404

Merged
merged 4 commits into from Jul 11, 2016

Conversation

Projects
None yet
5 participants
@xu-cheng
Copy link
Contributor

xu-cheng commented Jun 27, 2016

  • Add install-vendor command to install vendor ruby/git
  • The vendor Ruby/Git will be put inside
    Library/Homebrew/vendor/{ruby,git}/<version>, with a symlink
    Library/Homebrew/vendor/{ruby,git}/opt pointed to it.
    In addition, a Library/Homebrew/vendor/{ruby-version,git-version} will
    track the latest version of vendor binaries.
  • It is important to notice that for most of users they will continue to use system ruby/git. vendor version is only used for old systems.
  • add --homebrew=fail-on-old-vendor-version flag for ENV/scm/git to help auto detect outdated vendor version.
  • set minimal git version to 1.8, which supports -c flag during git clone.
  • current vendored ruby/git is for test purpose only.

To see vendor git, master...xu-cheng:vendor-bak

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 27, 2016

Could we make this PR just for Ruby for now to slim the diff and add documentation for how to build and upload the Ruby as part of this PR? Thanks!

@tdsmith tdsmith referenced this pull request Jun 27, 2016

Closed

--insecure arg added back #403

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jun 28, 2016

I make the PR only for ruby now.

add documentation for how to build and upload the Ruby as part of this PR?

I have move the portable formulae repo under Homebrew's organization: https://github.com/Homebrew/homebrew-portable
I think document should go to that repo.

There are already some basic document here. However, I may not find enough free time lately to work on automatic building/uploading tool. If any maintainers are interesting to work on that, feel free to do so.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jun 28, 2016

cc @mistydemeo @sjackman for suggesting on automatic building PowerPC and Linux vendor tools.

then
curl_args="-#fLA"
else
curl_args="-sfLA"

This comment has been minimized.

@mistydemeo

mistydemeo Jun 28, 2016

Contributor

In a followup PR, I'd like to look into vendoring curl as well; older platforms won't be able to fetch from all hosts without.

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

This is on my watch list. However, I haven't figured out how to download vendor curl itself. Maybe a separate fetching strategy using system curl with --insecure flag?

This comment has been minimized.

@mistydemeo

mistydemeo Jun 28, 2016

Contributor

It's a bit of a chicken and egg problem, huh? Yes, maybe...

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Assuming they got Homebrew from some trusted source, maybe the way to address this would be to ignore transport security and only rely on the SHA-256 to validate the integrity of the download? (I guess this pretty much boils down to system curl with the --insecure flag.)

temporary_path="${CACHED_LOCATION}.incomplete"

mkdir -p "$HOMEBREW_CACHE"
[[ -z "$HOMEBREW_QUIET" ]] && echo "==> Downloading $VENDOR_URL"

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

A minor detail, but could be relevant in a context where the exit code of such a line is used. If the condition isn't met, this returns exit code 1 indicating failure. If you invert the condition and use || instead of &&, then the exit code will always indicate success. In this particular example:

[[ -n "$HOMEBREW_QUIET" ]] || echo "==> Downloading $VENDOR_URL"

mkdir -p "$VENDOR_DIR/$VENDOR_NAME"
chdir "$VENDOR_DIR/$VENDOR_NAME"
[[ -z "$HOMEBREW_QUIET" ]] && echo "==> Install $(basename "$VENDOR_URL")"

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

“Install” 👉 “Installing”.

ls -t | grep -Ev "^(opt|$VENDOR_VERSION)" | tail -n +4 | xargs rm -rf
else
rm -rf "$VENDOR_VERSION"
[[ -z "$HOMEBREW_QUIET" ]] && onoe "Install vendor $VENDOR_NAME failed."

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

“Install vendor” 👉 “Installing vendored”.

if "./$VENDOR_VERSION/bin/$VENDOR_NAME" --version >/dev/null 2>&1
then
ln -sfn "$VENDOR_VERSION" opt
ls -t | grep -Ev "^(opt|$VENDOR_VERSION)" | tail -n +4 | xargs rm -rf

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Can you add a comment that explains what this does? I think it is non-obvious. Better yet, is there a way to achieve a similar effect using find or find with xargs? I have to admit that parsing the output of ls and then grepping with an externally supplied unescaped string bothers me a little bit.

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

Mike askes me to keep several old copies in case of installation failure. This command comes from here. I don't know whether there is a better implementation. Will add a comment.

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

I agree that keeping around a few (one?) older versions in case of failure is a good thing.

-*)
[[ "$option" = *v* ]] && HOMEBREW_VERBOSE=1;
[[ "$option" = *q* ]] && HOMEBREW_QUIET=1;
[[ "$option" = *d* ]] && HOMEBREW_DEBUG=1;

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

The trailing semicolons in the above three lines are unnecessary.

@@ -1 +0,0 @@
2.0.0-p648

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

I might not understand the full context, but it feels wrong that this is added in a previous commit and then removed in this commit. Is that intentional?

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

oops, squash mistake.

@@ -0,0 +1,46 @@
setup-ruby-path() {
local HOMEBREW_VENDOR_RUBY_OPT;
local HOMEBREW_VENDOR_RUBY_PATH;

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

The semicolons are unnecessary and we mostly try to use lowercase names for local variables to distinguish them from global variables (all caps) and exported variables (all caps and with a HOMEBREW_ prefix).

HOMEBREW_RUBY_PATH="$(which ruby)"
fi

if [[ -z "$HOMEBREW_RUBY_PATH" || "$("$HOMEBREW_RUBY_PATH" -e "puts RUBY_VERSION.split('.').first")" != "2" ]]

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Does that mean that every time a Homebrew command is run (even if the system Ruby is sufficiently up-to-date) we have to call ruby once to learn its version number and then a second time to execute the command? (Unless of course the command is implemented in shell code.) If so, this sounds like it could be a major performance regression, particularly for commands that tend to finish fairly quickly. Can you comment on this?

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Another unrelated note: Should the comparison maybe use -lt 2 instead of != "2"?

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

Both checking system ruby version and comparing ruby version with 2(i.e. not allowing 3) come from Mike's requests. See #195 (comment) and #195 (comment)

I do think we should skip checking system ruby for better performance as for now any brew commands will invoke a dozen system calls before do any job.

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Could we use ruby --version and parse the output with some Bash built-ins instead of making it execute actual Ruby code to get the major version? ruby --version is significantly faster on my machine than ruby -e "<some-trivial-code>". (Still, finding a reliable way to completely omit this check on every run would be great.)

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

Would you mind give me a more detail example of using bash comparing version?

finding a reliable way to completely omit this check on every run would be great.

My previous attempt is to check whether system ruby 1.8 directory exists.

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

Or we could check system version, which it's already done by previous bash script.

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Would you mind give me a more detail example of using bash comparing version?

How about the following (obviously needs to be adapted a bit)?

ruby_major="$(ruby --version)"
ruby_major="${ruby_major#ruby }"
ruby_major="${ruby_major%%.*}"

(This works for me locally with Ruby 1.8.7, 1.9.3, and 2.0-2.3.)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 28, 2016

Member

If we're talking about performance problems: please let's measure the actual performance differences when they are trivial to measure. Thanks!

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

My very simplistic experiment yielded about 10 ms for ruby --version and about 60 ms for the ruby -e variant. Probably not much if seen individually, but it adds up in the grand scheme of things and further contributes to the increasing number of commands we run in our shell setup code before we start executing any of Homebrew's commands.

I didn't want to suggest premature optimization, but my gut feeling was that the impact would be noticeable, particularly because it's affecting every single Homebrew installation, even the 90%+ users that have a perfectly fine system Ruby as they are on a sufficiently modern OS X macOS.

@@ -0,0 +1,46 @@
setup-ruby-path() {
local vendor_ruby_opt_path;
local vendor_ruby_bin_path;

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

The unneeded semicolons are still here. 😉

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

oops, thanks for catching up.

-*)
[[ "$option" = *v* ]] && HOMEBREW_VERBOSE=1
[[ "$option" = *q* ]] && HOMEBREW_QUIET=1
[[ "$option" = *d* ]] && HOMEBREW_DEBUG=1

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

More places that might benefit from using [[ <inverse-condition> ]] || instead of [[ <condition> ]] &&.

This comment has been minimized.

@xu-cheng

xu-cheng Jun 28, 2016

Contributor

I don't think these places should. Noted these are the same logic in update.sh

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

Well, the argument is the same like in all other instances, but I agree that it doesn't make a difference under normal circumstances. (update.sh probably needs some cleanup though, if anyone finds some time for that.)

[[ "$option" = *d* ]] && HOMEBREW_DEBUG=1
;;
*)
[[ -n "$VENDOR_NAME" ]] && odie "This command does not take multiple vendor targets"

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

One more place that might benefit from using [[ <inverse-condition> ]] || instead of [[ <condition> ]] &&.

done

[[ -z "$VENDOR_NAME" ]] && odie "This command requires one vendor target."
[[ -n "$HOMEBREW_DEBUG" ]] && set -x

This comment has been minimized.

@UniqMartin

UniqMartin Jun 28, 2016

Contributor

One more place that might benefit from using [[ <inverse-condition> ]] || instead of [[ <condition> ]] &&.

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Jun 28, 2016

Add install-vendor command to install vendor ruby/git

Can we rename this to vendor-install or something similar? The problem here is that this has a common prefix with the very popular install command and will annoy people that are used to the install command being tab-completed (including the trailing space). We can of course also address this by excluding install-vendor from tab completion, but this adds unnecessary complication.

The vendor Ruby/Git will be put inside Library/Homebrew/vendor/{ruby,git}/<version>, with a symlink Library/Homebrew/vendor/{ruby,git}/opt pointed to it.

I think re-using opt here (I assume inspired from HOMEBREW_PREFIX/opt) feels a bit confusing to me, simply because the directory structure is a different one (<package>/opt vs. opt/<package>). I believe active, current, or latest (in this order of preference) would be a better name here. opt isn't self-explanatory if it appears in a directory listing next to a bunch of version numbers, but e.g. active is.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 28, 2016

I make the PR only for ruby now.

Thanks. I still see e.g. git_URL in here, though?

There are already some basic document here. However, I may not find enough free time lately to work on automatic building/uploading tool. If any maintainers are interesting to work on that, feel free to do so.

Would be good to add how/where to upload to that document, even if it's not automatic or terminal commands but links to e.g. Bintray. Also, rather than documenting multiple commands just making a simple script that runs them and could be part of that tap feels like it'll be easier e.g. brew portable-install ruby that just runs those commands listed in the Ruby and checks the exit codes.

Can we rename this to vendor-install or something similar? The problem here is that this has a common prefix with the very popular install command and will annoy people that are used to the install command being tab-completed (including the trailing space).

👍

I think re-using opt here (I assume inspired from HOMEBREW_PREFIX/opt) feels a bit confusing to me, simply because the directory structure is a different one (/opt vs. opt/). I believe active, current, or latest (in this order of preference) would be a better name here. opt isn't self-explanatory if it appears in a directory listing next to a bunch of version numbers, but e.g. active is.

👍 to current but 👍 to this point in general.


if [[ -n "$HOMEBREW_VERBOSE" ]]
then
curl_args="-fLA"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 28, 2016

Member

Could turn the shared args into a shared variable. Also, can you use the long form of these flags to make it more readable.

@UniqMartin UniqMartin referenced this pull request Jun 28, 2016

Closed

keg_relocate: Fix hardlink behavior for ruby-macho #400

4 of 5 tasks complete
@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jun 29, 2016

I still see e.g. git_URL in here, though?

Can we keep it as is for now, which can help me to reduce conflicting in git rebase. Please note I want to ship vendoring Ruby and git at the same time, or at least in short time between two. As I have stated in the old PR, we need to make sure vendoring system work for both Ruby and Git(as they are invoked in different ways). It will create a lot of implemention as well as maintenance problems if we fail to make sure them in the first time.

As for auto-building, I am considering to ultilize test-bot in #410. But there are several things need to be done. One simple script for all won't work, because there are a few manually tests invoked. I will need to tweak test and linkage command to make auto test possible.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 29, 2016

Can we keep it as is for now, which can help me to reduce conflicting in git rebase. Please note I want to ship vendoring Ruby and git at the same time, or at least in short time between two. As I have stated in the old PR, we need to make sure vendoring system work for both Ruby and Git(as they are invoked in different ways). It will create a lot of implemention as well as maintenance problems if we fail to make sure them in the first time.

I'm pretty distrustful of any system that involves us having to get it perfect first time. As a result, I'd like to consider making this opt-in using an environment variable so we can get wider testing with it in master before we roll it out to everyone to build confidence (as we did with the Bash updater which was a similarly dramatic change).

I'm 👎 on shipping Git and Ruby at the same time or having Git references in this PR. We should ship just Ruby first and shipping Git/Curl once we've had vendored Ruby working in production for users for e.g. a few weeks. Making the PR as minimal as possible will make review better and faster and introduce less changes that could break things.

As for auto-building, I am considering to ultilize test-bot in #410. But there are several things need to be done. One simple script for all won't work, because there are a few manually tests invoked. I will need to tweak test and linkage command to make auto test possible.

We don't need autobuild or autotesting but there's definitely room to script some of the instructions you have there and e.g. run commands and tell the developers to manually inspect them. Documentation gets easily outdated, scripts less so.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jun 30, 2016

As a result, I'd like to consider making this opt-in using an environment variable so we can get wider testing with it in master before we roll it out to everyone to build confidence (as we did with the Bash updater which was a similarly dramatic change).

We can do it.

having Git references in this PR

Sorry, but I fail to understand why a simple line git_URL would cause any problem. It's very frustrated for me as you are basically asking me to repeatedly manually fix git conflict for me to test both vendor Ruby and Git locally whenever I refresh this PR. I'd like to choose the road which makes my development easier.

I'm 👎 on shipping Git and Ruby at the same time

OK, maybe I should rephrase my word. I won't ship any PR before I have confidence this vendor system can work for both git and ruby, which means it should at lease pass my local tests. As I have kept repeatedly stated, some part of details in system cannot be easily changed afterwards.

I'm pretty distrustful of any system that involves us having to get it perfect first time.

I am just trying to make it as perfect as possible in the first time. I don't think you are against this right?

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jun 30, 2016

I think I have addressed all comments.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jun 30, 2016

We can do it.

Great 👍

Sorry, but I fail to understand why a simple line git_URL would cause any problem. It's very frustrated for me as you are basically asking me to repeatedly manually fix git conflict for me to test both vendor Ruby and Git locally whenever I refresh this PR. I'd like to choose the road which makes my development easier.

Sorry, I don't think resolving conflicts in your local development environment justifies adding code to a Homebrew pull request. It won't cause problems but it's dead code that implies a Git feature that's not reviewed yet or a hard requirement for 1.0 (like Ruby is). I'd suggest you focus on just building the Ruby feature for now and don't rebase the Git work until this PR is merged.

OK, maybe I should rephrase my word. I won't ship any PR before I have confidence this vendor system can work for both git and ruby, which means it should at lease pass my local tests. As I have kept repeatedly stated, some part of details in system cannot be easily changed afterwards.

Why does this system need to work for Git for us to ship the Ruby PR?

I am just trying to make it as perfect as possible in the first time. I don't think you are against this right?

Nope, but I do think it's important to focus on Ruby for now and Git later.

@xu-cheng

This comment has been minimized.

Copy link
Contributor

xu-cheng commented Jul 1, 2016

Why does this system need to work for Git for us to ship the Ruby PR?

This is where you missed. We do need to make vendor system including update detection works for both Ruby and Git. On the one hand, I don't like the idea that we will have two different systems to vendor them, if we find it fails for Git. On the other hand, it is extremely important that we should make sure users won't stuck on old vendor version. Because otherwise, we are creating a huge security hole here. And more improtantly, things like file structure cannot be esaily changed in the future PR at all.

If any help, I can remove the Git mention only before we are shipping this PR. But before then, I will keep it. I don't want to be shortsighted on robutness and security.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 1, 2016

If any help, I can remove the Git mention only before we are shipping this PR.

Yes, that's fine.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 11, 2016

Is there any ETA on this PR? There's a few Ruby 1.8 things in the wings that mean I'm wondering how long it's going to be until we're Ruby 2 only. Could you consider shipping this code (without the Git stuff) nearly as-is but just not enabling it without an environment variable so we can test it locally? I think this is higher priority than pretty much any other feature/cleanup in Homebrew at the moment. Another option would be creating this on a non-fork branch so multiple people can work on it.

add file directory for vendor Ruby
The vendor Ruby will be put inside `Library/Homebrew/vendor/portable-ruby/<version>`,
with a symlink `Library/Homebrew/vendor/portable-ruby/current` pointed to it.

In addition, a `Library/Homebrew/vendor/portable-ruby-version` will
track the latest version of vendor binaries.

This gives us version control on vendor Ruby and enables us to bump vendor
Ruby whenever needed such as security update.

@MikeMcQuaid MikeMcQuaid changed the title vendor ruby/git vendor ruby Jul 11, 2016

@@ -0,0 +1,195 @@
#: * `vendor-install` [<target>]:
#: Install vendor version of Homebrew dependencies.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 11, 2016

Member

Think we can hide this from the manpage (for now at least).

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 11, 2016

Member

e.g. with #: @hide_from_man_page as the first line.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Jul 11, 2016

@xu-cheng One final comment then: :shipit: and we can improve documentation etc. afterwards.

xu-cheng added some commits May 3, 2016

brew.sh: new HOMEBREW_RUBY_PATH resolution logic
* Use vendor Ruby if it's present
* Install vendor Ruby for system without Ruby 2.x

@xu-cheng xu-cheng merged commit 7508da2 into Homebrew:master Jul 11, 2016

@xu-cheng xu-cheng deleted the xu-cheng:vendor branch Jul 11, 2016

iMichka pushed a commit to iMichka/brew that referenced this pull request Jun 22, 2017

BottleLoader: Fix installing a bottle from an URL (Homebrew#404)
The name of the formula is not extracted correctly
when the URL includes a hyphen.

@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.