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.

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

doc: add note re term-size commit on top of npm #32403

Closed

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 21, 2020

Until npm updates update-notifier to a newer version, the dependency
tree will contain a version of term-size that has an unsigned macOS
binary. This will fail .pkg notarization and will result in failed
release builds. We built and signed a term-size and contributed it back
to the project for this purpose, but the dependency chain is long enough
that it's not likely to be included in a new npm very quickly.
Until it is, we need to cherry-pick commit d2f08a1 ontop of any npm
updates to master and any other release branch that includes
notarization.

The dependency chain is update-notifier -> boxen -> term-size. npm is on update-notifier@^2.5.0 but the latest is 4.1.0 and I can imagine it's a bit intimidating to just jump through those versions without grokking what's changed.

I've done this just now for master after the update to npm@6.14.3 in 4a3ccd8.

You can see what a failure looks like in the latest master nightly if you have access to ci-release: https://ci-release.nodejs.org/job/iojs+release/5763/nodes=osx1015-release-pkg/

17:54:06 3 errors occurred:
17:54:06 	* error for path "node-v14.0.0-nightly20200320f7771fffd0.pkg/npm-v6.14.3.pkg Contents/Payload/usr/local/lib/node_modules/npm/node_modules/term-size/vendor/macos/term-size": The binary is not signed.
17:54:06 	* error for path "node-v14.0.0-nightly20200320f7771fffd0.pkg/npm-v6.14.3.pkg Contents/Payload/usr/local/lib/node_modules/npm/node_modules/term-size/vendor/macos/term-size": The signature does not include a secure timestamp.
17:54:06 	* error for path "node-v14.0.0-nightly20200320f7771fffd0.pkg/npm-v6.14.3.pkg Contents/Payload/usr/local/lib/node_modules/npm/node_modules/term-size/vendor/macos/term-size": The executable does not have the hardened runtime enabled.

.pkg is missing from https://nodejs.org/download/nightly/v14.0.0-nightly20200320f7771fffd0/

@nodejs/build @nodejs/npm

Until npm updates update-notifier to a newer version, the dependency
tree will contain a version of term-size that has an unsigned macOS
binary. This will fail .pkg notarization and will result in failed
release builds. We built and signed a term-size and contributed it back
to the project for this purpose, but the dependency chain is long enough
that it's not likely to be included in a new npm very quickly.
Until it is, we need to cherry-pick commit d2f08a1 ontop of any npm
updates to master and any other release branch that includes
notarization.
@rvagg rvagg requested review from MylesBorins and Trott March 21, 2020 08:36
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 21, 2020
@rvagg rvagg mentioned this pull request Mar 21, 2020
doc/guides/maintaining-npm.md Outdated Show resolved Hide resolved
doc/guides/maintaining-npm.md Outdated Show resolved Hide resolved
rvagg and others added 2 commits March 23, 2020 13:23
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
@rvagg
Copy link
Member Author

rvagg commented Mar 23, 2020

.pkg in last nightly, after I ran the cherry-pick documented here, on master https://nodejs.org/download/nightly/v14.0.0-nightly20200322ecfb7b0988/

@sam-github
Copy link
Contributor

@nodejs/npm This is a fair amount build tooling required in order to get a box drawn around some text saying "an npm update is available", is there any chance that this could be fixed?

Note that the dependency on the executable is unnecessary, the equivalent functionality is already present in Node.js as pure-js: sindresorhus/terminal-size#15 (comment)

@rvagg will the dependency on an unsigned executable make npm install -g npm not work on OS X Catalina?

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

I'm not thrilled that this isn't getting fixed upstream, but that's out of our control, in the meantime, this is what we have to do.

ruyadorno added a commit to ruyadorno/cli that referenced this pull request Mar 23, 2020
Upstream node is patching `term-size` in order to avoid failing release
builds, see: nodejs/node#32403

Given that npm@6 has to support node6 and that dependency chain is long
enough, along with many major bumps dropping node6 along the way. I
propose we patch it here same as node upstream.

- ref: sindresorhus/terminal-size#16
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, agree we Sam, not optimal but what we need to do for now.

@rvagg
Copy link
Member Author

rvagg commented Mar 23, 2020

@rvagg will the dependency on an unsigned executable make npm install -g npm not work on OS X Catalina?

Nope! That's the fun of this (sarcasm), it's all about the .pkg building process, once it gets onto your machine then Gatekeeper butts out and you can change the on-disk files all you want. So for now, it only matter that macOS executables in our bundles are signed.

My guess is that this is only the beginning though, and maybe macOS is heading toward a container-style install system for packages, like Snaps are. It'll get interesting then. You can't overwrite the node Snap's npm with npm install -g npm as it is but I don't think I've seen any people complaining about that yet (probably because of Snap's being auto-updating so you don't really need to as long as we keep pushing out new versions of npm with Node).

rvagg added a commit that referenced this pull request Mar 24, 2020
Until npm updates update-notifier to a newer version, the dependency
tree will contain a version of term-size that has an unsigned macOS
binary. This will fail .pkg notarization and will result in failed
release builds. We built and signed a term-size and contributed it back
to the project for this purpose, but the dependency chain is long enough
that it's not likely to be included in a new npm very quickly.
Until it is, we need to cherry-pick commit d2f08a1 ontop of any npm
updates to master and any other release branch that includes
notarization.

PR-URL: #32403
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@rvagg rvagg closed this Mar 24, 2020
@rvagg rvagg deleted the rvagg/npm-term-size-cherry-pick-note branch March 24, 2020 02:23
@rvagg
Copy link
Member Author

rvagg commented Mar 24, 2020

Landed in 10d8e26

MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Until npm updates update-notifier to a newer version, the dependency
tree will contain a version of term-size that has an unsigned macOS
binary. This will fail .pkg notarization and will result in failed
release builds. We built and signed a term-size and contributed it back
to the project for this purpose, but the dependency chain is long enough
that it's not likely to be included in a new npm very quickly.
Until it is, we need to cherry-pick commit d2f08a1 ontop of any npm
updates to master and any other release branch that includes
notarization.

PR-URL: #32403
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@ruyadorno
Copy link
Member

@rvagg thanks for the patch! @sam-github worry not, we're going to be bringing it upstream to npm@6 npm/cli#1053 so that you can get rid of the extra step 😊

MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Until npm updates update-notifier to a newer version, the dependency
tree will contain a version of term-size that has an unsigned macOS
binary. This will fail .pkg notarization and will result in failed
release builds. We built and signed a term-size and contributed it back
to the project for this purpose, but the dependency chain is long enough
that it's not likely to be included in a new npm very quickly.
Until it is, we need to cherry-pick commit d2f08a1 ontop of any npm
updates to master and any other release branch that includes
notarization.

PR-URL: #32403
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
ruyadorno added a commit to npm/cli that referenced this pull request Mar 24, 2020
Upstream node is patching `term-size` in order to avoid failing release
builds, see: nodejs/node#32403

Given that npm@6 has to support node6 and that dependency chain is long
enough, along with many major bumps dropping node6 along the way. I
propose we patch it here same as node upstream.

- ref: sindresorhus/terminal-size#16

PR-URL: #1053
Credit: @ruyadorno
Close: #1053
Reviewed-by: @ruyadorno
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants