Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713293002:
Allow $TMPDIR to set temporary directory on OS X. (Closed)
DescriptionAllow $TMPDIR to set temporary directory on OS X.
This allows the continuous integration builds to be more hermetic by
supplying a managed value for $TMPDIR and having chromium + tests respect
it.
NSTemporaryDirectory takes its value directly from a non-overridable
confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X
more in line with Windows and Linux which both respect an environment
variable ($TEMP and $TMPDIR respectively).
BUG=696803
Review-Url: https://codereview.chromium.org/2713293002
Cr-Original-Commit-Position: refs/heads/master@{#453835}
Committed: https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9ba7e68750ee9a
Review-Url: https://codereview.chromium.org/2713293002
Cr-Commit-Position: refs/heads/master@{#453873}
Committed: https://chromium.googlesource.com/chromium/src/+/95fcf47ca01054ffe38e8609525a6ca8579b7824
Patch Set 1 #
Total comments: 1
Patch Set 2 #Patch Set 3 #Patch Set 4 : more fixes #Patch Set 5 : missing include #
Total comments: 8
Patch Set 6 : format #
Total comments: 1
Patch Set 7 : Fix comment #
Total comments: 4
Patch Set 8 : lowercase and filesystemRepresentation #Patch Set 9 : Fix typo #
Messages
Total messages: 56 (26 generated)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2713293002/diff/1/chrome/browser/file_select_... File chrome/browser/file_select_helper_mac.mm (right): https://codereview.chromium.org/2713293002/diff/1/chrome/browser/file_select_... chrome/browser/file_select_helper_mac.mm:21: // should be zipped to. Returns an empty path on any errors. This comment seems... misleading at best? `path` is not consulted in the old code at all.
The CQ bit was unchecked by iannucci@chromium.org
lambroslambrou@chromium.org changed reviewers: + lambroslambrou@chromium.org
remoting/ lgtm
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
iannucci@chromium.org changed reviewers: + jam@chromium.org, thakis@chromium.org
PTAL Please let me know if this is not, in fact, a good idea, or if there's some other way I should go about this :) Thanks! https://codereview.chromium.org/2713293002/diff/80001/chrome/browser/file_sel... File chrome/browser/file_select_helper_mac.mm (right): https://codereview.chromium.org/2713293002/diff/80001/chrome/browser/file_sel... chrome/browser/file_select_helper_mac.mm:22: // should be zipped to. Returns an empty path on any errors. Mentioned in patchset 1, but this comment is wildly misleading (in the old code as well as the new code). Not sure how to improve it though, as I'm not familiar with this code.
+maruel, vadimsh for FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
maruel@chromium.org changed reviewers: + maruel@chromium.org
That's great, it'll help a lot with unintentional accumulation of junk on the bots. I'm personally not concerned about users because if TMPDIR is set, this is normal expectation for the environment value to be used in general. non-owner lgtm
thakis@chromium.org changed reviewers: + mark@chromium.org - thakis@chromium.org
This feels like a mark@ CL, so adding him and moving myself to cc. https://cs.chromium.org/search/?q=nstemporarydirectory+package:%5Echromium$&t... https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:30: // Then fallback to the immutable NSTemporaryDirectory value. nit: "fallback" is a noun, "fall back" is the verb
Keyboard shortcut fail, sent to soon. Also wanted to say: What's the goal in being "more hermetic"? This doesn't cover several calls to NSGetTemporaryDirectory() in chrome code (https://cs.chromium.org/search/?q=nstemporarydirectory+package:%5Echromium$&t...) and lots of system libraries probably use that to create files there too.
thakis@chromium.org changed reviewers: + thakis@chromium.org
(or, asked another way, is there some tracking bug that this CL's BUG= line should point to?)
Description was changed from ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG= ========== to ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG= ==========
thakis@chromium.org changed reviewers: - thakis@chromium.org
On 2017/02/27 01:17:57, Nico wrote: > (or, asked another way, is there some tracking bug that this CL's BUG= line > should point to?) Probably :) I'll write one tomorrow and update this CL. Re other calls: The NSTemporaryDirectory calls I didn't change are in iOS (where the tempdir-ness doesn't really matter). Though it looks like I might have missed a non iOS test which I should fix too (I'll get it tomorrow). I could change the iOS ones too, but I felt like I should minimize the change.
I haven’t considered the merits yet, but I wanted to mention: TMPDIR is always set on macOS. Is that what you expect?
On 2017/02/27 03:31:13, Mark Mentovai wrote: > I haven’t considered the merits yet, but I wanted to mention: TMPDIR is always > set on macOS. Is that what you expect? Yes, by default $TMPDIR starts with the value of NSTemporaryDirectory() (but changing has no effect on programs that use NSTemporaryDirectory()).
On 2017/02/27 01:17:57, Nico wrote: > (or, asked another way, is there some tracking bug that this CL's BUG= line > should point to?) https://github.com/luci/luci-py/issues/157 was the initial issue. The goal is to make sure *unintentional* accumulation of temporary files is taken care of in an automated fashion. This is done by having a per-task temporary directory. After a task, the temporary directory is deleted; https://github.com/luci/luci-py/blob/master/client/run_isolated.py#L549 and if it fails, that's a very strong signal that the task had a serious issue and we leverage this information to mark the task as broken. This works on all platforms except OSX because of NSTemporaryDirectory(). AFAIK the directory gets cleaned up on reboots but having it per task, even if it is only for cooperating tasks (e.g. chrome unit tests), is even better.
what do you want me to look at?
Description was changed from ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG= ========== to ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 ==========
@jam: I think mark@ can cover OWNERS @mark: I've added a bug (crbug.com/696803), but what maruel@ is correct.
Ping :). Any additional thoughts on this?
https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:26: // In order to facilitate hermetic runs on OS X, first check $TMPDIR “first check” makes it sound like TMPDIR might not be set, but it usually will be set to the NSTemporaryDirectory. The comment should say so, otherwise it’s kind of misleading. https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:27: NSProcessInfo* procInfo = [NSProcessInfo processInfo]; Meh. Just do #include <stdlib.h> const char* env_tmpdir = getenv("TMPDIR"); if (env_tmpdir) { *path = base::FilePath(env_tmpdir); return true; } because that’s what NSProcessInfo is going to do under the hood anyway, and it’s way less work to just do the getenv() compared to creating all sorts of objects and allocating all sorts of memory and doing all sorts of string conversions when the thing we really want is just the raw const char* to stick into a FilePath’s std::string. https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:30: // Then fallback to the immutable NSTemporaryDirectory value. On 2017/02/27 01:16:20, Nico wrote: > nit: "fallback" is a noun, "fall back" is the verb Fix this :)
PTAL https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:26: // In order to facilitate hermetic runs on OS X, first check $TMPDIR On 2017/02/28 21:59:21, Mark Mentovai wrote: > “first check” makes it sound like TMPDIR might not be set, but it usually will > be set to the NSTemporaryDirectory. The comment should say so, otherwise it’s > kind of misleading. Actually now that I've thought about it a bit more... I'm guessing that it's /NEVER/ set on iOS. Would it be worth an ifdef? https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:27: NSProcessInfo* procInfo = [NSProcessInfo processInfo]; On 2017/02/28 21:59:20, Mark Mentovai wrote: > Meh. Just do > > #include <stdlib.h> > > const char* env_tmpdir = getenv("TMPDIR"); > if (env_tmpdir) { > *path = base::FilePath(env_tmpdir); > return true; > } > > because that’s what NSProcessInfo is going to do under the hood anyway, and it’s > way less work to just do the getenv() compared to creating all sorts of objects > and allocating all sorts of memory and doing all sorts of string conversions > when the thing we really want is just the raw const char* to stick into a > FilePath’s std::string. That was actually my first instinct, but I wasn't sure what was preferred. https://codereview.chromium.org/2713293002/diff/80001/base/files/file_util_ma... base/files/file_util_mac.mm:30: // Then fallback to the immutable NSTemporaryDirectory value. On 2017/02/28 21:59:20, Mark Mentovai wrote: > On 2017/02/27 01:16:20, Nico wrote: > > nit: "fallback" is a noun, "fall back" is the verb > > Fix this :) Done.
https://codereview.chromium.org/2713293002/diff/100001/base/files/file_util_m... File base/files/file_util_mac.mm (right): https://codereview.chromium.org/2713293002/diff/100001/base/files/file_util_m... base/files/file_util_mac.mm:9: #include <stdlib.h> `git cl format` made me do this
And actually it looks like $TMPDIR is maybe set on iOS too: http://chaosinmotion.com/blog/?p=829 ¯\_(ツ)_/¯
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
iannucci wrote: > And actually it looks like $TMPDIR is maybe set on iOS too: > http://chaosinmotion.com/blog/?p=829 > > ¯\_(ツ)_/¯ Sure, why wouldn’t it be? LGTM
Wait, wait, that was LGTM with these comments. Why didn’t the comments get posted? RIETVELD! https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... File chrome/browser/file_select_helper_mac.mm (right): https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... chrome/browser/file_select_helper_mac.mm:34: dest = dest.Append(base::SysNSStringToUTF8(bundleID)); For things that become file paths, this is better dest = dest.Append([bundleID fileSystemRepresentation]); Same on line 39. https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... chrome/browser/file_select_helper_mac.mm:38: NSString* GUID = [[NSProcessInfo processInfo] globallyUniqueString]; We name variables in lowercase, this should be “guid”.
On 2017/03/01 02:33:49, Mark Mentovai wrote: > Wait, wait, that was LGTM with these comments. Why didn’t the comments get > posted? RIETVELD! > I think if you 'reply' as opposed to 'publish+mail comments' it doesn't send them... but yes, I agree :D (RIETVELLLLD!!!!) > https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... > File chrome/browser/file_select_helper_mac.mm (right): > > https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... > chrome/browser/file_select_helper_mac.mm:34: dest = > dest.Append(base::SysNSStringToUTF8(bundleID)); > For things that become file paths, this is better > > dest = dest.Append([bundleID fileSystemRepresentation]); > > Same on line 39. > > https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... > chrome/browser/file_select_helper_mac.mm:38: NSString* GUID = [[NSProcessInfo > processInfo] globallyUniqueString]; > We name variables in lowercase, this should be “guid”.
https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... File chrome/browser/file_select_helper_mac.mm (right): https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... chrome/browser/file_select_helper_mac.mm:34: dest = dest.Append(base::SysNSStringToUTF8(bundleID)); On 2017/03/01 02:33:49, Mark Mentovai wrote: > For things that become file paths, this is better > > dest = dest.Append([bundleID fileSystemRepresentation]); > > Same on line 39. Done. https://codereview.chromium.org/2713293002/diff/120001/chrome/browser/file_se... chrome/browser/file_select_helper_mac.mm:38: NSString* GUID = [[NSProcessInfo processInfo] globallyUniqueString]; On 2017/03/01 02:33:49, Mark Mentovai wrote: > We name variables in lowercase, this should be “guid”. Done.
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org, maruel@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/2713293002/#ps140001 (title: "lowercase and filesystemRepresentation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488336618776030,
"parent_rev": "2432c940f1e00e4556f1f5f9db6de360e2a1fb6b", "commit_rev":
"7b5db573b3ebbc32ee4608163b9ba7e68750ee9a"}
Message was sent while issue was closed.
Description was changed from ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 ========== to ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 Review-Url: https://codereview.chromium.org/2713293002 Cr-Commit-Position: refs/heads/master@{#453835} Committed: https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9b... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9b...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2724733002/ by iannucci@chromium.org. The reason for reverting is: Looks like this broke mac compile https://build.chromium.org/p/chromium/buildstatus?builder=Mac&number=24213.
Message was sent while issue was closed.
On 2017/03/01 04:44:08, iannucci wrote: > A revert of this CL (patchset #8 id:140001) has been created in > https://codereview.chromium.org/2724733002/ by mailto:iannucci@chromium.org. > > The reason for reverting is: Looks like this broke mac compile > https://build.chromium.org/p/chromium/buildstatus?builder=Mac&number=24213. So apparently the trybots don't build any of the remoting stuff? :( That's pretty bad.
Message was sent while issue was closed.
Description was changed from ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 Review-Url: https://codereview.chromium.org/2713293002 Cr-Commit-Position: refs/heads/master@{#453835} Committed: https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9b... ========== to ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 Review-Url: https://codereview.chromium.org/2713293002 Cr-Commit-Position: refs/heads/master@{#453835} Committed: https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9b... ==========
Ok, figured out how to build this target locally and uploaded patchset 9
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, lambroslambrou@chromium.org, maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2713293002/#ps160001 (title: "Fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488349815450880,
"parent_rev": "bff9c922e36599cb97e47c0e8f1f190cc97e69ad", "commit_rev":
"95fcf47ca01054ffe38e8609525a6ca8579b7824"}
Message was sent while issue was closed.
Description was changed from ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 Review-Url: https://codereview.chromium.org/2713293002 Cr-Commit-Position: refs/heads/master@{#453835} Committed: https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9b... ========== to ========== Allow $TMPDIR to set temporary directory on OS X. This allows the continuous integration builds to be more hermetic by supplying a managed value for $TMPDIR and having chromium + tests respect it. NSTemporaryDirectory takes its value directly from a non-overridable confstr(_CS_DARWIN_USER_TEMP_DIR) call internally. This CL brings OS X more in line with Windows and Linux which both respect an environment variable ($TEMP and $TMPDIR respectively). BUG=696803 Review-Url: https://codereview.chromium.org/2713293002 Cr-Original-Commit-Position: refs/heads/master@{#453835} Committed: https://chromium.googlesource.com/chromium/src/+/7b5db573b3ebbc32ee4608163b9b... Review-Url: https://codereview.chromium.org/2713293002 Cr-Commit-Position: refs/heads/master@{#453873} Committed: https://chromium.googlesource.com/chromium/src/+/95fcf47ca01054ffe38e8609525a... ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
