| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
Hmmm, not sure if this is correct. I mean, node_install_headers corresponds to npm_files?
Sorry, something went wrong.
There was a problem hiding this comment.
It should probably be node_install_npm.
Sorry, something went wrong.
There was a problem hiding this comment.
yeah, not sure how that happened, fixing
Sorry, something went wrong.
|
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 0 if you don't want that component installed, or the variable name has "DONT" in it or similar), I just think that the logic involved in that gets more awkward and it's simpler to say that an env var being set is a signal to do something rather than not do something. 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. |
Sorry, something went wrong.
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) |
Sorry, something went wrong.
|
os.environ.get('NODE_INSTALL_HEADERS', True) - is that "default to True"? What values of NODE_INSTALL_HEADERS will evaluate to True vs False in this case? |
Sorry, something went wrong.
Yes.
Empty strings are false, everything else is truthy. |
Sorry, something went wrong.
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)' '/' ? |
Sorry, something went wrong.
|
@rvagg Yes, and I don't like that 😢. Instead of saying, install only headers, we are saying don't install node and npm. |
Sorry, something went wrong.
|
Another option: $(PYTHON) tools/install.py install-headers And: $(PYTHON) tools/install.py install-headers install-node /path/to/node/ $(PYTHON) tools/install.py install-npm /path/to/npm/ |
Sorry, something went wrong.
|
Or brevity: $(PYTHON) tools/install.py headers And: $(PYTHON) tools/install.py headers node /path/to/node/ $(PYTHON) tools/install.py npm /path/to/npm/ Where the default, everything, is: $(PYTHON) tools/install.py install |
Sorry, something went wrong.
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: $(PYTHON) tools/install.py install --include-node --include-npm --include-headers And an inverse: $(PYTHON) tools/install.py install --no-include-node --no-include-npm --no-include-headers but tbh, they all kind of smell |
Sorry, something went wrong.
|
How about tools/install.py --install node,npm,headers tools/install.py --uninstall node,npm,headers |
Sorry, something went wrong.
|
@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 make. |
Sorry, something went wrong.
|
we could just add --components node,npm,headers that overrides the default of everything and works for both install and uninstall. |
Sorry, something went wrong.
yeah, that SGTM. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@rvagg @bnoordhuis I created #5741. PTAL |
Sorry, something went wrong.
|
#5741 is now my preferred option, closing in favour of that one but discussion is still open around all of this. |
Sorry, something went wrong.
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".
| Back | FazBrowse Home | New Git URL |
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.
extras (dtrace, systemtap, gdbinit, man page).
compiling native addons, previously HEADERS_ONLY, used for creating
the headers tarball for distribution.
symlink.
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_ONLY to NODE_INSTALL_HEADERS_ONLY although 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).