GetTempDir / ScopedTempDir on Windows doesn't use %TEMP% var on some bots |
|||||
Issue descriptionSee https://chromium-review.googlesource.com/c/chromium/src/+/1148946 for a test case (example failing log is https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8940086701074429744/+/steps/base_unittests__with_patch__on_Windows-10-15063/0/stdout) On some of the try bots, TEMP (and TMP, etc) is set to "C:\b\swarming\w\ir\tmp\t" but the GetTempDir function returns "C:\\Users\\CHROME~1\\AppData\\Local\\Temp". Result is that ScopedTempDir, which is used in a lot of tests and calls GetTempDir internally, doesn't put the temp dirs under %TEMP%. This makes bot administration harder because %TEMP% is cleaned regularly. GetTempDir is implemented with ::GetTempPath and according to https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-gettemppatha: 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.
,
Jul 25
I think it's worthwhile having the test emit the value of TMP. The behavior in the log could be explained by another test in the shard setting TMP.
,
Jul 25
Updated the test to print both TMP and USERPROFILE.
,
Jul 25
Confirmed: https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8940019099680390416/+/steps/base_unittests__with_patch__on_Windows-10-15063/0/stdout shows ../../base/files/scoped_temp_dir_unittest.cc(151): error: Expected equality of these values: tmp_var Which is: "C:\\Users\\CHROME~1\\AppData\\Local\\Temp" temp_var Which is: "C:\\b\\s\\w\\it054_kl" Do you think it would make sense to change ScopedTempDir to return %TEMP% if it is set and is an existing, writeable dir, and fall back to GetTempDir if not? (That would still have problems if a test overwrites TEMP and doesn't put it back though.)
,
Jul 25
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-py.git/+/be6678547fd1db1a05f9f66135dad643eb1e1617 commit be6678547fd1db1a05f9f66135dad643eb1e1617 Author: Marc-Antoine Ruel <maruel@chromium.org> Date: Wed Jul 25 18:34:36 2018 [client] Overhaul temporary directory override in run_isolated Include the rationale for each environment, because I'll have all forgotten this in one week. R=qyearsley@chromium.org Bug: 867102 Change-Id: I5f6b5f824e7bd6225a182a6c075742c560d60060 Reviewed-on: https://chromium-review.googlesource.com/1150422 Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org> [modify] https://crrev.com/be6678547fd1db1a05f9f66135dad643eb1e1617/client/run_isolated.py
,
Jul 26
Re comment 4: I don't think we should vary from what the platform does. I can envision some code somewhere that calls the platform function directly rather than our wrapper -- it's easier to understand if they do the same thing. Hopefully it's a non-issue with the commit in comment 5.
,
Aug 23
,
Aug 31
maruel: this is resolved now, correct? Would you please either mark this as fixed, or provide more info about what work is needed? Thanks.
,
Aug 31
There's still a sad MAC_CHROMIUM_TMPDIR hack, but other than that it's fixed. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mar...@chromium.org
, Jul 25