GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Refactor Mac installer generation by remixz · Pull Request #776 · nodejs/node

Conversation

@remixz

@remixz remixz commented Feb 10, 2015

Copy link
Copy Markdown
Contributor

This refactors the Mac installer generation to use Packages. This moves io.js away from using the deprecated Package Maker, and allows for easy localization of the Mac installer.

I've included a README with localization instructions, which should make it easy for localization teams to localize the installer. The only thing I couldn't test with this was the package signing, so that should be tested before merging. This might also help pave the way to fixing other Mac installer-related issues, since now it's using something a bit modern for generation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@remixz
Refactor Mac .pkg generation
97e79f5
This refactors the Mac .pkg generation to use Packages. This moves io.js
away from using the deprecated Package Maker, and allows for easy
localization of the installer.
@rvagg rvagg self-assigned this Feb 10, 2015
@rvagg

rvagg commented Feb 10, 2015

Copy link
Copy Markdown
Member

Amazing, this is a great contribution @remixz, I'll try and make time to test this out and give feedback.

Initial quesitons:

  1. is it possible to bring this on par to the Windows installer and make the node symlink an optional part of the install (opt-out)? We could perhaps do the same with npm so you could choose to not even install that, but that's arguably less useful.
  2. should we include the license as rtf? part of the Windows installer build process is to make an rtf version (see vcbuild.bat), is there any reason to do the same here?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz

remixz commented Feb 10, 2015

Copy link
Copy Markdown
Contributor Author
  1. Yes! I'll have to add a flag or something to disable the symlink in tools/install.py so the node symlink isn't included in the installer, but otherwise it's easy to do. It'll just be an empty install step with a post-install script. I can also make the installer show the "custom install" view by default, so it looks something like this:
    screen shot 2015-02-09 at 8 54 26 pm
    As well, with the PR as it is, npm can be unselected, but it's still installed with the io.js step. This actually does beg the question: why is there a separate step for npm? From what I can tell, it's not needed.
  2. We can add it as rtf, but there's no advantage I can think of, aside from changing the font.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz

remixz commented Feb 10, 2015

Copy link
Copy Markdown
Contributor Author

Idea: Should this include an uninstaller? Choosing the option would disable all the others, like this:

screen shot 2015-02-10 at 12 01 09 am

Then it can just run an arbitrary script that would remove files. This would help bring parity with other platforms.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost

ghost commented Feb 10, 2015

Copy link
Copy Markdown

I think an uninstall is important.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Feb 10, 2015

Copy link
Copy Markdown
Member

is it possible to bring this on par to the Windows installer and make the node symlink an optional part of the install (opt-out)?

I'm not sure if that makes sense on OS X. A symlink opt-out may give the false impression that joyent/node and io.js can coexist peacefully when they can't.

The installer installs to /usr/local. If a joyent/node install already exists there, io.js will happily stamp on it. The symlink is just the top of the ice berg.

The only scenario that I can think of where an opt-out would help is when joyent/node and io.js are installed to separate locations that are both on the PATH. The exact meaning of the "Create a symlink" checkbox would need to be called out in the installer if we don't want angry bug reports. :-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz

remixz commented Feb 11, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis A description can be added to the symlink installer entry, so when someone clicks on it to uncheck it, it can say something like "joyent/node and io.js don't coexist nicely, keeping this symlink is recommended".

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Feb 11, 2015

Copy link
Copy Markdown
Member

@remixz Sounds good.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Florian-R Florian-R mentioned this pull request Feb 11, 2015
please remove node symlink from iojs installer #796
Closed
remixz added 3 commits February 11, 2015 17:50
@remixz
Add symlink step to Mac installer
f186d1c
@remixz
Merge remote-tracking branch 'origin/v1.x' into refactor-mac-installer
8491b02
@remixz
Create empty files on pre-pkg, set up uninstall
e08d2df
@jessy1092 jessy1092 mentioned this pull request Feb 12, 2015
Translate installers for OS X and Windows #819
Closed
@tellnes

