Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.4k
tools: allow separate component install w/ env var#5734
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
tools/install.py Outdated
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.
Hmmm, not sure if this is correct. I mean, node_install_headers corresponds to npm_files?
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.
It should probably be node_install_npm.
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.
yeah, not sure how that happened, fixing
rvagg commented Mar 16, 2016
I'm also happy to consider that (1) environment variables for this are kind of gross and perhaps we should be using command-line arguments and/or (2) that they should be opt-out rather than opt-in (e.g. set a variable to This is my proposal as is but I'd like to make sure it smells OK too, so speak up if your nose is twitching. |
bnoordhuis commented Mar 16, 2016
Is that intentional or just how it turned out? You can approach it like this: ifos.environ.get('NODE_INSTALL_HEADERS', True): header_files(action) ifos.environ.get('NODE_INSTALL_NODE', True): node_files(action) ifos.environ.get('NODE_INSTALL_NPM', True): npm_files(action) |
rvagg commented Mar 16, 2016
|
bnoordhuis commented Mar 16, 2016
Yes.
Empty strings are false, everything else is truthy. |
rvagg commented Mar 16, 2016
So using that logic, this: NODE_INSTALL_HEADERS_ONLY=1 $(PYTHON) tools/install.py install '$(TARNAME)' '/' would become: NODE_INSTALL_NODE="" NODE_INSTALL_NPM="" $(PYTHON) tools/install.py install '$(TARNAME)' '/' ? |
thefourtheye commented Mar 16, 2016
@rvagg Yes, and I don't like that 😢. Instead of saying, install only headers, we are saying don't install node and npm. |
rvagg commented Mar 16, 2016
Another option: And: |
rvagg commented Mar 16, 2016
Or brevity: And: Where the default, everything, is: |
Primary use cases are: headers tarball (previously using `HEADERS_ONLY`) and OS X installer so it has npm files separate from core node + header files. * set `NODE_INSTALL_NODE_ONLY` for core node executable and associated extras (dtrace, systemtap, gdbinit, man page). * set `NODE_INSTALL_HEADERS_ONLY` for header files as required for compiling native addons, previously `HEADERS_ONLY`, used for creating the headers tarball for distribution. * set `NODE_INSTALL_NPM_ONLY` to install npm only, including executable symlink. If none of these are set, install everything. Options are mutually exclusive, run install.py multiple times to install multiple components.
rvagg commented Mar 16, 2016
Although, "uninstall" is a problem in that case. Perhaps: And an inverse: but tbh, they all kind of smell |
thefourtheye commented Mar 16, 2016
How about |
rvagg commented Mar 16, 2016
@thefourtheye yeah, that's not bad, but it's a big departure from the existing API which uses fixed arguments:
I reckon there's >0 users in the wild that are relying on this script outside of |
rvagg commented Mar 16, 2016
we could just add |
thefourtheye commented Mar 16, 2016
yeah, that SGTM. |
rvagg commented Mar 16, 2016
If someone with better Python chops wants to code some of that up for me it'd be appreciated, otherwise I'll muddle through it when I have the time. |
thefourtheye commented Mar 16, 2016
@rvagg@bnoordhuis I created #5741. PTAL |
rvagg commented Mar 16, 2016
#5741 is now my preferred option, closing in favour of that one but discussion is still open around all of this. |
Refer: nodejs#5734 This introduces a command line option, ('-c' or '--components') to install components optionally. The valid components are * node * npm * headers All these components can be installed or uninstalled, like this python tools/install.py -c node,headers install . / python tools/install.py --components npm uninstall . / "-c" is just the short form of "--components".
Primary use cases are: headers tarball (previously using
HEADERS_ONLY)and OS X installer so it has npm files separate from core node + header
files. This is a component of #5656, a rework of the OS X installer, which now
has a working checkbox to optionally install npm.
NODE_INSTALL_NODE_ONLYfor core node executable and associatedextras (dtrace, systemtap, gdbinit, man page).
NODE_INSTALL_HEADERS_ONLYfor header files as required forcompiling native addons, previously
HEADERS_ONLY, used for creatingthe headers tarball for distribution.
NODE_INSTALL_NPM_ONLYto install npm only, including executablesymlink.
If none of these are set, install everything.
Options are mutually exclusive, run install.py multiple times to install
multiple components.
/cc @nodejs/build @fhemberger
I'm also open to making this a semver-major change because of the change from
HEADERS_ONLYtoNODE_INSTALL_HEADERS_ONLYalthough I don't imagine anyone's actually using that in the wild (it's new and also pretty obscure for use outside of the Makefile).