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

doc: blorb about npm_config_devdir #1185

Merged
merged 1 commit into from
May 14, 2017
Merged

doc: blorb about npm_config_devdir #1185

merged 1 commit into from
May 14, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 27, 2017

Fixes: #1180

@gibfahn
Copy link
Member

gibfahn commented Apr 27, 2017

I'm pretty sure this works for any of the --option_name, you just set npm_config_option_name, so we can just state it generically.

Also maybe worth mentioning that if you do npm config set option_name val then it only works if you run node-gyp through npm, but that if you do export npm_config_option_name=val or set npm_config_option_name=val then it works everywhere.

@refack refack force-pushed the patch-2 branch 6 times, most recently from b5a7585 to a005142 Compare April 27, 2017 23:17
@refack
Copy link
Contributor Author

refack commented Apr 27, 2017

@gibfahn PTAL

README.md Outdated
They can be set in two ways:
1. via `npm config` - these work only when `node-gyp` is triggered by `npm`<br>
e.g. `$ npm config set npm_config_devdir /tmp/.gyp`
2. as environment variables - work also when `noge-gyp` is invoked diretly<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typos, noge-gyp->node-gyp and diretly->directly.

README.md Outdated
1. via `npm config` - these work only when `node-gyp` is triggered by `npm`<br>
e.g. `$ npm config set npm_config_devdir /tmp/.gyp`
2. as environment variables - work also when `noge-gyp` is invoked diretly<br>
e.g. `c:\> set npm_config_devdir=c:\temp\.gyp`
Copy link
Member

@gibfahn gibfahn Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an

$ export npm_config_devdir=/tmp/.gyp or
c:\> set npm_config_devdir=c:\temp\.gyp on Windows.

README.md Outdated
Configuration
--------

#### `node-gyp` responds to configuration settings:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/configuration settings/environment variables/? Also please insert a blank line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure either, but see if it works.

@refack
Copy link
Contributor Author

refack commented Apr 28, 2017

@gibfahn @bnoordhuis @richardlau fixed. PTAL

README.md Outdated
Configuration
--------

#### `node-gyp` responds to environment variables:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing Ben's requested newline after this

README.md Outdated
2. as environment variables - these work also when `node-gyp` is invoked
directly:<br>
`> set npm_config_devdir=c:\temp\.gyp` for Windows or<br>
`$ export npm_config_devdir=/tmp/.gyp` for unix like environments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few remarks:

  • is the #### at the top necessary?
  • are the <br> tags necessary?
  • maybe s/triggered/executed/? 'triggered' is somewhat unspecific.
  • s/for example/For example/
  • s/unix like/UNIX-like/
  • insert a line or two explaining dashes should be replaced with underscores

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the #### at the top necessary?
In the other suctions the sub-title is followed by a table. I thought here it should be delineated in some way

maybe s/triggered/executed/? 'triggered' is somewhat unspecific.

npm -> node-gyp is sort of a trigger... but I changed it anyway.

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more nits

README.md Outdated

#### `node-gyp` responds to environment variables:
Variables take the form `npm_config_OPTION_NAME` for any of the options
listed above.<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the <br>? If you want a line break just add two trailing spaces (or leave a blank line between for a proper paragraph break). In any case I think the next line can be part of this one.

README.md Outdated
listed above.<br>
They can be set in two ways:
1. as `npm` configuration - these work only when `node-gyp` is triggered by
`npm`. for example:<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<br>

README.md Outdated
`$ npm config set [--global] npm_config_devdir /tmp/.gyp`
2. as environment variables - these work also when `node-gyp` is invoked
directly:<br>
`> set npm_config_devdir=c:\temp\.gyp` for Windows or<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<br>

README.md Outdated
`npm`. for example:<br>
`$ npm config set [--global] npm_config_devdir /tmp/.gyp`
2. as environment variables - these work also when `node-gyp` is invoked
directly:<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<br>

README.md Outdated
2. as environment variables - these work also when `node-gyp` is invoked
directly:<br>
`> set npm_config_devdir=c:\temp\.gyp` for Windows or<br>
`$ export npm_config_devdir=/tmp/.gyp` for unix like environments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather just have this as the default and then have the Windows one for Windows, that way people don't have to know whether they're in a Unix like environment, they just use default unless they're on Windows.

$ export npm_config_devdir=/tmp/.gyp or > set npm_config_devdir=c:\temp\.gyp for Windows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to mix it up for windows Falks #629, we tend to suffer most from node-gyp issues

Copy link
Member

@gibfahn gibfahn Apr 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying that Windows is less important, just that I think it's clearer to have the Unix method as the default.

$ export npm_config_devdir=/tmp/.gyp or
> set npm_config_devdir=c:\temp\.gyp for Windows

If you're on a mac, you might not know that you're on a Unix-like environment. But you do know you're not on Windows, so you end up with the right command.

And FWIW any Unix user who wants to respect the $XDG_*_CONFIG spec will want to set this as well, which is one of the reasons this was actually implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.
(me 😭😭😭)

README.md Outdated

__`node-gyp` responds to environment variables:__
Variables take the form `npm_config_OPTION_NAME` for any of the options
listed above (dashes in option names should be replaced by an underscores).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an

README.md Outdated
They can be set in two ways:
1. as `npm` configuration - these work only when `node-gyp` is executed by
`npm`. For example:
`$ npm config set [--global] npm_config_devdir /tmp/.gyp`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t this be npm config set [--global] devdir /tmp/.gyp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!
Shame on us...🤦

@refack
Copy link
Contributor Author

refack commented Apr 28, 2017

@richardlau @gibfahn Had rephrase for @addaleax's correction. PTAL.

@gibfahn
Copy link
Member

gibfahn commented May 13, 2017

@bnoordhuis LGTY?

@gibfahn gibfahn merged commit c307b30 into nodejs:master May 14, 2017
@refack refack deleted the patch-2 branch May 14, 2017 16:24
@refack
Copy link
Contributor Author

refack commented May 14, 2017

Just FMI was that a "Squash Merge". It looks really good on master.

@addaleax
Copy link
Member

@refack there’s a lot of trailing whitespace here, btw… for next time :)

@refack
Copy link
Contributor Author

refack commented May 14, 2017

@refack there’s a lot of trailing whitespace here, btw… for next time :)

Ack. I got used to my IDE trimming them, but it doesn't for markdown because they might be semantic.
That said all the trainings are intentional for formating, except for the one in line 199

This was referenced Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants