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: allow separate component install w/ env var #5734

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 16, 2016

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.

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

/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).

@thefourtheye thefourtheye added the tools Issues and PRs related to the tools directory. label Mar 16, 2016
else:
node_files(action)
header_files(action)
if 'true' == variables.get('node_install_headers'): npm_files(action)
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Member Author

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

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

@bnoordhuis
Copy link
Member

Options are mutually exclusive, run install.py multiple times to install multiple components.

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)

@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2016

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?

@bnoordhuis
Copy link
Member

os.environ.get('NODE_INSTALL_HEADERS', True) - is that "default to True?

Yes.

What values of NODE_INSTALL_HEADERS will evaluate to true vs false in this case?

Empty strings are false, everything else is truthy.

@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2016

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)' '/' 

?

@thefourtheye
Copy link
Contributor

@rvagg Yes, and I don't like that 😢. Instead of saying, install only headers, we are saying don't install node and npm.

@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2016

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/

@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2016

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

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 rvagg force-pushed the install-separate branch from 06f560d to edea788 Compare March 16, 2016 10:50
@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2016

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

@thefourtheye
Copy link
Contributor

How about

tools/install.py --install node,npm,headers
tools/install.py --uninstall node,npm,headers

@rvagg
Copy link
Member Author

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:

  • argv[1]: 'install' or 'uninstall' (cmd)
  • argv[2]: prefix for destination (DESTDIR)
  • argv[3]: prefix within destination (PREFIX)

I reckon there's >0 users in the wild that are relying on this script outside of make.

@rvagg
Copy link
Member Author

rvagg commented Mar 16, 2016

we could just add --components node,npm,headers that overrides the default of everything and works for both install and uninstall.

@thefourtheye
Copy link
Contributor

we could just add --components node,npm,headers ...

yeah, that SGTM.

@rvagg
Copy link
Member Author

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

@rvagg @bnoordhuis I created #5741. PTAL

@rvagg
Copy link
Member Author

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.

@rvagg rvagg closed this Mar 16, 2016
@rvagg rvagg deleted the install-separate branch March 16, 2016 23:21
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Apr 20, 2016
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".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants