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

On macOS, os.tmpdir() is not a real path #11422

Closed
novemberborn opened this issue Feb 16, 2017 · 22 comments
Closed

On macOS, os.tmpdir() is not a real path #11422

novemberborn opened this issue Feb 16, 2017 · 22 comments
Labels
os Issues and PRs related to the os subsystem. question Issues that look for answers.

Comments

@novemberborn
Copy link

  • v7.4.0:
  • macOS Sierra 10.12.3:
  • os:

os.tmpdir() returns a path like /var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T. Per

node/lib/os.js

Line 43 in 00c86cc

path = process.env.TMPDIR ||
that's the same path as the $TMPDIR environment variable. However on macOS this is a symlink. The real path is /private/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T:

❯ node -pe 'os.tmpdir()'
/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T
❯ node -pe 'fs.realpathSync(os.tmpdir())'
/private/var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T

On the face of it the current behavior seems correct, and it's just macOS that is annoying. On the other hand it makes for surprising failures when writing cross-platform code. See sindresorhus/temp-write#8 for example.

At the very least I think this pitfall should be documented (happy to open a PR), but that just means users need to always wrap os.tmpdir() with fs.realpathSync(). Should os.tmpdir() do this by itself? On macOS only, or all platforms?

(Note that this is different from #7545. When your shell is in a directory like /var/folders/_6/p8qxp_3n62zg9081tvb0lcc80000gn/T then process.env.PWD is correctly set to that path.)

@sindresorhus
Copy link

I think os.tmpdir() should consistently return the resolved path on all platforms. You would never want the symlink path.

sindresorhus added a commit to sindresorhus/temp-write that referenced this issue Feb 16, 2017
To work around: nodejs/node#11422

Fixes #8

I went with resolving the path synchronously and lazily once. It's done sync in the async method too, but only once, so I think that's an ok compromise.
@sam-github
Copy link
Contributor

Don't you think

proces.env.TEMP = '/some/path';
assert.equal(os.tmpdir(), process.env.TEMP)

should be an invariant? You would request that we don't return the string that the user/OS has set the temp dir string to, but instead return some other path we derive from it? What if TEMP points a symlink, and they change the symlink, won't an app that called os.tmpdir() once before the change be mysteriously stuck to the old path and people will report the opposite bug, that we didn't respect TEMP?

I think setting TEMP to a symlink may be surprising in itself, but node unilaterally deciding the user didin't really mean what they did might be even more surprising.

node/lib/os.js

Lines 34 to 52 in a196895

exports.tmpdir = function() {
var path;
if (isWindows) {
path = process.env.TEMP ||
process.env.TMP ||
(process.env.SystemRoot || process.env.windir) + '\\temp';
if (path.length > 1 && path.endsWith('\\') && !path.endsWith(':\\'))
path = path.slice(0, -1);
} else {
path = process.env.TMPDIR ||
process.env.TMP ||
process.env.TEMP ||
'/tmp';
if (path.length > 1 && path.endsWith('/'))
path = path.slice(0, -1);
}
return path;
};
is the code in question.

@mscdex mscdex added os Issues and PRs related to the os subsystem. question Issues that look for answers. labels Feb 16, 2017
@sindresorhus
Copy link

Don't you think ... should be an invariant?

No, because that's an implementation detail, and it's already a non-invariant—Your example would fail if the user had TMPDIR set to a different directory on macOS, because it's checked before TEMP. If users want to get something that matches exactly what they set, they should get it from process.env directly.

sindresorhus added a commit to sindresorhus/temp-write that referenced this issue Feb 17, 2017
* Resolve the path to the temp directory

To work around: nodejs/node#11422

Fixes #8

I went with resolving the path synchronously and lazily once. It's done sync in the async method too, but only once, so I think that's an ok compromise.

* Add a test
@bnoordhuis
Copy link
Member

One reason why I think this is unlikely to happen is that it would mean changing os.tmpdir() to take a callback (because silently sync operations in core? No.)

It's either that or resolve and cache at startup, but then changes to process.env.TMPDIR won't be visible. Neither option is very palatable.

@novemberborn
Copy link
Author

One reason why I think this is unlikely to happen is that it would mean changing os.tmpdir() to take a callback

I don't think a os.tmpdirSync() is particular user friendly either.

It's either that or resolve and cache at startup, but then changes to process.env.TMPDIR won't be visible.

The implementation of os.tmpdir() (which environment variables it uses, and in what order) isn't documented though. It just says:

The os.tmpdir() method returns a string specifying the operating system's default directory for temporary files.

It should probably be treated as a breaking change, sure, but returning a real path may make the os.tmpdir() function align better with user expectations. (It would with mine, but that's just a sample of 1.)

If that's not a viable direction we should at least document that it may not return a real path, and that this is a known issue on macOS.

@bnoordhuis
Copy link
Member

I did a quick survey and both python's tempfile.gettempdir() and ruby's Dir.tmpdir return the unresolved path, i.e., the symlink. Node isn't the outlier.

The implementation of os.tmpdir() (which environment variables it uses, and in what order) isn't documented though.

The environment variables and the order they're tried in is fairly standard for UNIX and Windows programs. I'd be okay with having that documented.

@Trott
Copy link
Member

Trott commented Jul 26, 2017

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Jul 26, 2017
@novemberborn
Copy link
Author

@Trott the documentation around this could be approved. Might be worth keeping the ticket around for that. I just haven't gotten around to it.

@Trott
Copy link
Member

Trott commented Jul 27, 2017

@Trott the documentation around this could be approved. Might be worth keeping the ticket around for that. I just haven't gotten around to it.

I guess I wouldn't personally object to a doc update, but I don't know about keeping an issue open about it as it seems superfluous too. My reasoning is:

  • It seems like the expectation here is unusual. @bnoordhuis notes that Ruby and Python behave the same way. I just checked and so does Perl. I'm unaware of any language that resolves the symlink when determining tmpdir, but correction is welcome.

  • Since this behavior seems universal and not peculiar to Node.js, and no other languages document the symlink issue, I'm not sure Node.js needs the extra information in its docs. Excessive minutiae may degrade the docs rather than improve them.

That being stated, as I said before, I personally wouldn't object to adding this information. Just not sure it needs to be an open item and honestly I'm not sure others wouldn't object.

@fabiospampinato
Copy link

fabiospampinato commented Feb 19, 2020

Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree.

Can it be re-opened?

P.s. Closing issues because they are arbitrarily old doesn't sound like a good idea to me, if the metric to optimize for is the number of issues open one may as well close the issue tracker.

@Trott
Copy link
Member

Trott commented Feb 20, 2020

Can it be re-opened?

Are you asking that it be re-opened pending a doc change? Or pending a change in the behavior of os.tmpdir()?

P.s. Closing issues because they are arbitrarily old doesn't sound like a good idea to me,

I agree. Fortunately, that's not what happened.

It's not that the issue was old. It's that the issue was inactive. meaning that no one was commenting, no one was working on a PR to address it, and the case for doing nothing seems compelling.

if the metric to optimize for is the number of issues open one may as well close the issue tracker.

The thing to optimize for is to have the contents of the issue tracker be useful and relevant. This means not having a lot of "open" issues for things that aren't going to be addressed. This seemed like it fell into that category.

I guess two options that haven't been suggested (or that I've missed, in which case, hey, sorry):

  • Create a new function with the proposed behavior. If we change the behavior here, it will mean making os.tmpdir() asynchronous and it will break everyone's existing usage of os.tmpdir(). So people will need to rewrite their code for the new behavior whether we stick it in a new function or we make a breaking change in os.tmpdir(). The user-friendly thing to do, IMO, would be to leave os.tmpdir() alone and create a new function (or add an option to os.tmpdir() that makes the behavior change opt-in). People need to rewrite their code either way, but you don't break people's existing code. They migrate when they want/need to. I'm not convinced this is a great idea, but if we do want to introduce this functionality, I suspect this or something very much like it would be the way to go.

  • Create an npm module that has this new behavior. If the npm module gets very widespread usage, then that's a data point that we should at least consider it in core.

On balance, I'm in favor of doing nothing or maybe updating the docs. I think I'll go open a PR to update the docs now and see what other people think.....

@Trott Trott reopened this Feb 20, 2020
@sam-github
Copy link
Contributor

The issue tracker is useful as a list of things there is a reasonable expectation that someone will work on ("work" being discuss, PR a fix for, investigate, etc.).

Its not useful to keep a list of things that people don't like or find surprising about Node.js, but that no one (since July 2017!) has considered important enough to actually do something about. That list would be very long indeed.

@Trott
Copy link
Member

Trott commented Feb 20, 2020

#31877

@fabiospampinato
Copy link

Are you asking that it be re-opened pending a doc change? Or pending a change in the behavior of os.tmpdir()?

I'm in favor of the proposed behavior change, but if for some reason that can't be done I'd appreciate having the docs reflect this little discrepancy.

It's not that the issue was old. It's that the issue was inactive. meaning that no one was commenting, no one was working on a PR to address it, and the case for doing nothing seems compelling.

Fair enough, but when having lots of open issues it's expected that some of them may not be worked/commented on for a while.

The thing to optimize for is to have the contents of the issue tracker be useful and relevant.

I think a better approach would be to tag these potential abandoned issues with a specific tag and filter that tag out when browsing the open issues, this way people finding this issue could still considering adding their "👍" on it or commenting, signaling that it's something they care about, which they are probably not going to do if the issue is closed, and having issues like this still in the issue tracker shouldn't be a problem as they can be filtered out.

If we change the behavior here, it will mean making os.tmpdir() synchronous and it will break everyone's existing usage of os.tmpdir()

I don't understand what's the problem here, isn't os.tmpdir already synchronous? If you meant a-synchronous why would it have to become asynchronous?

Create an npm module that has this new behavior. If the npm module gets very widespread usage, then that's a data point that we should at least consider it in core.

temp-dir may be that module, it seems fairly popular with ~1.9M weekly downloads and 220k dependents on GitHub.

Its not useful to keep a list of things that people don't like or find surprising about Node.js

I agree, if a decision was made that those things won't be changed. For this specific issue though it seems that there was a decision to make about what to do about it, none has been made and the issue was closed. You may argue closing the issue was actually the decision made but that was just a temporary action as the issue is now open again and nothing changed.

but that no one (since July 2017!) has considered important enough to actually do something about.

This issue is about a very minor detail, but I'd argue if there's an issue being tracked and a decision on what to do about it hasn't been taken yet (not even a "we won't change this") for X years that doesn't mean it shouldn't be tracked anymore.

Also this issue has been closed in 2017, perhaps that also discouraged people from commenting on it these past years.

Thanks for reopening 👍

@Trott
Copy link
Member

Trott commented Feb 20, 2020

isn't os.tmpdir already synchronous?

I indeed meant "asynchronous". Sorry about the error. I have edited the comment to be correct.

If you meant a-synchronous why would it have to become asynchronous?

I based that on @bnoordhuis's comment above.

@fabiospampinato
Copy link

fabiospampinato commented Feb 20, 2020

because silently sync operations in core? No.

@Trott @bnoordhuis and why not? It would have to be performed under macOS only, and unless the value of process.env.TMPDIR changes, which I see no reasons why it would in pretty much every case, it will only have to be performed once.

I'd personally rather do this than pull a PHP and introduce os.realtmpdir.

It doesn't seem too much of a performance burden to me, and popular third-party libraries like temp-dir are already doing this anyway, even under Windows and Linux when it's not needed, given the choice I'd rather have this smoothed over my Node itself.

@Trott
Copy link
Member

Trott commented Feb 20, 2020

@fabiosantoscode It seems like a premise in the case you're making is that Node.js should do more than Ruby, Python, Perl, PHP, Java, etc. currently do to smooth out cross-platform differences like this. Is that correct? If so, why is that? (I'm not saying we should or shouldn't. I want to know what your thinking is, though, since you have definite opinions here and I want to understand as fully as I can. Is it that those other languages should do more too? Or is Node.js unique?)

@fabiospampinato
Copy link

@Trott I don't care what those other languages are doing because I don't use them, I suppose though that if changing this is Node is for the better other platforms should implement the change too, obviously.

I want to know what your thinking is, though, since you have definite opinions here and I want to understand as fully as I can

I'm just sharing my opinion regarding a very marginal issue in order to potentially improve a bit a platform I care about, I don't know enough to have definitive opinions on almost anything.

Basically this feels inconsistent to me, as a Node user who expects Node to smooth over little differences like this across OS', and it would break things under some scenarios, scenarios that I've never personally encountered though, like: #31877 (comment).

If for whatever reason this can't be changed I'd argue this and other similar quirks/behaviors/cross-platform differences should be documented, as stumbling on them even after having read the docs is not great.

Just to mention another "quirk" I know of that should be documented too: non-bigint ino numbers reported under Windows are basically trash because they can overflow the bits JS allocates for regular numbers and different files can have the same ino number reported.

Other than this I don't think I have anything useful to add to the discussion.

@fabiosantoscode
Copy link
Contributor

This has nothing to do with me, but since I've been mentioned so many times I'll chip in :)

