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.

Sign up for GitHub

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

node: 22.9.1 (updated install instructions) #194041

Closed
wants to merge 2 commits into from
+0 −61
Closed

node: 22.9.1 (updated install instructions) #194041

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e022698
node: 22.9.1 (updated install instructions)
ovflowd Oct 12, 2024
33f7636
Merge branch 'master' into patch-1
ovflowd Oct 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 0 additions & 61 deletions Formula/n/node.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,6 @@

fails_with gcc: "5"

# We track major/minor from upstream Node releases.
# We will accept *important* npm patch releases when necessary.
resource "npm" do
url "https://registry.npmjs.org/npm/-/npm-10.8.3.tgz"
sha256 "b7dc7eb48d7479b93668e913c7ad686ab2aa71c705d4a56b5323d1bffdba2972"
end
Comment on lines -49 to -54
Copy link
Member

Choose a reason for hiding this comment

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

This is the same version as what's included with Node 22.9.0: nodejs/node@61047dd130

Copy link
Author

Choose a reason for hiding this comment

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

So that's what # We track major/minor from upstream Node releases. refers to?

Is there any technical reason of why then omitting the default way Node will attempt to bundle NPM?

Copy link
Author

Choose a reason for hiding this comment

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

If there any reasons at all, whnich IMO are valid if given to OS or compatibility reasons -- can a warn/info be added?

Like "Homebrew will manually bundle the targeted npm version for this Node.js version -- read more at XXX.XYZ"

Giving knowledge to the end-user is always fundamental IMO.

Copy link
Member

@carlocab carlocab Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So that's what # We track major/minor from upstream Node releases. refers to?

Yes. It means that the major and minor version should be the same, and ideally the patch version is the same unless there are very strong reasons not for it to be (see line just below that).

Is there any technical reason of why then omitting the default way Node will attempt to bundle NPM?

Not really sure -- it's been done this way since Homebrew/legacy-homebrew#28075, which is well before I was involved in Homebrew.

But, given that it's been done this way for nearly a decade (or more?), and it seems to be working well since then: we'd better have very strong reasons to change it.

If there any reasons at all, whnich IMO are valid if given to OS or compatibility reasons -- can a warn/info be added?

Like "Homebrew will manually bundle the targeted npm version for this Node.js version -- read more at XXX.XYZ"

We can add it to caveats whenever the version doesn't match from the one upstream, but I don't know of any instance when it didn't.


def install
ENV.llvm_clang if OS.mac? && (DevelopmentTools.clang_build_version <= 1100)

Expand All @@ -62,12 +55,8 @@
# make sure subprocesses spawned by make are using our Python 3
ENV["PYTHON"] = which("python3.12")

# Never install the bundled "npm", always prefer our
# installation from tarball for better packaging control.
args = %W[
--prefix=#{prefix}
--without-npm
--without-corepack
--with-intl=system-icu
--shared-libuv
--shared-nghttp2
Expand Down Expand Up @@ -97,57 +86,9 @@

system "./configure", *args
system "make", "install"

# Allow npm to find Node before installation has completed.
ENV.prepend_path "PATH", bin

bootstrap = buildpath/"npm_bootstrap"
bootstrap.install resource("npm")
# These dirs must exists before npm install.
mkdir_p libexec/"lib"
system "node", bootstrap/"bin/npm-cli.js", "install", "-ddd", "--global",
"--prefix=#{libexec}", resource("npm").cached_download

# The `package.json` stores integrity information about the above passed
# in `cached_download` npm resource, which breaks `npm -g outdated npm`.
# This copies back over the vanilla `package.json` to fix this issue.
cp bootstrap/"package.json", libexec/"lib/node_modules/npm"
# These symlinks are never used & they've caused issues in the past.
rm_r libexec/"share" if (libexec/"share").exist?

bash_completion.install bootstrap/"lib/utils/completion.sh" => "npm"
end

def post_install
Copy link
Member

Choose a reason for hiding this comment

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

Note that removing this will likely mean that user-installed Node modules go into the 🗑️ when they brew upgrade node.

Copy link
Author

Choose a reason for hiding this comment

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

I had no idea, and this was not intentional. What chunks of the post_install cover that?

node_modules = HOMEBREW_PREFIX/"lib/node_modules"
node_modules.mkpath
# Kill npm but preserve all other modules across node updates/upgrades.
rm_r node_modules/"npm" if (node_modules/"npm").exist?

cp_r libexec/"lib/node_modules/npm", node_modules
# This symlink doesn't hop into homebrew_prefix/bin automatically so
# we make our own. This is a small consequence of our
# bottle-npm-and-retain-a-private-copy-in-libexec setup
# All other installs **do** symlink to homebrew_prefix/bin correctly.
# We ln rather than cp this because doing so mimics npm's normal install.
ln_sf node_modules/"npm/bin/npm-cli.js", bin/"npm"
ln_sf node_modules/"npm/bin/npx-cli.js", bin/"npx"
ln_sf bin/"npm", HOMEBREW_PREFIX/"bin/npm"
ln_sf bin/"npx", HOMEBREW_PREFIX/"bin/npx"

# Create manpage symlinks (or overwrite the old ones)
%w[man1 man5 man7].each do |man|
# Dirs must exist first: https://github.com/Homebrew/legacy-homebrew/issues/35969
mkdir_p HOMEBREW_PREFIX/"share/man/#{man}"
# still needed to migrate from copied file manpages to symlink manpages
rm(Dir[HOMEBREW_PREFIX/"share/man/#{man}/{npm.,npm-,npmrc.,package.json.,npx.}*"])
ln_sf Dir[node_modules/"npm/man/#{man}/{npm,package-,shrinkwrap-,npx}*"], HOMEBREW_PREFIX/"share/man/#{man}"
end

(node_modules/"npm/npmrc").atomic_write("prefix = #{HOMEBREW_PREFIX}\n")
end

test do

Check failure on line 91 in Formula/n/node.rb

View workflow job for this annotation

GitHub Actions / Linux

`brew test --verbose node` failed on Linux! 

/home/linuxbrew/.linuxbrew/Homebrew/Library/Homebrew/test.rb:52:in `<main>'
# Make sure Mojave does not have `CC=llvm_clang`.
ENV.clang if OS.mac?

Expand All @@ -162,8 +103,6 @@
output = shell_output("#{bin}/node -e 'console.log(new Intl.NumberFormat(\"de-DE\").format(1234.56))'").strip
assert_equal "1.234,56", output

# make sure npm can find node
ENV.prepend_path "PATH", opt_bin
ENV.delete "NVM_NODEJS_ORG_MIRROR"
assert_equal which("node"), opt_bin/"node"
assert_predicate HOMEBREW_PREFIX/"bin/npm", :exist?, "npm must exist"
Expand Down
Loading