-
Notifications
You must be signed in to change notification settings - Fork 46
os: add method tmpdir()
#569
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
Conversation
It implements the `os` method: - https://nodejs.org/dist/latest-v12.x/docs/api/os.html#os_os_tmpdir
|
Note: this is a prepared PR for adding the |
| } | ||
|
|
||
| exports.tmpdir = function tmpdir() { | ||
| var path = process.env.TMPDIR || |
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.
why not get env from getenv(3)
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.
What's the benefits in that way? Actually we did set the env at GetEnvironArray via environ(7).
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.
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.
getenv is only used if node environment has not been setup.
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.
why not read directly from process.env like Windows?
because of thesetuid root?
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.
See nodejs/node#18511 for all related history, this seems a historic design problem that allows packages sharing via process.env(write/read), but anyway, we have to handle this security, thanks @lolBig
|
Restart the failed job(https://travis-ci.com/yodaos-project/ShadowNode/jobs/246598500) caused by networks. |
|
|
||
| assert.deepStrictEqual(actual, expected); | ||
|
|
||
| // test for tmpdir |
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.
A default case shall be covered. i.e. no TMPDIR nor TMP and TEMP environs are set.
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.
Ok, BTW I just copied these tests from here: https://github.com/nodejs/node/blob/master/test/parallel/test-os.js#L46-L78.
| * `scopeid` {Number} The numeric IPv6 scope ID (only specified when family is IPv6) | ||
| * `cidr` {String} The assigned IPv4 or IPv6 address with the routing prefix in CIDR notation. If the netmask is invalid, this property is set to null | ||
|
|
||
| ```js |
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.
Is this a necessary change?
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.
Yea, the below lines actually are not in JavaScript.
Implements the Node.js
osAPI: https://nodejs.org/dist/latest-v12.x/docs/api/os.html#os_os_tmpdirChecklist
npm testpasses