I believe making node code more cross platform by default is important. It's a major selling point for this runtime.

As someone who writes libraries on npm for multiple platforms (the most experimental ones just for Linux and Mac) I find it important that the platform layer of this runtime attempts to make things more uniform across platforms. The reason is, that I'm using my own platform, like most people are. Testing on other platforms is cumbersome (even with a CI, given that testing on windows is considerably slower on Travis for example) and I believe I shouldn't have to do it.

Node provides a few platform interfaces in the form of functions and objects, which are really nice to use and the community has picked them up and made really cool things with them. It's my belief that these should work uniformly across platforms, and when this is not possible, it should be documented.

@Trott
Copy link
Member

Trott commented Feb 21, 2020

This has nothing to do with me, but since I've been mentioned so many times I'll chip in :)

Whoops, sorry about that. the mis-mention was my doing. I need to type more carefully in this issue tracker, clearly.

@fabiosantoscode
Copy link
Contributor

No problem :)

@jasnell
Copy link
Member

jasnell commented Jun 19, 2020

Closing this as there's been no further activity on it. It can be re-opened again if necessary

@jasnell jasnell closed this as completed Jun 19, 2020
SamuelMarks added a commit to SamuelMarks/cpython that referenced this issue Jan 25, 2024
On macOS this turns `/tmp` into `/private/<actual directory>`; see this issue from Node.js for someone bringing up the identical problem - nodejs/node#11422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os Issues and PRs related to the os subsystem. question Issues that look for answers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants