tools: install components optionally #5741
Conversation
|
Seems like a good change, LGTM |
tools/install.py
Outdated
| global node_prefix, install_path, target_defaults, variables | ||
| components = set(['node', 'npm', 'headers']) |
There was a problem hiding this comment.
This should probably be without "npm" since that's added later when you set "node_install_npm" in the config.
There was a problem hiding this comment.
@rvagg Ah, you are correct. Currently also, when we install node and headers, we install npm only if 'node_install_npm' is set. Good catch. I'll fix it.
|
Solid work @thefourtheye, much better than if I'd been left to do this! I'll close #5734 in favour of this but the question of whether this is semver-major is still open as it removes the |
|
+1 to semver-major just to be on the safe side. |
rvagg
added
the
semver-major
|
k, made it so, it means the new installer won't land till v6 but that's probably good anyway. /cc @fhemberger @thefourtheye I just have that one item above, aside from that this lgtm but I'll also test it against my installer PR to verify before you land it. |
|
Thanks @rvagg :-) I used most of the code from your PR only :-) I removed |
tools/install.py
Outdated
| for component in components: | ||
| funcs[component](action) | ||
|
|
||
|
|
There was a problem hiding this comment.
while I like extra newlines, I don't think we're doing that in this file, so this needs single newlines between function declarations
There was a problem hiding this comment.
Oops sorry. Actually Python style guide recommends two line gaps between functions. As the indentation is important in Python, the two line gaps make it easier to read. I'll change it, so that it will be consistent throughout the file.
There was a problem hiding this comment.
@rvagg And its done now :-)
|
@bnoordhuis We use |
tools/install.py
Outdated
| usage = 'usage: %prog [-c components] COMMAND DESTDIR PREFIX' | ||
| parser = optparse.OptionParser(usage) | ||
| parser.add_option('-c', '--components', dest='components', | ||
| help='Comma seperated list of components. Valid values ' |
|
LGTM with nit. |
thefourtheye
force-pushed
the
install-seperate-components
branch
from
dab2d5f to
16f5095
Compare
|
Okay, addressed ben's comments, rebased and force pushed. |
|
If all you did was fix the nit, then rubber-stamp LGTM. |
|
@rvagg ... have you had the opportunity to test this one yet? |
|
no, and it's not an urgent change because it's only needed for the osx installer upgrade, I'll get to it but perhaps not this week |
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".
thefourtheye
force-pushed
the
install-seperate-components
branch
from
16f5095 to
eb0bb29
Compare
|
Rebased! |
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4 to
c7066fb
Compare
|
@rvagg ping! |
|
Bump |
|
@thefourtheye @rvagg ... is this still something we should pursue? |
|
I would have @rvagg take a call. |
|
ping @rvagg |
|
This seems to need:
|
|
Closing due to lack of forward progress on this. |
Pull Request check-list
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
Affected core subsystem(s)
tools
Description of change
Refer: #5734
This introduces a command line option, ('-c' or '--components') to
install components optionally. The valid components are
All these components can be installed or uninstalled, like this
"-c" is just the short form of "--components"
cc @rvagg @bnoordhuis