Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2086103002:
Add base::ExecutableExistsInPath (Closed)
|
Created:
4 years, 6 months ago by Tom (Use chromium acct) Modified:
4 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd base::ExecutableExistsInPath
BUG=622021
Committed: https://crrev.com/1929bb230afca9fd8958133bbbe948edbd28aa05
Cr-Commit-Position: refs/heads/master@{#401783}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 5
Patch Set 3 : Add unit test #
Total comments: 2
Patch Set 4 : Rewrite unit test #
Total comments: 11
Patch Set 5 : Don't leak base::Environment #
Messages
Total messages: 39 (16 generated)
thomasanderson@google.com changed reviewers: + jam@chromium.org, thestig@chromium.org
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086103002/20001
Maybe add unit tests to make sure 'sh' is in $PATH and 'foobar' isn't? You need someone from net/OWNERS, not top level OWNERS. https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util.h#... base/files/file_util.h:204: BASE_EXPORT bool ExecutableExistsInPath(const char* executable); Use a StringPiece https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util_po... base/files/file_util_posix.cc:461: std::unique_ptr<base::Environment> env(base::Environment::Create()); You can omit base:: in namespace base.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add base::ExecutableExistsInPath BUG=622021 ========== to ========== Add base::ExecutableExistsInPath BUG=622021 ==========
jam@chromium.org changed reviewers: - jam@chromium.org
removing myself, thestig can review base and chrome and you can add a net/ person
thomasanderson@google.com changed reviewers: + mattm@chromium.org
https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util.h#... base/files/file_util.h:204: BASE_EXPORT bool ExecutableExistsInPath(const char* executable); On 2016/06/21 21:31:21, Lei Zhang (Slow) wrote: > Use a StringPiece Used FilePath::StringType instead, as this is what the surrounding methods seem to use. PLMK if it should still be StringPiece https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util_po... File base/files/file_util_posix.cc (right): https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util_po... base/files/file_util_posix.cc:461: std::unique_ptr<base::Environment> env(base::Environment::Create()); On 2016/06/21 21:31:21, Lei Zhang (Slow) wrote: > You can omit base:: in namespace base. Done. https://codereview.chromium.org/2086103002/diff/40001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2086103002/diff/40001/base/files/file_util_un... base/files/file_util_unittest.cc:841: TEST_F(FileUtilTest, ExecutableExistsInPath) { Not much to it at this point... Maybe it would be better to manually set the PATH here to 2 temp directories, and add/remove some executable (and nonexecutable) files to test more thoroughly?
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086103002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
https://codereview.chromium.org/2086103002/diff/40001/net/proxy/proxy_config_... File net/proxy/proxy_config_service_linux.cc (right): https://codereview.chromium.org/2086103002/diff/40001/net/proxy/proxy_config_... net/proxy/proxy_config_service_linux.cc:841: if (base::ExecutableExistsInPath("gnome-network-properties")) { This should probably accept the Environment to use as an argument? That would avoid having to re-do the parsing if you already have one like here. Also the unittest uses a mock environment for testing. It doesn't seem to test this condition explicitly but it might depend on it anyway. (eg, it may pass/fail depending on whether the test system has the binary in path. I haven't tried to verify if that actually happens but it seems worth preserving the old behavior in any case.)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086103002/60001
On 2016/06/22 21:12:26, mattm wrote: > https://codereview.chromium.org/2086103002/diff/40001/net/proxy/proxy_config_... > File net/proxy/proxy_config_service_linux.cc (right): > > https://codereview.chromium.org/2086103002/diff/40001/net/proxy/proxy_config_... > net/proxy/proxy_config_service_linux.cc:841: if > (base::ExecutableExistsInPath("gnome-network-properties")) { > This should probably accept the Environment to use as an argument? That would > avoid having to re-do the parsing if you already have one like here. done > > Also the unittest uses a mock environment for testing. It doesn't seem to test > this condition explicitly but it might depend on it anyway. (eg, it may > pass/fail depending on whether the test system has the binary in path. I haven't > tried to verify if that actually happens but it seems worth preserving the old > behavior in any case.) Rewrote the unit test
lgtm https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2086103002/diff/20001/base/files/file_util.h#... base/files/file_util.h:204: BASE_EXPORT bool ExecutableExistsInPath(const char* executable); On 2016/06/22 16:57:55, Tom Anderson wrote: > On 2016/06/21 21:31:21, Lei Zhang (Slow) wrote: > > Use a StringPiece > > Used FilePath::StringType instead, as this is what the surrounding methods seem > to use. PLMK if it should still be StringPiece Ya, that's fine. https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util.h#... base/files/file_util.h:41: class Environment; nit: alphabetical order https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util.h#... base/files/file_util.h:204: // $PATH environment variable. ... environment variable in |env|. https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:864: // Write file. Maybe just TouchFile() instead?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... File chrome/browser/printing/printer_manager_dialog_linux.cc (right): https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... chrome/browser/printing/printer_manager_dialog_linux.cc:36: if (!base::ExecutableExistsInPath(base::Environment::Create(), *command)) This will leak the Environment. (We should really update Create() to return a unique_ptr instead of a bare pointer.)
https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... File chrome/browser/printing/printer_manager_dialog_linux.cc (right): https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... chrome/browser/printing/printer_manager_dialog_linux.cc:36: if (!base::ExecutableExistsInPath(base::Environment::Create(), *command)) On 2016/06/23 21:32:53, mattm wrote: > This will leak the Environment. (We should really update Create() to return a > unique_ptr instead of a bare pointer.) Ditto in chrome/browser/ui/webui/settings_utils_linux.cc. I can make the fix separately. There's already a bug to do this for base/ APIs.
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util.h#... base/files/file_util.h:41: class Environment; On 2016/06/23 19:17:06, Lei Zhang (Slow) wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util.h#... base/files/file_util.h:204: // $PATH environment variable. On 2016/06/23 19:17:06, Lei Zhang (Slow) wrote: > ... environment variable in |env|. Done. https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:864: // Write file. On 2016/06/23 19:17:06, Lei Zhang (Slow) wrote: > Maybe just TouchFile() instead? TouchFile checks if a file exists first https://cs.chromium.org/chromium/src/base/files/file_util.cc?sq=package:chrom... https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... File chrome/browser/printing/printer_manager_dialog_linux.cc (right): https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... chrome/browser/printing/printer_manager_dialog_linux.cc:36: if (!base::ExecutableExistsInPath(base::Environment::Create(), *command)) On 2016/06/23 21:32:53, mattm wrote: > This will leak the Environment. (We should really update Create() to return a > unique_ptr instead of a bare pointer.) Done. https://codereview.chromium.org/2086103002/diff/60001/chrome/browser/printing... chrome/browser/printing/printer_manager_dialog_linux.cc:36: if (!base::ExecutableExistsInPath(base::Environment::Create(), *command)) On 2016/06/23 21:35:32, Lei Zhang (Slow) wrote: > On 2016/06/23 21:32:53, mattm wrote: > > This will leak the Environment. (We should really update Create() to return a > > unique_ptr instead of a bare pointer.) > > Ditto in chrome/browser/ui/webui/settings_utils_linux.cc. I can make the fix > separately. There's already a bug to do this for base/ APIs. I'm going to leave the unique_ptr change to Lei :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086103002/80001
lgtm
https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util_un... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2086103002/diff/60001/base/files/file_util_un... base/files/file_util_unittest.cc:864: // Write file. On 2016/06/23 21:55:12, Tom Anderson wrote: > On 2016/06/23 19:17:06, Lei Zhang (Slow) wrote: > > Maybe just TouchFile() instead? > > TouchFile checks if a file exists first > https://cs.chromium.org/chromium/src/base/files/file_util.cc?sq=package:chrom... I'm going to change my title to Offerer of Bad Advice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2086103002/#ps80001 (title: "Don't leak base::Environment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2086103002/80001
Message was sent while issue was closed.
Description was changed from ========== Add base::ExecutableExistsInPath BUG=622021 ========== to ========== Add base::ExecutableExistsInPath BUG=622021 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add base::ExecutableExistsInPath BUG=622021 ========== to ========== Add base::ExecutableExistsInPath BUG=622021 Committed: https://crrev.com/1929bb230afca9fd8958133bbbe948edbd28aa05 Cr-Commit-Position: refs/heads/master@{#401783} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1929bb230afca9fd8958133bbbe948edbd28aa05 Cr-Commit-Position: refs/heads/master@{#401783} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