tellnes commented Feb 12, 2015

Copy link
Copy Markdown
Contributor

Could we reuse the translations for other distributions?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tellnes
tellnes reviewed Feb 12, 2015
View reviewed changes
Comment thread Makefile

@tellnes tellnes Feb 12, 2015

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should this be added to .PHONY?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@remixz remixz Feb 13, 2015

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@tellnes Good catch. Fixed, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@fhemberger

fhemberger commented Feb 12, 2015

Copy link
Copy Markdown
Contributor

I prepared the German translation for the installer using Packages: https://github.com/fhemberger/io.js/commit/65ba54ec0eb112ce040415ac2b3b321b8e38b5d1
As soon as this PR lands, we can use this commit as a basis for further internationalization.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz
Add pre-pkg to .PHONY in makefile
2fb6c21
@remixz

remixz commented Feb 13, 2015

Copy link
Copy Markdown
Contributor Author

@tellnes It's likely, but there'd have to be some changes so that it doesn't specifically reference Mac locations.

@fhemberger Awesome!! 👍 🎉 It might need to be rebased by the time this is merged, since the file will probably change a bit when the uninstaller is finalized, but this is great.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis
bnoordhuis reviewed Feb 13, 2015
View reviewed changes
Comment thread tools/install.py Outdated

@bnoordhuis bnoordhuis Feb 13, 2015

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

no_symlink = len(args) > 4 and args[4] == 'true'?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@fhemberger fhemberger mentioned this pull request Feb 13, 2015
Status of translation nodejs/nodejs-de#16
Closed
7 of 9 tasks
remixz added 3 commits February 13, 2015 15:48
@remixz
Wrap Makefile lines at 80 characters
5580022
@remixz
Wrap osx-pkg README at 80 characters
d1c2ac7
@remixz
Simplify no_symlink check in tools/install.py
1755cc8
@remixz

remixz commented Feb 13, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis Implemented all of your changes. Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Feb 13, 2015

Copy link
Copy Markdown
Member

LGTM but I'm not an expert when it comes to OS X installers.

@iojs/collaborators Can I have one more LGTM?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz

remixz commented Feb 13, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis Ah, I need to finish the uninstaller portion before this can ship. (unless you want to move that to another PR?)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz
Add uninstaller
1434328
@remixz

remixz commented Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

Cool. I've added the uninstaller, which means once reviewed, this should be good to go.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@evanlucas

evanlucas commented Feb 14, 2015

Copy link
Copy Markdown
Contributor

From running local tests, it looks like tools/osx-codesign.sh may need to be updated. It tries to explicitly codesign node instead of iojs

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@evanlucas

evanlucas commented Feb 14, 2015

Copy link
Copy Markdown
Contributor

Ah nevermind, got it to work. Is there a way to change the success screen when uninstall is selected? Currently, it is the same, which could cause some confusion.

This is what is shown after a successful uninstall:

screen shot 2015-02-13 at 7 43 26 pm

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@remixz
Change codesign to sign iojs, not node
5e2c01c
@remixz

remixz commented Feb 14, 2015

Copy link
Copy Markdown
Contributor Author

@evanlucas I changed tools/osx-codesign.sh to sign iojs, since even though you got it working, I'm pretty sure it should be signing iojs, unless someone has a reason for it not to.

For the success screen, I'm pretty sure it can't change based on what's installed, which definitely leaves a sucky experience for people doing an uninstall.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fishrock123 Fishrock123 added the install Issues and PRs related to installing Node.js and its installers. label Feb 15, 2015
@rvagg

rvagg commented Feb 16, 2015

Copy link
Copy Markdown
Member

i18n changes pending resolution of #819

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhemberger

fhemberger commented Mar 2, 2015

Copy link
Copy Markdown
Contributor

Is the updated installer ready to land?

As #819 seems to be resolved according to the TC meeting, I'd love to add the translation for the installer and make it a proper PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bnoordhuis

bnoordhuis commented Mar 2, 2015

Copy link
Copy Markdown
Member

Should /usr/local/lib/node_modules/ be removed?

I don't know if it's possible to implement this but what tools/install.py does is unlink the directory iff it's empty after removing all installed files.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

remixz added 2 commits March 4, 2015 15:58
@remixz
Merge remote-tracking branch 'origin/v1.x' into refactor-mac-installer
cb069d4
Conflicts:
	.gitignore
@remixz
Remove node_modules if folder is empty
2526e1d
@remixz

remixz commented Mar 5, 2015

Copy link
Copy Markdown
Contributor Author

@bnoordhuis Yup, that can be done! Added with 2526e1d.

@fhemberger If the translation issue is all resolved, then I think this is ready to land.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhemberger

fhemberger commented Mar 5, 2015

Copy link
Copy Markdown
Contributor

Thanks, I'll create a proper translation PR for further discussion as soon as this has landed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhemberger

fhemberger commented Mar 17, 2015

Copy link
Copy Markdown
Contributor

@remixz We should add a paragraph to introduction.rtf to clarify that node.js and io.js cannot be installed in parallel (and the symlink will overwrite the one of any previously installed node versions). If the user wants to use both, they have to use version managers like nvm or n and use them instead of the Mac installer (see discussion in #389).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rvagg rvagg mentioned this pull request Mar 18, 2015
Cherrypicked commits from joyent/node #1163
Closed
@chrisdickinson chrisdickinson added the land-on-master label Apr 17, 2015
@fhemberger

fhemberger commented May 24, 2015

Copy link
Copy Markdown
Contributor

Any news on the status of the new Mac installer? cc @nodejs/core

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Qard

Qard commented Jun 26, 2015

Copy link
Copy Markdown
Member

@nodejs/collaborators Anyone familiar with the existing OSX installer that can review this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rvagg

rvagg commented Jun 27, 2015

Copy link
Copy Markdown
Member

On my list but not a high priority compared to other things like node-gyp, so help from others would be greatly appreciated. @remixz if you have time would you mind rebasing on the current master, there's been a bunch of changes in this area that you'll need to catch up on.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@silverwind silverwind added the macos Issues and PRs related to the macOS platform. label Jul 13, 2015
@rvagg

rvagg commented Jul 28, 2015

Copy link
Copy Markdown
Member

@remixz would you mind having another look at this, rebase and target a new PR against next for now and we might try and do this at the same time as #2247. It's not a top priority but would be good to get this done before the converged release.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@evanlucas

evanlucas commented Aug 26, 2015

Copy link
Copy Markdown
Contributor

I think we should definitely try to get this merged before the converged release. Anything I can do to help?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhemberger

fhemberger commented Aug 26, 2015

Copy link
Copy Markdown
Contributor

The changes in this PR need to be merged against the current master for the 4.0.0 release, including the renaming from 'iojs' to 'node'. It would be great if we could get this running, so we can ship localized installers in the next major version.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fhemberger

fhemberger commented Aug 27, 2015

Copy link
Copy Markdown
Contributor

I did a rebase against master in #2571, can you all please have a look?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fishrock123 Fishrock123 mentioned this pull request Aug 28, 2015
Refactor mac installer (Rebase of #776 against master) #2571
Closed
@jasnell

jasnell commented Oct 22, 2015

Copy link
Copy Markdown
Member

Closing this in favor of #2571

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell jasnell closed this Oct 22, 2015
@rvagg rvagg mentioned this pull request Mar 11, 2016
Refactor mac installer (built on #2571) #5656
Closed
@rvagg rvagg mentioned this pull request Jun 27, 2016
Make uninstaller for Mac #7371
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@rvagg rvagg

Labels

install Issues and PRs related to installing Node.js and its installers. macos Issues and PRs related to the macOS platform.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@remixz @rvagg @bnoordhuis @tellnes @fhemberger @Fishrock123 @evanlucas @Qard @jasnell @chrisdickinson @silverwind
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.