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

tools: add preinstall script for npm on OS X #3986

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Make sure we cleanly remove npm before installing on OS X

Related: #3606

This is working for me locally, but would be great to have some others test.

@Fishrock123
Copy link
Contributor

Notes: I couldn't reproduce the original anymore, so I don't know how we can confirm this will actually work?

@Fishrock123 Fishrock123 added the install Issues and PRs related to the installers. label Nov 23, 2015
@evanlucas
Copy link
Contributor Author

Good point. I'll see if I can spin up a vm and try to reproduce

@mscdex mscdex added the macos Issues and PRs related to the macOS platform / OSX. label Nov 23, 2015
# TODO Can this be done inside the .pmdoc?
# TODO Can we extract $PREFIX from the installer?
cd /usr/local/bin
rm -rf ../lib/node_modules/npm
Copy link
Member

Choose a reason for hiding this comment

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

why are we doing a cd followed by rm and not rm with absolute path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, good question. That is how the postinstall script is done. And now that I look again, it was done that way for a reason. Updating now

Copy link
Member

Choose a reason for hiding this comment

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

maybe we also need a set -e if we're doing multiple lines, if the cd fails then the rm will still execute

Make sure we cleanly remove npm before installing on OS X
@evanlucas
Copy link
Contributor Author

Ok, updated with absolute path and set -e

@rvagg
Copy link
Member

rvagg commented Dec 8, 2015

test build of this created @ https://nodejs.org/download/test/v6.0.0-test201512088ac2af7628/ if anyone else wants to review the changes

@rvagg
Copy link
Member

rvagg commented Dec 8, 2015

I touched a bunch of TEST files under /usr/local/lib before running the installer and they still exist:

$ node -v
v6.0.0-test201512088ac2af7628
$ find /usr/local -name TEST
/usr/local/lib/node_modules/npm/node_modules/TEST
/usr/local/lib/node_modules/npm/TEST
/usr/local/lib/node_modules/TEST
/usr/local/lib/TEST

So something's not right with this as far as I can tell.

@rvagg
Copy link
Member

rvagg commented Dec 8, 2015

Perhaps it needs to be "preupgrade" as well as "preinstall". Reading here http://s.sudre.free.fr/Stuff/PackageMaker_Howto.html but I don't know how authoritative this is.

@evanlucas
Copy link
Contributor Author

@rvagg Ok yes it looks like neither the preinstall, nor the postinstall scripts were running. What is weird to me is that the local.pkg installs npm, so there really is no need to have an npm.pkg. Even if one deselects the npm choice, npm will still be installed. I can make it a single package and then the scripts work properly. I guess my question becomes, is it worth it? With how outdated PackageMaker.app is, I really think that we should try to get #2571 landed. Is there anything blocking that?

@Fishrock123
Copy link
Contributor

@evanlucas I think the other issue mostly needs review.

@evanlucas
Copy link
Contributor Author

Closing. Hopefully we can get the new OS X pkg stuff in soon. It should replace this anyways

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.

4 participants