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

[WIP] language/node: fix prepublish scripts #2820

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@chrmoritz
Contributor

chrmoritz commented Jun 24, 2017

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

Refs: Homebrew/homebrew-core#14827

This PR adds support for installing node modules with a prepublish step, which requires the execution of some of it's (dev)dependencies. This is required for adding the now-cli formula to homebrew-core (see: Homebrew/homebrew-core#14827).

The current implementation of pack_for_installation is based on the assumption that the module we want to pack is self-contained. Unfortunately this is not always the case and some modules require a prepublish step, which is executed before (=at the beginning of) every npm pack run (and it was even executed during our old pre npm@5 way of installing modules, but there we had all dependencies in place).
Unfortunately prepublish scripts are likely trying to execute at least some of their dev(dependencies), so we have to install them a second (or first?) time before executing npm pack. I did made this an opt-in, so that we don't have the overhead of installing all dependencies twice in the majority of formulas for node modules without such prepublish steps.

cc @ilovezfs

I was able to successfully install now-cli and coffeescript with this PR.

@chrmoritz chrmoritz referenced this pull request Jun 24, 2017

Closed

now-cli 7.0.2 (new formula) #14827

4 of 4 tasks complete

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch from 4191e62 to e8ea18c Jun 24, 2017

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jun 24, 2017

Thinking about this further we may run into another edge case with prepublish scripts in the future (which is still no fixed in this current version of this PR):

Imagine a formula for a node module with a prepublish script, which does not support to be run twice in a row (e.g. you need to run a npm run clean before running prepublish a second time or it will fail because the build artifacts already exist). Now unlike now-cli it has a package on npm containing a copy of it with already build artifacts (prepublish is run before something is uploaded to the npm registry). Now if we use it inside the formula and run language/node on it our npm pack would run prepublish a second time on the already prebuild package, which would fail (see previous assumption). So we may need to add another parameter to std_npm_install_args which if set to true would pass an additional --ignore-scripts to npm pack to fix this use case.

Should I add support for this edge case too in this PR or do we want to wait until we need it in homebrew-core before supporting it?

BTW: I'm preparing a followup PR with more language/node improvements.

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch from e8ea18c to 43765db Jun 25, 2017

@@ -32,13 +40,13 @@ def self.setup_npm_environment
end
end
def self.std_npm_install_args(libexec)
def self.std_npm_install_args(libexec, prepublish_requires_deps = false)

This comment has been minimized.

@chrmoritz

chrmoritz Jun 25, 2017

Contributor

Should this stay an explicit opt-in or do we want to do this unconditionally? Note that later would add a for most formula unnecessary overhead of installing all dependencies twice (before pack and during install).

Also any suggestions for a better variable name?

unless $CHILD_STATUS.exitstatus.zero? && !output.lines.empty?
raise "npm failed to pack #{Dir.pwd}"
end
output.lines.last.chomp

This comment has been minimized.

@chrmoritz

chrmoritz Jun 25, 2017

Contributor

This change is needed because npm pack could contain verbose output from a prepublish script and we are only interested in the last line from npm itself containing the name of the created package.

Does someone with more knowledge in ruby has a suggestion how this could be improved?

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Jun 25, 2017

Thanks for this @chrmoritz!

I'm assuming these warnings are just an upstream issue they'll need to address at some point?

npm WARN prepublish-on-install As of npm@5, `prepublish` scripts are deprecated.
npm WARN prepublish-on-install Use `prepare` for build steps and `prepublishOnly` for upload-only.
npm WARN prepublish-on-install See the deprecation note in `npm help scripts` for more information.

> now@7.0.2 prepublish /private/tmp/now-cli-20170624-10884-uanglh/now-cli-7.0.2
> in-install || (npm run webpack && cp /dev/null download/dist/now)

npm notice created a lockfile as package-lock.json. You should commit this file.
npm WARN babel-loader@7.0.0 requires a peer of webpack@2 but none was installed.
npm WARN ajv-keywords@2.1.0 requires a peer of ajv@>=5.0.0 but none was installed.
# We don't want to run the prepublish script for some node modules in
# some formulae, so skip it by using --ignore-scripts.
pack_cmd += " --ignore-scripts" if pack_skip_prepublish
output = Utils.popen_read(pack_cmd)

This comment has been minimized.

@ilovezfs

ilovezfs Jun 25, 2017

Contributor

