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

Refactor Mac installer generation #776

Closed
wants to merge 12 commits into from

Conversation

remixz
Copy link
Contributor

@remixz remixz commented Feb 10, 2015

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.

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
Copy link
Member

rvagg commented Feb 10, 2015

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?

@remixz
Copy link
Contributor Author

remixz commented Feb 10, 2015

  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.

@remixz
Copy link
Contributor Author

remixz commented Feb 10, 2015

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.

@ghost
Copy link

ghost commented Feb 10, 2015

I think an uninstall is important.

@bnoordhuis
Copy link
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. :-)

@remixz
Copy link
Contributor Author

remixz commented Feb 11, 2015

@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".

@bnoordhuis
Copy link
Member

@remixz Sounds good.

@tellnes
Copy link
Contributor

tellnes commented Feb 12, 2015

Could we reuse the translations for other distributions?

@@ -252,16 +253,26 @@ release-only:
exit 1 ; \
fi

pre-pkg:
Copy link
Contributor

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?

Copy link
Contributor Author

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!

@fhemberger
Copy link
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.

@remixz
Copy link
Contributor Author

remixz commented Feb 13, 2015

@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.

@@ -194,6 +195,8 @@ def run(args):
conf = load_config()
variables = conf['variables']
target_defaults = conf['target_defaults']
# argv[4] is a variable representing whether a symlink should be made
no_symlink = True if len(args) > 4 and args[4] == 'true' else False
Copy link
Member

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'?

@remixz
Copy link
Contributor Author

remixz commented Feb 13, 2015

@bnoordhuis Implemented all of your changes. Thanks!

@bnoordhuis
Copy link
Member

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

@iojs/collaborators Can I have one more LGTM?

@remixz
Copy link
Contributor Author

remixz commented Feb 13, 2015

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

@remixz
Copy link
Contributor Author

remixz commented Feb 14, 2015

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

@evanlucas
Copy link
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

@evanlucas
Copy link
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

@remixz
Copy link
Contributor Author

remixz commented Feb 14, 2015

@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.

@Fishrock123 Fishrock123 added the install Issues and PRs related to the installers. label Feb 15, 2015
@rvagg
Copy link
Member

rvagg commented Feb 16, 2015

i18n changes pending resolution of #819

@fhemberger
Copy link
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.

@bnoordhuis
Copy link
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.

@remixz
Copy link
Contributor Author

remixz commented Mar 5, 2015

@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.

@fhemberger
Copy link
Contributor

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

@fhemberger
Copy link
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).

@fhemberger
Copy link
Contributor

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

@Qard
Copy link
Member

Qard commented Jun 26, 2015

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

@rvagg
Copy link
Member

rvagg commented Jun 27, 2015

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.

@silverwind silverwind added the macos Issues and PRs related to the macOS platform / OSX. label Jul 13, 2015
@rvagg
Copy link
Member

rvagg commented Jul 28, 2015

@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.

@evanlucas
Copy link
Contributor

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

@fhemberger
Copy link
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.

@fhemberger
Copy link
Contributor

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

@jasnell
Copy link
Member

jasnell commented Oct 22, 2015

Closing this in favor of #2571

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
install Issues and PRs related to the installers. macos Issues and PRs related to the macOS platform / OSX.
Projects
None yet
Development

Successfully merging this pull request may close these issues.