GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: install components optionally by thefourtheye · Pull Request #5741 · nodejs/node

Conversation

@thefourtheye

@thefourtheye thefourtheye commented Mar 16, 2016

Copy link
Copy Markdown
Contributor

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

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

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

cc @rvagg @bnoordhuis

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thefourtheye thefourtheye added the tools Issues and PRs related to the tools directory. label Mar 16, 2016
@thefourtheye thefourtheye mentioned this pull request Mar 16, 2016
tools: allow separate component install w/ env var #5734
Closed
@jasnell

jasnell commented Mar 16, 2016

Copy link
Copy Markdown
Member

Seems like a good change, LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rvagg
rvagg reviewed Mar 16, 2016
View reviewed changes
Comment thread tools/install.py Outdated

@rvagg rvagg Mar 16, 2016

Copy link
Copy Markdown
Member

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.

This should probably be without "npm" since that's added later when you set "node_install_npm" in the config.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@rvagg

rvagg commented Mar 16, 2016

Copy link
Copy Markdown
Member

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 HEADERS_ONLY env var.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 17, 2016

Copy link
Copy Markdown
Member

+1 to semver-major just to be on the safe side.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg rvagg added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 17, 2016
@rvagg

rvagg commented Mar 17, 2016

Copy link
Copy Markdown
Member

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.
fhemberger reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye

thefourtheye commented Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

Thanks @rvagg :-) I used most of the code from your PR only :-) I removed npm from the default set of components, PTAL.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rvagg
rvagg reviewed Mar 17, 2016
View reviewed changes
Comment thread tools/install.py Outdated

@rvagg rvagg Mar 17, 2016

Copy link
Copy Markdown
Member

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.

while I like extra newlines, I don't think we're doing that in this file, so this needs single newlines between function declarations

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@thefourtheye thefourtheye Mar 17, 2016

Copy link
Copy Markdown
Contributor Author

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.

@rvagg And its done now :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@thefourtheye

thefourtheye commented Mar 19, 2016

Copy link
Copy Markdown
Contributor Author

@bnoordhuis We use optparse now, to be consistent with other scripts and addressed your comments. PTAL.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Mar 19, 2016
View reviewed changes
Comment thread tools/install.py Outdated

@bnoordhuis bnoordhuis Mar 19, 2016

Copy link
Copy Markdown
Member

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.

Typo: separated

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bnoordhuis

bnoordhuis commented Mar 19, 2016

Copy link
Copy Markdown
Member

LGTM with nit.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye
thefourtheye force-pushed the install-seperate-components branch from dab2d5f to 16f5095 Compare March 20, 2016 07:12
@thefourtheye

thefourtheye commented Mar 20, 2016

Copy link
Copy Markdown
Contributor Author

Okay, addressed ben's comments, rebased and force pushed.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Mar 20, 2016

Copy link
Copy Markdown
Member

If all you did was fix the nit, then rubber-stamp LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member

@rvagg ... have you had the opportunity to test this one yet?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg

rvagg commented Mar 22, 2016

Copy link
Copy Markdown
Member

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@thefourtheye
tools: install components optionally
eb0bb29 
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
thefourtheye force-pushed the install-seperate-components branch from 16f5095 to eb0bb29 Compare April 20, 2016 05:55
@thefourtheye

thefourtheye commented Apr 20, 2016

Copy link
Copy Markdown
Contributor Author

Rebased!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@estliberitas
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
@thefourtheye

thefourtheye commented Apr 26, 2016

Copy link
Copy Markdown
Contributor Author

@rvagg ping!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@rvagg rvagg mentioned this pull request Jun 8, 2016
Refactor mac installer (built on #2571) #5656
Closed
@Trott
Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09
@rvagg
rvagg force-pushed the master branch from 5df0ca4 to c133999 Compare October 18, 2016 16:45
@rvagg
rvagg force-pushed the master branch from c133999 to 83c7a88 Compare October 18, 2016 17:01
@thefourtheye

thefourtheye commented Nov 6, 2016

Copy link
Copy Markdown
Contributor Author

Bump
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins
MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00
@jasnell

jasnell commented Feb 28, 2017

Copy link
Copy Markdown
Member

@thefourtheye @rvagg ... is this still something we should pursue?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. label Mar 1, 2017
@thefourtheye

thefourtheye commented Mar 1, 2017

Copy link
Copy Markdown
Contributor Author

I would have @rvagg take a call.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@fhinkel

fhinkel commented Mar 26, 2017

Copy link
Copy Markdown
Contributor

ping @rvagg
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@refack
refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11
@Trott

Trott commented Aug 13, 2017

Copy link
Copy Markdown
Member

This seems to need:

  • confirmation from @rvagg that we still want it?
  • a rebase
  • perhaps a quick look to make sure it works just fine if we start bundling npx
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Aug 24, 2017

Copy link
Copy Markdown
Member

Closing due to lack of forward progress on this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Aug 24, 2017
@thefourtheye
thefourtheye deleted the install-seperate-components branch August 25, 2017 03:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs manually marked as stalled and scheduled for automatic closure. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@thefourtheye @jasnell @rvagg @bnoordhuis @fhinkel @Trott
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.