os: reorder get temp environment variable for windows

[chouzz]

According to windows document, the temp direcotry should check $TMP
first, after that, check $TEMP, or node will get different temp
directory compared with c/c++ program if user have different path
for $TMP and $TEMP in windows.

Refs: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha

[Trott]

Welcome, @chouzz, and thanks for the pull request. Assuming this doesn't cause any existing tests to fail, we'll want to add a test for this, probably in test/parallel/test-os.js.

[tniessen]

cc @nodejs/platform-windows

[chouzz]

Welcome, @chouzz, and thanks for the pull request. Assuming this doesn't cause any existing tests to fail, we'll want to add a test for this, probably in test/parallel/test-os.js.

Hi, it's my first contributions. Fortunately，it doesn't affect existing tests. I will add test for this.

[chouzz]

Hi, it's my first contributions. Fortunately，it doesn't affect existing tests. I will add test for this.

It should cause current os.tmpdir() tests failed, but seems github action didn't check this! I modified the test case for the this PR.

[RaisinTen]

test/parallel/test-os.js actually failed during the windows CI run before it got updated - https://github.com/nodejs/node/runs/5513561147?check_suite_focus=true#step:7:8135 but the GitHub UI doesn't make that obvious unfortunately. Good that it passes now.

not ok 1977 parallel/test-os
  ---
  duration_ms: 0.185
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:123
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    + actual - expected
    
    + '/tmp'
    - '/temp'
         ^
        at Object.<anonymous> (D:\a\node\node\test\parallel\test-os.js:46:10)
        at Module._compile (node:internal/modules/cjs/loader:1099:14)
        at Object.Module._extensions..js (node:internal/modules/cjs/loader:1153:10)
        at Module.load (node:internal/modules/cjs/loader:975:32)
        at Function.Module._load (node:internal/modules/cjs/loader:822:12)
        at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
        at node:internal/main/run_main_module:17:47 {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: '/tmp',
      expected: '/temp',
      operator: 'strictEqual'
    }
    
    Node.js v18.0.0-pre
  ...

Labelling this as a semver-major since this is breaking. Not sure if it's worth the breakage though because other modules might already be depending on this behaviour.

[richardlau]

GitHub actions don't currently run tests on Windows as the tests were not consistently passing. All the coverage jobs (including on Jenkins) ignore test failures -- I don't recall why.

[chouzz]

test/parallel/test-os.js actually failed during the windows CI run before it got updated - https://github.com/nodejs/node/runs/5513561147?check_suite_focus=true#step:7:8135 but the GitHub UI doesn't make that obvious unfortunately. Good that it passes now.

Thanks for clarifying.

because other modules might already be depending on this behaviour.

@RaisinTen Could you please clearfiy which modules depend on this? As I can see, some modules may depending on this, also may not depending on this.

As for me, this is a bug. I have a server program written by c/c++ will create temp file in temp directory, and client base on nodejs will get the file, in this case, I will get "can't find temp file" error. Becase server side create temp file in $TMP, but client get temp file in $TEMP. The problem only happens if user have different paths for $TMP and $TEMP.

On the other hand, $TMP and $TEMP are same path by default(%USERPROFILE%\AppData\Local\Temp) in windows. They may changed by:

• The user himself
• Some softwares. (Some tools can move folders to other disk from C: and change the $TMP environment to save C drive's space)

[RaisinTen]

Could you please clearfiy which modules depend on this?

I don't know of a specific module name but someone just like you might have already written a program that does the same exact thing, except it has been written by keeping in mind that os.tmpdir() might take process.env.TEMP into account first. So this change would break their code.

I have a server program written by c/c++ will create temp file in temp directory, and client base on nodejs will get the file, in this case, I will get "can't find temp file" error. Becase server side create temp file in $TMP, but client get temp file in $TEMP. The problem only happens if user have different paths for $TMP and $TEMP.

Did you consider passing the absolute file paths from the server to the client? That way, the client would be able to use the right file paths, regardless of the value of the environment variables.

As for me, this is a bug.

I disagree. Programs are free to choose any sequence they desire. See https://devblogs.microsoft.com/oldnewthing/20150417-00/?p=44213

the old DISKCOPY and EDIT programs would look for TEMP before looking for TMP.
Windows went through a similar exercise, but for whatever reason, the original authors of the Get­Temp­File­Name function chose to look for TMP before looking for TEMP.
The result of all this is that the directory used for temporary files by any particular program is at the discretion of that program

I also noticed that Python uses a different sequence - https://docs.python.org/3/library/tempfile.html#tempfile.gettempdir

On Windows, the directories C:\TEMP, C:\TMP, \TEMP, and \TMP, in that order.

[chouzz]

I also noticed that Python uses a different sequence

I saw nodejs have same logic for $TMP and $TEMP 10 years ago. And it changed 7 years ago, then it try to get $TEMP before $TMP in windows.

Programs are free to choose any sequence they desire

OK. I also see the wiki said in DOS:

Operating system tools typically only use %TEMP%, whereas third-party programs also use %TMP%. Typically %TEMP% takes precedence over %TMP%.

https://en.wikipedia.org/wiki/Environment_variable

But seems all languages based on win32 api will get $TMP first. So the question is that do we need to change this order? Should node be consistent with them？

[targos]

The order was changed in nodejs/node-v0.x-archive#5083

[chouzz]

The order was changed in nodejs/node-v0.x-archive#5083

returns "/tmp" on unix (always?) and GetTempPath on windows?
http://msdn.microsoft.com/en-us/library/windows/desktop/aa364992%28v=vs.85%29.aspx

The GetTempPath function checks for the existence of environment variables in the following order and uses the first path found:
The path specified by the TMP environment variable.
The path specified by the TEMP environment variable.
The path specified by the USERPROFILE environment variable.
The Windows directory.
Note that the function does not verify that the path exists, nor does it test to see if the current process has any kind of access rights to the path. The GetTempPath function returns the properly formatted string that specifies the fully qualified path based on the environment variable search order as previously specified. The application should verify the existence of the path and adequate access rights to the path prior to any use for file I/O operations

The issue had linked to windows GetTempPath function. But seems the auther of the PR think $TEMP should come first.

Also, usually windows users set their temporary folder with variable TEMP, not TMP.
TMP was used only for old dos command line apps.
So, TEMP must come first.

[chouzz]

Feel free to close the PR if you think it's not suitable. @RaisinTen @targos

My program can solve this problem easily.

[RaisinTen]

Feel free to close the PR if you think it's not suitable. @RaisinTen @targos

Yea, since programs are not mandated to follow a specific sequence, I think the current behaviour is alright and since you mentioned that the problem this PR was trying to solve can already be solved by your program, there's no reason to keep this PR open anymore, so I'm closing it. Thank you for the PR regardless!

My program can solve this problem easily.

Good to know. :)