language/node: adjustments for npm 5.0.x #2710
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CC @MikeMcQuaid @ilovezfs @chrmoritz @JCount @dunn and possibly @zkat if you want to keep an eye on this side of the discussion as well. |
4 tasks
|
So were you able to determine whether @chrmoritz is correct that this can cause e.g. .brew_home to end up getting installed? |
It's not there for me. Perhaps because alexjs tree |
|
@chrmoritz was saying it's dependent on whether the files to install happen to be specifically whitelisted or not: Homebrew/homebrew-core#14085 (comment) |
|
So far it hasn't appeared in any of: |
|
@DomT4 yeah, I'm pretty skeptical that, even if it's true in some cases, that it would actually be a regression as opposed to preexisting behavior since upstream has said in more than one place that pack+install is the same as the old install, but obviously @chrmoritz knows more about the topic than I do by a mile. |
|
I'm down to |
|
Do we know if this new behavior (i.e. running pack first) is backwards compatible with the prior versions of npm < 5? |
ilovezfs
reviewed
Jun 1, 2017
Library/Homebrew/language/node.rb
Outdated
| --verbose | ||
| --global | ||
| --prefix=#{libexec} | ||
| #{Dir.pwd}/#{Utils.popen_read("npm pack").chomp} |
There was a problem hiding this comment.
It used to be possible to use std_npm_install_args with an arbitrary package name by doing something like
args = std_npm_install_args(libexec/"vendor") << "foo@2"
system "npm", "install", *args
but now it looks like that won't be possible.
Admittedly, I don't think that behavior is actually used in core currently, but I'm wondering if we should allow an optional second argument with arbitrary package name(s), or a splat argument for any additional parameters, or something else along those lines, so that the node language module doesn't lose the flexibility it currently has. (i.e., if second arg is nil, do the pack magic, otherwise pass the parameters along and don't do pack magic.)
There was a problem hiding this comment.
Are you sure this was possible? Would std_npm_install_args(libexec/"vendor") << "foo@2" really overide the "."paramater which tells npm to install the local folder (or at least told before npm@5)?
I think it would just tell npm to install BOTH the local folder = "." AND foo@2and if couldn't find a valid node module in the current working directory it would error out early.
I don't think that such a hacky behavior (installing the module at the current local folder and an other named module) is something we want to support here. Also it looks to contradict against the homebrew philosopy that foo should be vendored as a resource too in such a use case.
There was a problem hiding this comment.
The package name actually went first in the example I was recalling, where this did work:
system "npm", "install", "yarn", *Language::Node.std_npm_install_args(buildpath/"vendor/yarn")
system buildpath/"vendor/yarn/bin/yarn", "install"
Also it looks to contradict against the homebrew philosopy that foo should be vendored as a resource too in such a use case.
We have an exception for things installed via other trusted package managers (e.g. venv.pip_install, cabal_install, etc.)
There was a problem hiding this comment.
Yeah this will work, but it will install BOTH yarn and the module at the current working directory to buildpath/"vendor/yarn"which is not what you would expect from it (the installing the module at the cwd too part) and it will only work if the cwd is a valid node module.
Just do a npm install grunt-cli -g --verbose . from a cwd which is not a node module to confirm this.
It's more like a very dirty hack using the API for something it was not meant to be used to. Supporting such a use case by adding a third global_npm_install_args (which does the right thing without installing the cwd as a module too) should be a much cleaner way to do so.
There was a problem hiding this comment.
Supporting such a use case by adding a third global_npm_install_args (which does the right thing without installing the cwd as a module too) should be a much cleaner way to do so.
Yes, that would probably be a nice thing to have at some point.
There was a problem hiding this comment.
This should have some sort of error handling for the case where npm pack fails. Run Utils.popen_read("npm pack") before interpolation and check the exit code with $?.
|
I said
It should be beackwarts compatible to at least npm@3, because it's basicaly the exact same thing our npm install folder call has done before unter the hood. (see this comment) |
When would that occur and how does moving it make any difference? |
When a module author would decide to not use this optional field and rely on the default package everything except stuff in
Because now |
Makes sense now. |
MikeMcQuaid
requested changes
Jun 1, 2017
Library/Homebrew/language/node.rb
Outdated
| --verbose | ||
| --global | ||
| --prefix=#{libexec} | ||
| #{Dir.pwd}/#{Utils.popen_read("npm pack").chomp} |
There was a problem hiding this comment.
This should have some sort of error handling for the case where npm pack fails. Run Utils.popen_read("npm pack") before interpolation and check the exit code with $?.
|
I can't comment on Mike's request for changes directly, so I'll note here that yes, I'll look at that. |
|
Thanks, @DomT4! |
|
Changes pushed here. Let me know how much y'all hate it 😸 |
ilovezfs
reviewed
Jun 1, 2017
Library/Homebrew/language/node.rb
Outdated
| Utils.popen_read("npm pack").chomp | ||
| return if $?.exitstatus.zero? | ||
| raise "npm failed to pack #{Dir.pwd}" | ||
| end |
There was a problem hiding this comment.
does this still actually return the tarball path?
There was a problem hiding this comment.
irb(main):001:0> def foo
irb(main):002:1> 3
irb(main):003:1> return if 1 == 1
irb(main):004:1> end
=> :foo
irb(main):005:0> foo
=> nil
There was a problem hiding this comment.
It's not returning the variable, aye. Boo. So much for being lazy I guess 😄
There was a problem hiding this comment.
Might be able to use a lambda I guess. Maybe. Will check.
There was a problem hiding this comment.
Or I could just turn the Utils.popen_read into a block. I might try that. That feels sane. I've been thinking about this too long today & now my mind is sludge 😆
|
Nope. This doesn't |
|
I return, with a fresh push, which actually seems to work as intended now. I took some inspiration from |
MikeMcQuaid
requested changes
Jun 1, 2017
Library/Homebrew/language/node.rb
Outdated
| # create and install real directories when passed a file path. We can | ||
| # restore the old behaviour by packing the path first with "npm pack" | ||
| # which also helpfully spits out the filename it just created allowing | ||
| # us to pass that as the final argument. |
There was a problem hiding this comment.
I think we can trim this comment such that it doesn't reference the old behaviour; future people looking at this comment will want to know what it does and why it's necessary but doesn't really say that.
There was a problem hiding this comment.
Basically this comment should answer the question: "why do we need to run npm pack at all?" I don't understand the answer to that despite having read most of the conversation and definitely not just from this code.
There was a problem hiding this comment.
Hmm. I don't know how to explain it with brevity really, it's a fairly big change.
Essentially when you used to npm install some-directory --global --prefix="blah" you could then dispose of your build directory & purely retain the end product, the "blah" prefix in this example. From the new npm if you pass it a directory to install it only installs symlinks back to that directory in the "blah" prefix, which doesn't work for Homebrew since we trash buildpaths/testpaths the second we're done with 'em normally.
npm pack is supported as an upstream way of retaining the old behaviour Homebrew relies on to be able to sensibly package npm-using formulae.
There was a problem hiding this comment.
Ok, something like npm pack is needed to make a "clean" installation to a new directory i.e. not just install symlinks back to the source directory in the buildpath will do
There was a problem hiding this comment.
I was thinking about:
# Homebrew assumes the buildpath/testpath will always be disposable
# and from npm 5.0.0 the logic changed so that when a directory is
# fed to `npm install` only symlinks are created linking back to that
# directory, consequently breaking that assumption. We require a tarball
# because npm install creates a "real" installation when fed a tarball.But not sure if that's still too wordy/full/etc.
Library/Homebrew/language/node.rb
Outdated
| end | ||
|
|
||
| def self.std_npm_install_args(libexec) | ||
| setup_npm_environment | ||
| # tell npm to not install .brew_home by adding it to the .npmignore file | ||
| # (or creating a new one if no .npmignore file already exists) | ||
| open(".npmignore", "a") { |f| f.write("\n.brew_home\n") } | ||
| pack = pack_for_installation |
There was a problem hiding this comment.
Pedantry but I'd add a newline above this because the comment doesn't seem to reference it but yet it feels related.
|
When the time comes to roll here, ideally, we probably want:
I think that makes logical sense. |
MikeMcQuaid
approved these changes
Jun 1, 2017
|
Going to bed by any maintainer can feel free to merge this in the order Dom says above when green. I want to wait a bit after merge before cutting a new tag, though, just to make sure there's not other bugs there. |
ilovezfs
approved these changes
Jun 1, 2017
|
🚢'd Thanks @DomT4 |
|
Thanks @ilovezfs ❤️. This side of the changes proved relatively painless, thankfully. Or at least, so far 😄. |
|
Don't jinx it. |
|
🤐 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
brew testswith your changes locally? (It dies, but for very unrelated reasons as far as I can see.)Goes with: Homebrew/homebrew-core#14085