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

Follow XDG OS conventions for storing data #1570

Merged
merged 2 commits into from Oct 20, 2018

Conversation

@Siilwyn
Copy link
Contributor

commented Oct 13, 2018

  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Closes #175 and #1124. Besides changes to follow the XDG standard I took the liberty to:

  • Fix a small current linter issue.
  • Replace the osenv dependency with native os to decrease the amount of dependencies.
  • Add a package lock.

Since os.userInfo() requires Node.js 6 or higher and the default data location is changed this would be a major version bump. I did not go for moving the old locations or falling back because it adds unneeded complexity not worth the trouble.

if (prog.devDir) {
prog.devDir = prog.devDir.replace(/^~/, homeDir)
} else if (homeDir) {
prog.devDir = path.resolve(homeDir, '.node-gyp')
prog.devDir = envPaths('node-gyp', { suffix: '' }).cache

This comment has been minimized.

Copy link
@Siilwyn

Siilwyn Oct 13, 2018

Author Contributor

The suffix is 'nodejs' by default, another option is: envPaths('gyp') which will result in gyp-nodejs.
I opted for .cache because I think it fits the type of data more than .data since it is data being cached to prevent unneeded requests.

@refack

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

Could you take out package-lock.json and rebase?

@refack refack self-assigned this Oct 14, 2018
@Siilwyn

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

@refack yes, consider it done. 👍

"request": "^2.87.0",
"rimraf": "2",
"semver": "~5.3.0",
"tar": "^3.1.3",
"which": "1"
},
"engines": {
"node": ">= 4.0.0"
"node": ">= 6.0.0"

This comment has been minimized.

Copy link
@refack

refack Oct 16, 2018

Member

This is tricky. We need more buy-in from other contributors.
@nodejs/node-gyp

This comment has been minimized.

Copy link
@richardlau

richardlau Oct 16, 2018

Member

So far there have been no objections to dropping support for Node.js <6 for node-gyp v4 (#1519). I also suggested dropping support for Node.js 6 which will be EOL in April.

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

consider it done. 👍

@Siilwyn thank you.
We are setting up out CI cluster to test this on macOS. Once we test this we could land it.

@Siilwyn

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Good to hear, looking forward to the merge. :)

@refack

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

CI: https://ci.nodejs.org/job/nodegyp-test-pull-request/98/
(Had to add macOS as a tested platform)

@refack
refack approved these changes Oct 20, 2018
@refack

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

After running CI I found this:

bash-3.2$ pwd
/Users/iojs/Library/Caches/node-gyp
bash-3.2$ ls -la
total 0
drwxr-xr-x  4 iojs  iojs  136 Oct 20 16:32 .
drwxr-xr-x  3 iojs  iojs  102 Oct 20 16:15 ..
drwxr-xr-x  4 iojs  iojs  136 Oct 20 16:15 10.12.0
drwxr-xr-x  4 iojs  iojs  136 Oct 20 16:32 8.12.0
bash-3.2$ pwd
/Users/iojs
bash-3.2$ ls .node-gyp
ls: .node-gyp: No such file or directory

(version 6 was tested on a different worker)

Siilwyn added 2 commits Oct 13, 2018
Breaking change: needs Node.js version 6 or higher

#1570
Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #1570
Fixes: #175
Fixes: #1124
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack merged commit 2b5ce52 into nodejs:master Oct 20, 2018
@Siilwyn Siilwyn deleted the Siilwyn:follow-os-standards branch Oct 21, 2018
rvagg added a commit that referenced this pull request Apr 24, 2019
Breaking change: needs Node.js version 6 or higher

#1570
Reviewed-By: Refael Ackermann <refack@gmail.com>
rvagg added a commit that referenced this pull request Apr 24, 2019
PR-URL: #1570
Fixes: #175
Fixes: #1124
Reviewed-By: Refael Ackermann <refack@gmail.com>
@rvagg rvagg referenced this pull request Apr 24, 2019
@joaocgreis joaocgreis referenced this pull request May 29, 2019
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.