One thing I'd noticed while trying to get now-cli to work was that stderr for the pack command isn't getting printed, which made it a bit difficult to see what was going wrong. I wonder if there's a reasonable way to get stderr printed as well even though we need to capture stdout for the tarball name, or if we just need to continue to debug those situations by manually running npm pack within the debrew shell.

This comment has been minimized.

@chrmoritz

chrmoritz Jun 25, 2017

Contributor

+1 I too had to manually debug npm pack, through I don't know enough ruby to fix this.

This comment has been minimized.

@chrmoritz

chrmoritz Jun 25, 2017

Contributor

Looks like

output = `#{pack_cmd}`

does the job. (similar to shell_output)

@@ -32,13 +44,13 @@ def self.setup_npm_environment
end
end
def self.std_npm_install_args(libexec)
def self.std_npm_install_args(libexec, prepublish_requires_deps = false, pack_skip_prepublish = false)

This comment has been minimized.

@woodruffw

woodruffw Jun 25, 2017

Member

Minor nit, but let's shorten these arguments down and make them keywords:

def self.std_npm_install_args(libexec, req_deps: false, skip_prepub: false)

(Or something along those lines).

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 25, 2017

Member

I think longer arguments makes these much easier to read but agreed on keywords. req_ generally means requirement_ in Homebrew. This is why saving a few characters on variable names is generally a false economy as you need to add a comment to explain your translation.

This comment has been minimized.

@chrmoritz

chrmoritz Jun 25, 2017

Contributor

Something like requires_deps would be strictly speaking incorrect, because the deps would be required in any case at install time. Having to set requires_deps: false to not install deps at pack/prepublish time, but have them installed in the install step would likely create more confusion than the longer names.

*Note:* If your module requires a (dev)dependencies to to present at npm prepublish time, you have to invoke `std_npm_install_args` with `prepublish_requires_deps` set to `true` like:
```ruby
system "npm", "install", *Language::Node.std_npm_install_args(libexec, prepublish_requires_deps = true)

This comment has been minimized.

@woodruffw

woodruffw Jun 25, 2017

Member

You can just do std_npm_install_args(libexec, true) here, since it's a default parameter and not a keyword (unless it gets changed above, in which case it will become req_deps: true).

@@ -4,15 +4,27 @@ def self.npm_cache_config
"cache=#{HOMEBREW_CACHE}/npm_cache\n"
end
def self.pack_for_installation
def self.pack_for_installation(prepublish_requires_deps, pack_skip_prepublish)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 25, 2017

Member

I think they should be keyword or hash arguments; taking two booleans here means the method calls are a bit opaque.

# some formulae, so skip it by using --ignore-scripts.
pack_cmd += " --ignore-scripts" if pack_skip_prepublish
output = Utils.popen_read(pack_cmd)
unless $CHILD_STATUS.exitstatus.zero? && !output.lines.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 25, 2017

Member

Use if if it's for two conditions.

@@ -32,13 +44,13 @@ def self.setup_npm_environment
end
end
def self.std_npm_install_args(libexec)
def self.std_npm_install_args(libexec, prepublish_requires_deps = false, pack_skip_prepublish = false)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jun 25, 2017

Member

I think longer arguments makes these much easier to read but agreed on keywords. req_ generally means requirement_ in Homebrew. This is why saving a few characters on variable names is generally a false economy as you need to add a comment to explain your translation.

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jun 25, 2017

I've addressed some of the review comments in the latest commit and made all log output more verbose including the log output from npm pack as suggested by @ilovezfs here (it now matches the loglevel of the npm_debug.log's, while --verbose was previously an alias for -dd).

Note: In the current direction the now-cli is going it looks like running the republish scripts there is no longer needed (because we want to avoid bundling a null node executable into the binary and instead use our owb node formula at runtime for it). This doesn't mean there are no other possible formulas not submitted to core yet, which would benefit from the prepublish_requires_deps, through now-cli might only benefit from pack_skip_prepublish as proposed here.

  • I've changed the arguments to keyword, but I'm still not totally happy with their naming either (as original mention here). Through I have no ideas for better names for them, which would still be meaningful.
expect(resp).to include("--prefix=#{npm_install_arg}", "#{Dir.pwd}/pack")
end
#it "does not raise error with a zero exitstatus" do
# allow(Utils).to receive(:popen_read).with("npm pack").and_return("pack")

This comment has been minimized.

@chrmoritz

chrmoritz Jun 25, 2017

Contributor

is it possible to do something similar for

output = `#{pack_cmd}`

?

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Jun 25, 2017

Note: In the current direction the now-cli is going it looks like running the republish scripts there is no longer needed (because we want to avoid bundling a null node executable into the binary and instead use our owb node formula at runtime for it). This doesn't mean there are no other possible formulas not submitted to core yet, which would benefit from the prepublish_requires_deps, through now-cli might only benefit from pack_skip_prepublish as proposed here.

I'm pretty 👎 on merging anything that doesn't have at least one formula in core that wants/needs to use it.

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jun 25, 2017

I'm pretty 👎 on merging anything that doesn't have at least one formula in core that wants/needs to use it.

Thats what I wanted to know in the exact oposite case in #2820 (comment) .

Nevertheless it would still need the pack_skip_prepublish part of this PR and the more log output part of it coudn't hurt anyway too. Unneeded would only be the prepublish_requires_deps keyword and the part from line 12,

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Jun 25, 2017

Nevertheless it would still need the pack_skip_prepublish part of this PR and the more log output part of it coudn't hurt anyway too. Unneeded would only be the prepublish_requires_deps keyword and the part from line 12,

Yep 👍. Thanks @chrmoritz 🙇

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch from 7d864e8 to 9022d62 Jun 25, 2017

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 26, 2017

language/node: log verbose npm pack output
This makes npm pack to log verbose debug output to the console to
simplify debugging npm pack failures.
Refs: Homebrew#2820 (comment)
Prevously Utils.popen_read swallowed all debug output.

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 26, 2017

language/node: npm pack ignore prepublish scripts
This tells npm pack to don't run prepublish scripts at all.
I think this is the best default because:
* most modules don't have a prepublish script at all and aren't affected
  by this change
* most prepublish scripts are calling devDeps, which would fail in our
  case, because (dev)Deps aren't installed at npm pack time until Homebrew#2820
  gets resolved
* we favor npm registry tarball for formula downloads, which are already
  prepublished, so we would in the best case needlessly run prepublish
  a second time and in the worst case it would fail (because a clean
  step is required before running prepublish a second time in a row)
* This change does the right thing for >99% of all the packages and
  would only affect packages with prepublish scripts downloaded from a
  non-npm registry tarball (like github tarballs) and with a prepublish
  script wich does no't require any devDep (unlike for cross platform)

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch from 9022d62 to ee82827 Jun 26, 2017

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jun 26, 2017

The (dev)Deps required in prepublish scripts part of this PR is blocked by the ongoing discussion in the now-cli PR and @MikeMcQuaid comment about not wanting to fix prepublish scripts in language/node before it is actual needed in core.

Because of this I've split the improvements independent from this issue from this PR into a separate PR in #2826 (and added some other small improvements which I had already on my radar to get landed over there). Please review this PR in the meantime.

BTW: This PR is now rebased on #2826 with ee82827 as the only commit related to this PR anymore.

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Jun 27, 2017

Because of this I've split the improvements independent from this issue from this PR into a separate PR in #2826 (and added some other small improvements which I had already on my radar to get landed over there). Please review this PR in the meantime.

👍

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 27, 2017

language/node: log verbose npm pack output
This makes npm pack to log verbose debug output to the console to
simplify debugging npm pack failures.
Refs: Homebrew#2820 (comment)
Prevously Utils.popen_read swallowed all debug output.

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 27, 2017

language/node: npm pack ignore prepublish scripts
This tells npm pack to don't run prepublish scripts at all.
I think this is the best default because:
* most modules don't have a prepublish script at all and aren't affected
  by this change
* most prepublish scripts are calling devDeps, which would fail in our
  case, because (dev)Deps aren't installed at npm pack time until Homebrew#2820
  gets resolved
* we favor npm registry tarball for formula downloads, which are already
  prepublished, so we would in the best case needlessly run prepublish
  a second time and in the worst case it would fail (because a clean
  step is required before running prepublish a second time in a row)
* This change does the right thing for >99% of all the packages and
  would only affect packages with prepublish scripts downloaded from a
  non-npm registry tarball (like github tarballs) and with a prepublish
  script wich does no't require any devDep (unlike for cross platform)

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 28, 2017

language/node: log verbose npm pack output
This makes npm pack to log verbose debug output to the console to
simplify debugging npm pack failures.
Refs: Homebrew#2820 (comment)
Prevously Utils.popen_read swallowed all debug output.

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 28, 2017

language/node: npm pack ignore prepublish scripts
This tells npm pack to don't run prepublish scripts at all.
I think this is the best default because:
* most modules don't have a prepublish script at all and aren't affected
  by this change
