Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

tools, build: macOS installer improvements #15179

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
Member

jpwesselink commented Sep 4, 2017 edited

Replacement for current macOS installer scripts.

This builds on prior work by @rvagg, @lance, @fhemberger & @evanlucas

  • Uses pkgbuild and productbuild instead of outdated Packagemaker.
  • Optional installation of npm.
  • Removes previous npm installation before installing npm.
  • Packages carry correct version attributes.
  • Support for intl installer features, defaults to en.
  • Fancy formatted license.
  • Renamed osx references to macOS.

Packagemaker, which is used for building the macOS packages is outdated. The last release was in 2012, as part of the Auxiliary Tools for Xcode. Next to this, it seems thatPackagemaker is poorly documented, and unwieldy to use at best. pkgbuild and productbuild are easier to use, still supported by Apple, and (correct me if I'm wrong), the de facto standard for creating macOS installation files.

The optional installation of npm also removes any previous installed npm distribution.

Package versions also propagate to the package artefacts, which will fix #15012

$ pkgutil --pkg-info org.nodejs.node.pkg
package-id: org.nodejs.node.pkg
version: v9.0.0-nightly2017-09-04c34f2eae80
volume: /
location: /
install-time: 1504526061
$ pkgutil --pkg-info org.nodejs.npm.pkg
package-id: org.nodejs.npm.pkg
version: v5.3.0
volume: /
location: /
install-time: 1504523991

License is formatted using ./tools/license2rtf.js, but since the font size turns out fairly small, it might need some tweaking.

Localized resources can be provided by adding them to{lang}.lproj directory to ./tools/macos-installer/productbuild/Resources/. As of writing the supported language is English. Please see Apple's documentation on Localized Resources in Bundles

To test packaging without code or package signing, I found that the method below worked best for me. (if there is a better way to do this, I'd love to know)

$ DISTTYPE=nightly DATESTRING=2017-09-04 COMMIT=c34f2eae80 make pkg -j 4

cc @nodejs/build, @jasnell, @rvagg, @MylesBorins, @watilde, @ashleygwilliams

Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097

Checklist
Affected core subsystem(s)

tools, build

jpwesselink added a commit to jpwesselink/node that referenced this pull request Sep 4, 2017

tools, build: macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.

PR-URL: nodejs#15179
Fixes: nodejs#15012
Refs: nodejs#5656
Refs: nodejs#2571
Refs: nodejs#7097

Thanks for picking this up!! I'll try to run it this week and confirm that code signing still works and such as well.

Looks good! I noted in the code that the symlink path to npx needs to be changed. Otherwise, it's a great improvement.

+
+cd /usr/local/bin || exit 1
+ln -sf ../lib/node_modules/npm/bin/npm-cli.js npm
+ln -sf ../lib/node_modules/npx/bin/npx-cli.js npx
@lance

lance Sep 5, 2017

Contributor

This should be:

ln -sf ../lib/node_modules/npm/bin/npx-cli.js npx
@jpwesselink

jpwesselink Sep 5, 2017 edited

Member

Ok to amend those changes to same commit, or should I add new commits, and squash later?

@lance

lance Sep 5, 2017

Contributor

Everything will be squashed when it lands in master, so you can just add new commits.

jpwesselink added a commit to jpwesselink/node that referenced this pull request Sep 5, 2017

tools, build: macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.

PR-URL: nodejs#15179
Fixes: nodejs#15012
Refs: nodejs#5656
Refs: nodejs#2571
Refs: nodejs#7097

lance approved these changes Sep 5, 2017

Member

fhemberger commented Sep 6, 2017

@jpwesselink Awesome, thanks for picking it up! Hope we can finally land a new installer this time. 😀

Member

jpwesselink commented Sep 6, 2017

@fhemberger I hope we can :) New users would really benefit from it

jasnell approved these changes Sep 6, 2017

Gave it a try and it appears to work. Rubber stamp LGTM but I'm not an expert with macos installers so would appreciate others taking a look @nodejs/tsc

Contributor

mscdex commented Sep 6, 2017

@nodejs/platform-macos is probably a better group to ping?

+#!/bin/sh
+
+[[ -d /usr/local/lib/node_modules/npm ]] \
+ && rm -rf /usr/local/lib/node_modules/npm
@joyeecheung

joyeecheung Sep 7, 2017

Contributor

This script needs to return an exit code 0 to work (see docosx). Otherwise it will abort (showing "The installation failed" if there isn't a /usr/local/lib/node_modules/npm ). Can you add

exit 0

to the end of this file?

@jpwesselink

jpwesselink Sep 7, 2017

Member

Sure can

Also a couple suggestions

Makefile
+ $(RM) -r $(MACOSOUTDIR)
+ mkdir -p $(MACOSOUTDIR)/installer/productbuild
+ cat tools/macos-installer/productbuild/distribution.xml.tmpl \
+ | sed -E "s/\\{nodeversion\\}/$(FULLVERSION)/g" \
@joyeecheung

joyeecheung Sep 7, 2017

Contributor

Nit: tab indentation (also the similar lines below)

+ </head>
+ <body>
+ <p>
+ Node.js was installed at <code>/usr/local/bin/node</code>
@joyeecheung

joyeecheung Sep 7, 2017 edited

Contributor

The message looks like this after installation:

screen shot 2017-09-07 at 1 43 13 pm

Can you wrap each line with <p> or <li>(with <ul> or <ol>) so they would break better? Also it would be nice to include the version numbers in the message.

@jpwesselink

jpwesselink Sep 7, 2017

Member

Like Node.js {nodeversion} was installed ...?

@joyeecheung

joyeecheung Sep 7, 2017

Contributor

Maybe something like

<div>
  <p>This package has installed:</p>
  <ul>
    <li>Node.js {nodeversion} to <code>/usr/local/bin/node</code></li>
    <li>npm {npmversion} to <code>/usr/local/bin/npm</code></li>
  </ul>
  <p>Make sure that <code>/usr/local/bin</code> is in your <code>$PATH</code>.</p>
</div>
@jpwesselink

jpwesselink Sep 7, 2017

Member

TY, let me grab some screenshots of the result

@jpwesselink

jpwesselink Sep 7, 2017

Member

Here we go.
screen shot 2017-09-07 at 08 29 39

I think we should wrap the unordered list in a paragraph as well to give the last line some space.

@joyeecheung

joyeecheung Sep 8, 2017

Contributor

I would suggest to use css for that because technically paragraphs can not contain lists...

+ </head>
+ <body>
+ <p>
+ This package will install Node.js {nodeversion} and npm {npmversion} into <code>/usr/local/</code>.
@joyeecheung

joyeecheung Sep 7, 2017

Contributor

It would be nice if this message is formatted similar to how the conclusion message is formatted, maybe like this:

<div>
  <p>This package will install</p>
  <ul>
    <li>Node.js {nodeversion} to <code>/usr/local/bin/node</code></li>
    <li>npm {npmversion} to <code>/usr/local/bin/npm</code></li>
  </ul>
</div>
@jpwesselink

jpwesselink Sep 7, 2017

Member

How about this? screen shot 2017-09-07 at 08 29 17

@joyeecheung

joyeecheung Sep 8, 2017

Contributor

Looking good!

jpwesselink added a commit to jpwesselink/node that referenced this pull request Sep 8, 2017

tools, build: macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm

PR-URL: nodejs#15179
Fixes: nodejs#15012
Refs: nodejs#5656
Refs: nodejs#2571
Refs: nodejs#7097
Member

jpwesselink commented Sep 8, 2017

Also adjusted the vertical alignment of the logo to match the logo alignment of the current installer.

Contributor

lance commented Sep 8, 2017

Should there be a semver-[major|minor] for a change to the installer?

Member

gibfahn commented Sep 8, 2017

Should there be a semver-[major|minor] for a change to the installer?

Only if it's a breaking change for end-users. Which bits would be? Maybe this?

Removes previous npm installation before installing npm.

LGTM, thanks!

Member

jpwesselink commented Sep 9, 2017 edited

Removes previous npm installation before installing npm.

I implemented this since I assumed that it is a desired feature long awaited bugfix after reading #2571 (diff) and #7097 (diff).

This will only remove /usr/local/lib/node_modules/npm directory. Can I leave the contents of the existing /usr/local/lib/node_modules directory as-is?

Owner

MylesBorins commented Sep 9, 2017

Removes previous npm installation before installing npm.

seems like a bugfix to me tbqh

Member

gibfahn commented Sep 9, 2017

To be clear I'm +1 on this being semver-patch (unless there's something I've missed that could break users).

Member

jpwesselink commented Sep 9, 2017

I’ll try and set up a couple of macOS and OSX boxes to test the installer

Member

jpwesselink commented Sep 12, 2017 edited

I couldn't find a test-plan for node on OSX / macOS (is there any?), so I have tested the node-v9.0.0 + npm-v5.3.0 installer on macOS Sierra 10.12.6 (16G29) as follows:

  • Does the installer put the files in the right location?
  • Are the binaries in PATH?
  • Does echo "console.log(require('fs').readFileSync('/usr/share/emacs/22.1/etc/COOKIES', 'utf8'))" | node return a cookie recipe?

Results

  • clean macOS install
  • with installed node-v6.11.3.pkg (npm i -g needs sudo)
  • with installed node-v8.4.0.pkg (npm i -g needs sudo)
  • with brew
  • with brew and brew's node-v6.11.3 (needs fix to PATH)
  • with brew and brew's node-v8.4.0
  • with nvm and nvm's node-v6.11.3 (npm i -g needs sudo)
  • with nvm and nvm's node-v8.4.0 (npm i -g needs sudo)

Problems

If node-v6.11.3 is installed with brew the PATH needs to be fixed since it will find brew's node installation first. Remove export PATH="/usr/local/opt/node@6/bin:$PATH" from ./bash_profile (and likes) will solve this.

Not sure if these tests reflect a trustworthy way of saying that 'it works on a mac', I'd love to have more input on this!

Owner

jasnell commented Sep 14, 2017

Generally we have no set plan for testing the installers... which is a problem. We need to have one. Generally this only ends up getting spot tested as we are doing releases, and generally only once in a while. I'll give this a try on my mac tomorrow.

tools, build: macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm

PR-URL: #15179
Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097
Member

BridgeAR commented Sep 19, 2017

Ping @jasnell

Owner

jasnell commented Sep 19, 2017

Owner

jasnell commented Sep 20, 2017

Ok, finally finished up. LGTM! Would really like @rvagg to take a look but not critical if he doesn't have the time.

Member

jpwesselink commented Sep 21, 2017

Member

BridgeAR commented Sep 28, 2017

Landed in 03954f7

@BridgeAR BridgeAR closed this Sep 28, 2017

BridgeAR added a commit that referenced this pull request Sep 28, 2017

tools, build: refactor macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm.

PR-URL: #15179
Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Owner

MylesBorins commented Sep 28, 2017

I'm removing semver-major and replaced it with semver-minor.

landing in v8.x-staging. Will include this in upcoming r.c. on Tuesday.

Assuming there are no regressions here I think we should land it in the upcoming 6.x minor. Will punt that decision over to @nodejs/lts

Member

sam-github commented Sep 28, 2017

If this is found to be stable, we should backport, installation is an important part of a robust experience. Installer changes can be bug-prone, its good to wait for the installer to be used on lots of weird OS X systems to make sure nothing unintended was introduced.

MylesBorins added a commit that referenced this pull request Sep 29, 2017

tools, build: refactor macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm.

PR-URL: #15179
Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 29, 2017

tools, build: refactor macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm.

PR-URL: #15179
Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 30, 2017

tools, build: refactor macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm.

PR-URL: nodejs/node#15179
Fixes: nodejs/node#15012
Refs: nodejs/node#5656
Refs: nodejs/node#2571
Refs: nodejs/node#7097
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 3, 2017

tools, build: refactor macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm.

PR-URL: #15179
Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 3, 2017

Merged

v8.7.0 proposal #15762

MylesBorins added a commit that referenced this pull request Oct 3, 2017

tools, build: refactor macOS installer
Creates macOS pkg installer by using `pkgbuild` and `productbuild`.
Removes previous npm installation before installing npm.
Packages carry correct version attributes.
Support for intl installer features, defaults to `en`.
Fancy formatted license.
Renamed `osx` references to `macOS`.
Optional installation of npm.

PR-URL: #15179
Fixes: #15012
Refs: #5656
Refs: #2571
Refs: #7097
Reviewed-By: Lance Ball <lball@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Contributor

lance commented Oct 7, 2017

How long before baking-for-lts becomes backport-requested-v6.x? Agree with @sam-github about backporting, but I'm unclear on how long baking-for-lts takes. I'd say 350° for 20 minutes, but that's probably more appropriate for my chocolate chip cookies.

Member

gibfahn commented Oct 7, 2017 edited

How long before baking-for-lts becomes backport-requested-v6.x? Agree with @sam-github about backporting, but I'm unclear on how long baking-for-lts takes. I'd say 350° for 20 minutes, but that's probably more appropriate for my chocolate chip cookies.

We don't have a set duration. I'd normally say one or two releases is a good baking duration. However as 6.x is going into maintenance in 6 months, we (LTS) need to decide whether this should land in this semver-minor (rc on the 17th) or the next one (assuming we do another semver-minor).

Thoughts @nodejs/lts ? I'd be inclined to leave it till the next one.

Assuming there are no regressions here I think we should land it in the upcoming 6.x minor. Will punt that decision over to @nodejs/lts

We can't really make that assumption though, if we assumed no regressions we'd backport everything immediately.

Member

richardlau commented Oct 8, 2017

We don't have a set duration. I'd normally say one or two releases is a good baking duration. However as 6.x is going into maintenance in 6 months, we (LTS) need to decide whether this should land in this semver-minor (rc on the 17th) or the next one (assuming we do another semver-minor).

Thoughts @nodejs/lts ? I'd be inclined to leave it till the next one.

This has yet to be released in any version; it's currently proposed for version 8.7.0. Assuming the proposed dates don't change that would be a week where this new installer is out in the wild.

Do we have any statistics for how used release candidates are (on macOS)? Unfortunately we don't have any automated tests for the installer so the only way to check for regressions is actual usage. I'd be inclined to agree to leave it to the next one.

Contributor

joyeecheung commented Oct 8, 2017

+1 to see if there is any regressions in 8.7.0 first

Owner

MylesBorins commented Oct 8, 2017 edited by gibfahn

Member

gibfahn commented Oct 8, 2017

If it fixes that npm issue, and @MylesBorins is willing to do another release to revert if something goes wrong, then I'm okay with it going into v6.12.0. It's unlikely to break things in production at any rate.

All this being said, please feel free to raise the issue in the 8.7.0 release pr if you feel strongly, we can get more input and attempt to reach consensus

Don't think anyone suggested it shouldn't go into 8.7.0, question was more around whether there's enough time between 8.7.0 and 6.12.0 to be confident that it works.

If we release Tuesday we can have it in the next 6.x minor r.c. and will have three weeks for it to bake in the wild.

This is good, but I'm not sure how we can encourage users to use the r.c. and try the Mac installer.

MylesBorins added a commit that referenced this pull request Oct 11, 2017

2017-10-11, Node.js Version 8.7.0 (Current)
Notable Changes:

* deps:
  * update npm to 5.4.2
    #15600
  * upgrade libuv to 1.15.0
    #15745
  * update V8 to 6.1.534.42
    #15393
* dgram:
  * support for setting dgram socket buffer size
    #13623
* fs:
  * add support O_DSYNC file open constant
    #15451
* util:
  * deprecate obj.inspect for custom inspection
    #15631
* tools, build:
  * there is a fancy new macOS installer
    #15179
* Added new collaborator
  * bmeurer - Benedikt Meurer - https://github.com/bmeurer
  * kfarnung - Kyle Farnung - https://github.com/kfarnung

PR-URL: #15762

MylesBorins added a commit that referenced this pull request Oct 11, 2017

2017-10-11, Node.js Version 8.7.0 (Current)
Notable Changes:

* deps:
  * update npm to 5.4.2
    #15600
  * upgrade libuv to 1.15.0
    #15745
  * update V8 to 6.1.534.42
    #15393
* dgram:
  * support for setting dgram socket buffer size
    #13623
* fs:
  * add support O_DSYNC file open constant
    #15451
* util:
  * deprecate obj.inspect for custom inspection
    #15631
* tools, build:
  * there is a fancy new macOS installer
    #15179
* Added new collaborator
  * bmeurer - Benedikt Meurer - https://github.com/bmeurer
  * kfarnung - Kyle Farnung - https://github.com/kfarnung

PR-URL: #15762

addaleax added a commit to addaleax/ayo that referenced this pull request Oct 12, 2017

2017-10-11, Node.js Version 8.7.0 (Current)
Notable Changes:

* deps:
  * update npm to 5.4.2
    nodejs/node#15600
  * upgrade libuv to 1.15.0
    nodejs/node#15745
  * update V8 to 6.1.534.42
    nodejs/node#15393
* dgram:
  * support for setting dgram socket buffer size
    nodejs/node#13623
* fs:
  * add support O_DSYNC file open constant
    nodejs/node#15451
* util:
  * deprecate obj.inspect for custom inspection
    nodejs/node#15631
* tools, build:
  * there is a fancy new macOS installer
    nodejs/node#15179
* Added new collaborator
  * bmeurer - Benedikt Meurer - https://github.com/bmeurer
  * kfarnung - Kyle Farnung - https://github.com/kfarnung

PR-URL: nodejs/node#15762
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment