os: add homedir() by cjihrig · Pull Request #1791 · nodejs/node · GitHub

Do not land this yet.

This PR adds os.homedir(), which calls libuv's uv_os_homedir() to retrieve the current user's home directory. This is an alternative to #1670. This PR currently consists of two commits. 6887116 is the libuv update, which has not landed in io.js yet. This commit should be removed once a proper update is in place. e7e5992 is the io.js update.

node.h defines PATH_MAX on Windows so the #ifdef is probably not necessary here.

I'm not sure if this is a good assumption. I'd just check that it still returns something, possibly something that looks like a path.

You're right. Updated.

LGTM. You should probably run the CI before landing this.

I will once libuv is updated and I can remove the floating patch.

lgtm, the duplication in test-os.js seems unnecessary but I guess it's consistent with what's in there

Libuv 1.6.0 has landed. 👍

At this point, buf would be NULL only.

Floating patch removed. CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/766/

@rvagg the CI is still running, but I'm seeing some strange failures. Unable to compile on SmartOS, out of memory errors on FreeBSD, and too many files open on CentOS. Anything odd going on with the build?

@cjihrig eeek, not that I'm aware of, @jbergstroem has been tinkering on CentOS5 but not the others.

I'll have a look.

Affected os:es seems to hit file-max

freebsd: kern.maxfiles limit exceeded by uid 1003, please see tuning(7).
centos5: VFS: file-max limit 334442 reached

Jenkins has <100 file descriptors open which leads me to believe that ccache might try to. I can't say I've seen it previously, but I'll keep digging.

Looking at cat /proc/sys/fs/file-nr (or equivalents) at different machines; can't say I see it grow even close to limits while compiling.

(for reference, the run I'm measuring is here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/767/)

So yeah, I'm unable to reproduce what happened at that specific run. Jenkins.

Added limits.h in https://github.com/cjihrig/io.js/commit/69c1c33348ac1803615c73e42bf7dfef3155a4a7#diff-2b7b259d26f297c87b7d71512e33ccecR14 to get the compile working on SmartOS. The CentOS failure seemed to be flakey. FreeBSD is still throwing ENOMEM. @bnoordhuis any clue why this might be happening (other than the obvious answer that the process is out of memory)?

Updated CI run: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/769/

I speculate it's because of this. :-)

(IIRC, I pointed that out in the review, that sysconf() can return < 0, but I'm not 100% sure. At any rate, bufsize is a size_t so the -1 wraps around and becomes SIZE_MAX.)

(EDIT: That may have been another PR, come to think of it.)

@bnoordhuis you did mention that. The data type for bufsize not being switched from size_t to long was my bad (I did add the check for bufsize <= 0, but didn't change the data type). Assuming that this is the problem, I'm curious why this didn't cause a problem with the libuv CI. And, do you think we should ditch the sysconf() call and use something else as the starting point for malloc()?

EDIT: this has a good example that calls sysconf() and falls back to a different value if it fails, although they also use a size_t to hold the return value :-)

@bnoordhuis you were right, so I added a37072b as a floating patch on libuv, and the CI is much happier now. Let me know how you want this handled in libuv.

@cjihrig Left a comment. Can you file that as a libuv PR? We'll do a patch release for that.

Ah, should've tested the branch while searching for issues; I just assumed it was lingering processes or similar.

@bnoordhuis if I update the floating patch here, is this OK to land?

EDIT: Removed the floating patch since the libuv update PR was opened.

Refs: nodejs#1791

@cjihrig FYI: #1905

PR-URL: #1905 Refs: #1791 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Thanks @saghul. Hopefully one last CI run: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/774/

Hmmm.. freebsd isn't so happy. https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/774/nodes=freebsd101-64/tapTestReport/

os.homedir() calls libuv's uv_os_homedir() to retrieve the current user's home directory. PR-URL: #1791 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

It would help if I rebased the branch to include the libuv 1.6.1 update.

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/775/ is all green.

Landed in 6e78e5f.

Notable Changes: * libuv: Upgraded to 1.6.0 and 1.6.1, see full ChangeLog for details. (Saúl Ibarra Corretgé) #1905 #1889. Highlights include: - Fix TTY becoming blocked on OS X - Fix UDP send callbacks to not to be synchronous - Add uv_os_homedir() (exposed as os.homedir(), see below) * npm: See full release notes for details. (Kat Marchán) #1899. Highlight: - Use GIT_SSH_COMMAND (available as of Git 2.3) * openssl: - Upgrade to 1.0.2b and 1.0.2c, introduces DHE man-in-the-middle protection (Logjam) and fixes malformed ECParameters causing infinite loop (CVE-2015-1788). See the security advisory for full details. (Shigeki Ohtsu) #1950 #1958 - Support FIPS mode of OpenSSL, see README for instructions. (Fedor Indutny) #1890 * os: Add os.homedir() method. (Colin Ihrig) #1791 * smalloc: Deprecate whole module. (Vladimir Kurchatkin) #1822 * Add new collaborators: - Alex Kocharin (@rlidwka) - Christopher Monsanto (@monsanto) - Ali Ijaz Sheikh (@ofrobots) - Oleg Elifantiev (@Olegas) - Domenic Denicola (@domenic) - Rich Trott (@Trott)

I made a polyfill for anyone wanting to use this while still supporting older node/io versions:
https://github.com/sindresorhus/os-homedir

silverwind added os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels May 25, 2015

bnoordhuis reviewed May 25, 2015
View reviewed changes

Fishrock123 mentioned this pull request May 25, 2015

os: add .homedir() #1670

Closed

bnoordhuis reviewed May 25, 2015
View reviewed changes

Fishrock123 added the libuv Issues and PRs related to the libuv dependency or the uv binding. label May 26, 2015

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

thefourtheye reviewed Jun 4, 2015
View reviewed changes

cjihrig mentioned this pull request Jun 4, 2015

unix: handle invalid _SC_GETPW_R_SIZE_MAX values libuv/libuv#389

Closed

saghul added a commit to saghul/node that referenced this pull request Jun 5, 2015

deps: update libuv to version 1.6.1

d3b69c5

Refs: nodejs#1791

saghul mentioned this pull request Jun 5, 2015

deps: update libuv to version 1.6.1 #1905

Closed

bnoordhuis pushed a commit that referenced this pull request Jun 5, 2015

deps: update libuv to version 1.6.1

a5bd466

PR-URL: #1905 Refs: #1791 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

os: add homedir()

6e78e5f

os.homedir() calls libuv's uv_os_homedir() to retrieve the current user's home directory. PR-URL: #1791 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

cjihrig merged commit 6e78e5f into nodejs:master Jun 6, 2015

cjihrig deleted the homedir branch June 6, 2015 04:45

rvagg mentioned this pull request Jun 11, 2015

Release proposal: 2.3.0 #1939

Closed

nfischer mentioned this pull request Jan 19, 2016

feat: add tilde expansion to expand() shelljs/shelljs#293

Merged

renkun-ken mentioned this pull request Jan 10, 2020

Improve session watcher initialization REditorSupport/vscode-R#184

Merged

os: add homedir() by cjihrig · Pull Request #1791 · nodejs/node · GitHub

Uh oh!

Uh oh!

os: add homedir() - #1791

os: add homedir()#1791cjihrig merged 1 commit intonodejs:masternodejs/node:masterfrom cjihrig:homedirCopy head branch name to clipboard

Conversation

cjihrig commented May 25, 2015

Uh oh!

bnoordhuis May 25, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis May 25, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig May 25, 2015

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented May 25, 2015

Uh oh!

cjihrig commented May 25, 2015

Uh oh!

rvagg commented May 25, 2015

Uh oh!

Fishrock123 commented Jun 4, 2015

Uh oh!

thefourtheye Jun 4, 2015

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Jun 4, 2015

Uh oh!

cjihrig commented Jun 4, 2015

Uh oh!

rvagg commented Jun 4, 2015

Uh oh!

jbergstroem commented Jun 4, 2015

Uh oh!

jbergstroem commented Jun 4, 2015

Uh oh!

jbergstroem commented Jun 4, 2015

Uh oh!

jbergstroem commented Jun 4, 2015

Uh oh!

jbergstroem commented Jun 4, 2015

Uh oh!

cjihrig commented Jun 4, 2015

Uh oh!

bnoordhuis commented Jun 4, 2015

Uh oh!

cjihrig commented Jun 4, 2015

Uh oh!

cjihrig commented Jun 4, 2015

Uh oh!

bnoordhuis commented Jun 4, 2015

Uh oh!

jbergstroem commented Jun 4, 2015

Uh oh!

cjihrig commented Jun 5, 2015

Uh oh!

saghul commented Jun 5, 2015

Uh oh!

cjihrig commented Jun 5, 2015

Uh oh!

Fishrock123 commented Jun 5, 2015

Uh oh!

cjihrig commented Jun 6, 2015

Uh oh!

sindresorhus commented Jun 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

os: add homedir()#1791
cjihrig merged 1 commit into
nodejs:masterfrom
cjihrig:homedir