* most prepublish scripts are calling devDeps, which would fail in our
  case, because (dev)Deps aren't installed at npm pack time until Homebrew#2820
  gets resolved
* we favor npm registry tarball for formula downloads, which are already
  prepublished, so we would in the best case needlessly run prepublish
  a second time and in the worst case it would fail (because a clean
  step is required before running prepublish a second time in a row)
* This change does the right thing for >99% of all the packages and
  would only affect packages with prepublish scripts downloaded from a
  non-npm registry tarball (like github tarballs) and with a prepublish
  script wich does no't require any devDep (unlike for cross platform)

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 28, 2017

language/node: log verbose npm pack output
This makes npm pack to log verbose debug output to the console to
simplify debugging npm pack failures.
Refs: Homebrew#2820 (comment)
Prevously Utils.popen_read swallowed all debug output.

chrmoritz added a commit to chrmoritz/brew that referenced this pull request Jun 28, 2017

language/node: npm pack ignore prepublish scripts
This tells npm pack to don't run prepublish scripts at all.
I think this is the best default because:
* most modules don't have a prepublish script at all and aren't affected
  by this change
* most prepublish scripts are calling devDeps, which would fail in our
  case, because (dev)Deps aren't installed at npm pack time until Homebrew#2820
  gets resolved
* we favor npm registry tarball for formula downloads, which are already
  prepublished, so we would in the best case needlessly run prepublish
  a second time and in the worst case it would fail (because a clean
  step is required before running prepublish a second time in a row)
* This change does the right thing for >99% of all the packages and
  would only affect packages with prepublish scripts downloaded from a
  non-npm registry tarball (like github tarballs) and with a prepublish
  script wich does no't require any devDep (unlike for cross platform)

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch from ea6d624 to 7880596 Jun 30, 2017

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Jul 7, 2017

Code looks good here whenever we've got a formula to use it 👍

language/node: add support for prepublish scripts
This commit adds support for installing node modules with language node,
which prepublish scripts, which are likely trying to call one of their
dev(Dependencies) so that these have to be installed before npm pack.

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch 2 times, most recently from d849cea to a38e908 Jul 11, 2017

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jul 11, 2017

I've rebased this on master, did some cleanup to tests, docs and bundledDeps logic and renamed the option prepare_required: In the long term naming the option after the since npm@4 deprecated prepublish lifecycle script doesn't felt right, even if it's still commonly used in the npm ecosystem for legacy reasons.

Since npm@1.1.71, the npm CLI has run the prepublish script for both npm publish and npm install, because it's a convenient way to prepare a package for use. It has also turned out to be, in practice, very confusing. As of npm@4.0.0, a new event has been introduced, prepare, that preserves this existing behavior. A new event, prepublishOnly has been added as a transitional strategy to allow users to avoid the confusing behavior of existing npm versions and only run on npm publish.

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch 2 times, most recently from 3b6b3df to a25f579 Jul 11, 2017

@MikeMcQuaid

A few comments on your comments 😉. In general would be good to try and slim these down a bit where possible.

module Language
module Node
def self.npm_cache_config
"cache=#{HOMEBREW_CACHE}/npm_cache"
end
def self.pack_for_installation
def self.pack_for_installation(prepare_required: false)
# Doing this unconditional could break for already prepared npm registry

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member
Using `prepare_required` unconditionally could
# tarballs, because our local npm install would unecessary call prepare /
# prepublish a second time on them which might fail (e.g. calling mkdir
# on an already existing folder).
# We also can't just always do the local npm install und bundledDeps into

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

und -> and

# prepublish a second time on them which might fail (e.g. calling mkdir
# on an already existing folder).
# We also can't just always do the local npm install und bundledDeps into
# package trick and just pass --ignore-scripts to the local npm install

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member
`--ignore-scripts`

and generally do this for all referenced variable/parameter names and other flags.

# package.json and tells npm to bundle the dependencies into the
# package created during npm pack so that we avoid the overhead of
# installing all dependencies a second time during the final step of
# installation the package to libexecs.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

Can this be more than a single sentence?

# Install all the modules dependencies already here and not just at
# npm pack time, because they might be used by the prepublish / prepare
# script. Note that this local install already executes both lifecycle
# scripts if present to that we can / must pass --ignore-scripts to

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

can or must? I don't think they are interchangeable.

# too and would break for example installing of native addons.
if prepare_required
# Install all the modules dependencies already here and not just at
# npm pack time, because they might be used by the prepublish / prepare

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 12, 2017

Member

Pick prepublish or prepare here.

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jul 12, 2017

I agree that the comments in this PR aren't ideal. But I think even the current amount of comments aren't enough, if anyone should ever be able to understand the quite complex reasoning behind the code to support every possible edge case with our current approve (regarding lifecycle scripts) by just reading the language/node sources.

Maybe it would be better to write a PR comment gist explaining this and just link to it from a comment, to don't blow up the sources to much?

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jul 12, 2017

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch 2 times, most recently from 5643cb5 to 2ae1521 Jul 12, 2017

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jul 12, 2017

In theory we could remove the if prepare_required block from this PR and do 2. unconditionally to get the cleaner more npm supported way for language/node every time. Unfortunately this would bring language/node back to a state where case 3 + 4 are not supported (like before #2826), because npm registry tarballs where never meant to be extracted before installation, which breaks npms assumption that only tarballs are already prepared (and extracted stuff is not prepared). To fix this we would need again a flag like prepare_required to tell the first local npm install call wether to execute prepare or not by passing --production to it unless prepare_required (see npm background#1+4). Any opinion on wether we should do this?

Edit: Just found out that passing --production to a local npm install will not only prevent devDeps from being installed, but also prevents prepare script from being executed (because they might require devDeps for execution) while still executing (pre/post)install scripts during installing the deps. This makes being consistent with bundling all deps into the package (see method#2 above) much more easier while still supporting case 3+4. I've added a 3rd commit with this approve to the PR.

BTW: Removing the prepare_required option and always using --production would be equivalent to the current version of language/node in master and never using --production would be 100% equivalent to the state of language/node we had prior to the npm@5 upgrade.

chrmoritz added some commits Jul 1, 2017

language/node: bundle dependencies into package
if prepare_required is set to avoid installing all dependencies twice by
adding all dependencies to the bundledDependencies array of package.json

This makes the package created by npm pack, which we install later into
libexec self-contained, because it will contain all dependencies.
language/node: always bundle deps into pack
This way all cases are handled more consistently.
This also bring us further away from our current hacky usage of npm pack
as a npm@5 install symlink workaround. With this patch we are calling
npm pack on a complete dep tree (as expected / supported by npm) and we
get prepare script support out of the box from it.
Also this matches exactly what the pre npm@<=4 (node <=7) version of
language/node had done in npm internally before.

@chrmoritz chrmoritz force-pushed the chrmoritz:nodePrepublish branch from 2ae1521 to 2caf79e Jul 12, 2017

@chrmoritz

This comment has been minimized.

Contributor

chrmoritz commented Jul 12, 2017

testing 25b201e (all formula in homebrew-core will pass installation and test with this after Homebrew/homebrew-core#15553 gets merged):

  • prepare test formula
  • alexjs
  • angular-cli
  • autocode
  • azure-cli
  • babel
  • bower
  • chronograf (fails because of formula issues: incorrectly mixing yarn and npm, fixed in Homebrew/homebrew-core#15553)
  • coffeescript
  • diff-so-fancy
  • firebase-cli
  • generate-json-schema
  • gitter-cli
  • grunt-cli
  • heroku
  • insect
  • ios-sim
  • jhipster
  • jsdoc3
  • jsonlint
  • nativefier
  • svgo
  • typescript
  • webpack
  • write-good

@MikeMcQuaid Is there a way to use a system variant inside a language which handles brew verbose flag correctly like the system from formula (show stdout/stderr output if verbose is set and hide it if not)?

chrmoritz referenced this pull request in chrmoritz/homebrew-core Jul 12, 2017

module Language
module Node
def self.npm_cache_config
"cache=#{HOMEBREW_CACHE}/npm_cache"
end
def self.pack_for_installation
def self.node_system(cmd, *args)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 13, 2017

Member

Would be good to use the formula system method instead of reimplementing it here.

This comment has been minimized.

@chrmoritz

chrmoritz Jul 13, 2017

Contributor

Yes, but how to do so? Using just system inside a language will call Homebrew/utils.rb system and not Homebrew/formula.rb one.

An alternative implementation of this using util's _system(basically mixing the implementation of safe_system and quite_system) would be:

def self.node_system(cmd, *args)
  ohai "#{cmd} #{args * " "}"
  Homebrew._system(cmd, *args) do
    unless ARGV.verbose?
      $stdout.reopen("/dev/null")
      $stderr.reopen("/dev/null")
    end
  end
  raise ErrorDuringExecution.new(cmd, args) unless $CHILD_STATUS.success?
end

This comment has been minimized.

@chrmoritz

chrmoritz Jul 14, 2017

Contributor

And speaking about formula: Is it possible to access the formula's download url inside language/node? With it we could match it against a npm registry url regex and figure the correct way to handle life cycle scripts out on our own without requiring the formula author to set the prepare_require parameter.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 15, 2017

Member

The formula object can be passed through as an argument or this module could be included inside the formula itself. That'll handle both of these cases.

# Rewrites the package.json so that npm pack will bundle all deps
# into a self-contained package, so that we avoid installing them a
# second time during the final installation of the package to libexec.
pkg_json = JSON.parse(IO.read("package.json"))

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 13, 2017

Member

May be worth producing a nice error message if this file doesn't exist?

This comment has been minimized.

@chrmoritz

chrmoritz Jul 13, 2017

Contributor

What error message do you have in mind here?

A package.json should always exist unless someone is calling language/node from a directory, which is not a valid node module. The Error: No such file or directory - package.json from IO is nearly identical to the error from npm pack (npm ERR! code ENOLOCAL: Could not install from "" as it does not contain a package.json file.) someone would get with the current version of language/node when calling it on a non node module folder.

This comment has been minimized.

@chrmoritz

chrmoritz Jul 14, 2017

Contributor

Replacing the error msg with our own custom one would also hide the actual kind of the IO error: package.json doesn't exist (see above), permission error (no read/write permission) ...
Just doing nothing here and throw the original IO error seems to be the best for debugging.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Jul 15, 2017

Member

I was thinking any error message that's not a raw exception but you've convinced me this is fine as-is.

fcolasuonno-bbc added a commit to fcolasuonno-bbc/brew that referenced this pull request Aug 2, 2017

language/node: log verbose npm pack output
This makes npm pack to log verbose debug output to the console to
simplify debugging npm pack failures.
Refs: Homebrew#2820 (comment)
Prevously Utils.popen_read swallowed all debug output.

fcolasuonno-bbc added a commit to fcolasuonno-bbc/brew that referenced this pull request Aug 2, 2017

language/node: npm pack ignore prepublish scripts
This tells npm pack to don't run prepublish scripts at all.
I think this is the best default because:
* most modules don't have a prepublish script at all and aren't affected
  by this change
* most prepublish scripts are calling devDeps, which would fail in our
  case, because (dev)Deps aren't installed at npm pack time until Homebrew#2820
  gets resolved
* we favor npm registry tarball for formula downloads, which are already
  prepublished, so we would in the best case needlessly run prepublish
  a second time and in the worst case it would fail (because a clean
  step is required before running prepublish a second time in a row)
* This change does the right thing for >99% of all the packages and
  would only affect packages with prepublish scripts downloaded from a
  non-npm registry tarball (like github tarballs) and with a prepublish
  script wich does no't require any devDep (unlike for cross platform)

mansimarkaur added a commit to mansimarkaur/brew that referenced this pull request Aug 5, 2017

language/node: log verbose npm pack output
This makes npm pack to log verbose debug output to the console to
simplify debugging npm pack failures.
Refs: Homebrew#2820 (comment)
Prevously Utils.popen_read swallowed all debug output.

mansimarkaur added a commit to mansimarkaur/brew that referenced this pull request Aug 5, 2017

language/node: npm pack ignore prepublish scripts
This tells npm pack to don't run prepublish scripts at all.
I think this is the best default because:
* most modules don't have a prepublish script at all and aren't affected
  by this change
* most prepublish scripts are calling devDeps, which would fail in our
  case, because (dev)Deps aren't installed at npm pack time until Homebrew#2820
  gets resolved
* we favor npm registry tarball for formula downloads, which are already
  prepublished, so we would in the best case needlessly run prepublish
  a second time and in the worst case it would fail (because a clean
  step is required before running prepublish a second time in a row)
* This change does the right thing for >99% of all the packages and
  would only affect packages with prepublish scripts downloaded from a
  non-npm registry tarball (like github tarballs) and with a prepublish
  script wich does no't require any devDep (unlike for cross platform)
@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Sep 5, 2017

@chrmoritz What's the status of this?

@MikeMcQuaid

This comment has been minimized.

Member

MikeMcQuaid commented Sep 25, 2017

@chrmoritz Closing for now but happy to reopen when you let us know the status.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.