Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2094713002:
Include PowerShell in Windows PATH, if available. (Closed)
|
Created:
4 years, 6 months ago by dnj (Google) Modified:
4 years, 6 months ago CC:
chromium-reviews, infra-reviews+build_chromium.org, kjellander-cc_chromium.org Target Ref:
refs/heads/master Project:
build Visibility:
Public. |
DescriptionInclude PowerShell in Windows PATH, if available.
Allow pre-existing PowerShell PATH to carry forward in the sanitized PATH
if present to avoid requiring the user to hard-code the PowerShell
package installation (currently
c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe).
BUG=chromium:623118
TEST=None
Committed: https://chromium.googlesource.com/chromium/tools/build/+/989ce90d39cd75fe9bf41ea3ecccfedf0f50e7d6
Patch Set 1 #
Total comments: 6
Patch Set 2 : Cleaner, better dox. #
Total comments: 2
Patch Set 3 : Actually use "which". #
Total comments: 2
Patch Set 4 : abspath #Patch Set 5 : Don't use Python reserved word... #Messages
Total messages: 32 (15 generated)
dnj@google.com changed reviewers: + hinoka@chromium.org
PTAL. This solves a problem right now with the sanitized path losing information about where PowerShell is installed, requiring a user to manually locate it. I believe this is a better option, since the system already knows where the software is.
hinoka@google.com changed reviewers: + hinoka@google.com
https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py File slave/run_slave.py (right): https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py#newcode440 slave/run_slave.py:440: if phrase in e: if phrase in e.listdir()?
Oh I see what you're doing. You're matching on the folder name. Why not just use the "which()" approach?
On 2016/06/23 16:46:18, Ryan Tseng wrote: > Oh I see what you're doing. You're matching on the folder name. Why not just > use the "which()" approach? A proper Windows which is a bit more complicated: https://github.com/mbr/shutilwhich/blob/master/shutilwhich/lib.py I think this approach is simple, to the point, and sufficient for these purposes (specifically, "keep path element X" is simpler than "find the path element that has X and then keep it").
If we stipulate powershell.exe is always under WindowsPowerShell, why not also stipulate that it's always at c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe? That would seem more straightforward to me.
On 2016/06/23 21:18:19, hinoka wrote:
> If we stipulate powershell.exe is always under WindowsPowerShell, why not also
> stipulate that it's always at
> c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe? That would seem
more
> straightforward to me.
I think the name of the package ("WindowsPowerShell") changing is less likely
than the package version number changing.
lgtm + add more comments. https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py File slave/run_slave.py (right): https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py#newcode438 slave/run_slave.py:438: def get_path_element(phrase): This function does something pretty unexpected (to me) so i'd opt for commenting it # returns a path element where one or more of it's directories contains the given phrase. https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py#newcode509 slave/run_slave.py:509: # Include Windows PowerShell in PATH, if defined. # It does so by searching for a path entry that contains "WindowsPowerShell", # which is assumed to be the path entry to contain the powershell executable.
https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py File slave/run_slave.py (right): https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py#newcode438 slave/run_slave.py:438: def get_path_element(phrase): On 2016/06/23 22:17:55, hinoka wrote: > This function does something pretty unexpected (to me) so i'd opt for commenting > it > > # returns a path element where one or more of it's directories contains the > given phrase. Done. https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py#newcode440 slave/run_slave.py:440: if phrase in e: On 2016/06/23 16:45:30, Ryan Tseng wrote: > if phrase in e.listdir()? "e" is a string here. https://codereview.chromium.org/2094713002/diff/1/slave/run_slave.py#newcode509 slave/run_slave.py:509: # Include Windows PowerShell in PATH, if defined. On 2016/06/23 22:17:55, hinoka wrote: > # It does so by searching for a path entry that contains "WindowsPowerShell", > # which is assumed to be the path entry to contain the powershell executable. Done.
The CQ bit was checked by dnj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@chromium.org Link to the patchset: https://codereview.chromium.org/2094713002/#ps20001 (title: "Cleaner, better dox.")
The CQ bit was unchecked by dnj@google.com
Description was changed from ========== Include PowerShell in Windows PATH, if available. Allow pre-existing PowerShell PATH to carry forward in the sanitized PATH if present to avoid requiring the user to hard-code the PowerShell package installation (currently c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe). BUG= ========== to ========== Include PowerShell in Windows PATH, if available. Allow pre-existing PowerShell PATH to carry forward in the sanitized PATH if present to avoid requiring the user to hard-code the PowerShell package installation (currently c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe). BUG=chromium:623118 TEST=None ==========
The CQ bit was checked by dnj@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dnj@google.com changed reviewers: + agable@chromium.org
+agable@, PTAL from OWNER.
The CQ bit was unchecked by dnj@google.com
LGTM % comment. https://codereview.chromium.org/2094713002/diff/20001/slave/run_slave.py File slave/run_slave.py (right): https://codereview.chromium.org/2094713002/diff/20001/slave/run_slave.py#newc... slave/run_slave.py:440: def get_path_component_containing(phrase): This function is only used once, and is defined nearly a hundred lines from where it is used (albeit, most of those lines are constants). Why make it a function at all? The name of the function also doesn't denote that it returns the *first* path component containing the phrase. Honestly I'd just inline it and comment the loop.
https://codereview.chromium.org/2094713002/diff/20001/slave/run_slave.py File slave/run_slave.py (right): https://codereview.chromium.org/2094713002/diff/20001/slave/run_slave.py#newc... slave/run_slave.py:440: def get_path_component_containing(phrase): On 2016/06/24 18:29:16, agable wrote: > This function is only used once, and is defined nearly a hundred lines from > where it is used (albeit, most of those lines are constants). Why make it a > function at all? The name of the function also doesn't denote that it returns > the *first* path component containing the phrase. Honestly I'd just inline it > and comment the loop. My thought was that it is OS-independent and seemed generically useful, so I wouldn't mind porting more path-derivations over to this method in the future. However, agreed, I'll go ahead and move it locally. Actually I'll just switch over to straightforward 'which / dirname'. Why not?
This seems overkill for this purpose but lgtm https://codereview.chromium.org/2094713002/diff/40001/scripts/common/chromium... File scripts/common/chromium_utils.py (right): https://codereview.chromium.org/2094713002/diff/40001/scripts/common/chromium... scripts/common/chromium_utils.py:2062: return cmd This returns "powershell.exe" if it's in cwd https://codereview.chromium.org/2094713002/diff/40001/scripts/common/chromium... scripts/common/chromium_utils.py:2085: for dir in path: funky choice of varname...
The CQ bit was checked by dnj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org Link to the patchset: https://codereview.chromium.org/2094713002/#ps60001 (title: "abspath")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build Presubmit on master.tryserver.infra (JOB_FAILED, https://build.chromium.org/p/tryserver.infra/builders/Build%20Presubmit/build...)
The CQ bit was checked by dnj@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from agable@chromium.org, hinoka@chromium.org Link to the patchset: https://codereview.chromium.org/2094713002/#ps80001 (title: "Don't use Python reserved word...")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Include PowerShell in Windows PATH, if available. Allow pre-existing PowerShell PATH to carry forward in the sanitized PATH if present to avoid requiring the user to hard-code the PowerShell package installation (currently c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe). BUG=chromium:623118 TEST=None ========== to ========== Include PowerShell in Windows PATH, if available. Allow pre-existing PowerShell PATH to carry forward in the sanitized PATH if present to avoid requiring the user to hard-code the PowerShell package installation (currently c:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe). BUG=chromium:623118 TEST=None Committed: https://chromium.googlesource.com/chromium/tools/build/+/989ce90d39cd75fe9bf4... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/tools/build/+/989ce90d39cd75fe9bf4... |
