tools: allow separate component install w/ env var#5734
tools: allow separate component install w/ env var#5734rvagg wants to merge 1 commit intonodejs:masterfrom
Conversation
tools/install.py
Outdated
There was a problem hiding this comment.
Hmmm, not sure if this is correct. I mean, node_install_headers corresponds to npm_files?
There was a problem hiding this comment.
It should probably be node_install_npm.
There was a problem hiding this comment.
yeah, not sure how that happened, fixing
|
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. |
Is that intentional or just how it turned out? You can approach it like this: if os.environ.get('NODE_INSTALL_HEADERS', True): header_files(action)
if os.environ.get('NODE_INSTALL_NODE', True): node_files(action)
if os.environ.get('NODE_INSTALL_NPM', True): npm_files(action) |
|
|
Yes.
Empty strings are false, everything else is truthy. |
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)' '/' ? |
|
@rvagg Yes, and I don't like that 😢. Instead of saying, install only headers, we are saying don't install node and npm. |
|
Another option: And: |
|
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.
|
Although, "uninstall" is a problem in that case. Perhaps: And an inverse: but tbh, they all kind of smell |
|
How about |
|
@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 |
|
we could just add |
yeah, that SGTM. |
|
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. |
|
@rvagg @bnoordhuis I created #5741. PTAL |
|
#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).