Windows ".bat" bootstraps include problematic "Cygwin only" PATH injection. |
||||||
Issue descriptionWindows ".bat" bootstraps (e.g., https://chromium.googlesource.com/chromium/tools/depot_tools/+/12ef501b02a9ff66323ba2da0432dc6717d5bc89/apply_issue.bat ) tend to include a line: :: This is required with cygwin only. PATH=%~dp0;%PATH% This prepends PATH with "depot_tools" when using these bootstraps, making it so that such entry points force "depot_tools" versions of tools, notably "git.bat" and "python.bat" installed by the Windows bootstrap: https://chromium.googlesource.com/chromium/tools/depot_tools/+/12ef501b02a9ff66323ba2da0432dc6717d5bc89/bootstrap/win The first instance of this bootstrap seems to have appeared on 5/11, 2009, here: https://codereview.chromium.org/113205 (Found via): $ git log -S 'This is required with cygwin only' --source --all It looks like from here, the PATH hack was transplanted to the remainder of the ".bat" shims. There isn't really any context here as to why this is necessary. It seems that Cygwin users should be able to get a similar benefit by just adding "depot_tools" to the beginning of their PATH. Can anyone think of a reason why we can't remove this PATH hack in favor of requiring this from Cygwin users (with a PSA, of course): https://chromium-review.googlesource.com/c/510290 (Adding people who might know, and maruel@ for sure b/c it was his patch :) )
,
May 23 2017
Well at least I *did* state it was an ugly "fix". I suspect it was just cargo culted from that point on. I think the point was to make sure this directory was before /usr/bin, in particular for svn. I don't think we need to keep this, we can switch to detect if 'which svn' != '.\git' and print an error message in this case. Implementing this as a batch file is left as an exercise to the reader. :)
,
May 24 2017
Sent PSA, preparing to commit CL: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/efRm9b6Nu6Q
,
May 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/1b9a43aab385070fd75263a9e0f4b4312b5f510e commit 1b9a43aab385070fd75263a9e0f4b4312b5f510e Author: Dan Jacques <dnj@chromium.org> Date: Wed May 24 22:22:52 2017 Remove special Cygwin PATH manipulation from .bat. Several boilerplate batch files include a provision to prepend "depot_tools" to PATH prior to running those tools. This undermines the utility of PATH overrides, since these tools specifically force their "depot_tools" sub-paths to be used regardless of environment. The origin of this behavior is likely limited to a specific fix for a specific problem, but was then perpetuated by the copy/paste of boilerplate bootstrap code as more bootstraps were added. This is important in upcoming configurations, where core tools such as Python and Git will be overridden via PATH on the bots. Cygwin users who depended on this behavior should just add "depot_tools" to their PATH in the appropriate location (i.e. in their .bashrc). BUG= chromium:714293 , chromium:724902 TEST=None Change-Id: Ie948a430847d20326d2411e9296cacd02f83a537 Reviewed-on: https://chromium-review.googlesource.com/510290 Commit-Queue: Daniel Jacques <dnj@chromium.org> Reviewed-by: Robbie Iannucci <iannucci@chromium.org> [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/depot-tools-auth.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/cit.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/gn.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/clang-format.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/fetch.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/commit_queue.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/clang_format_merge_driver.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/roll-dep-svn.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/roll-dep.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/apply_issue.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/gclient.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/cpplint.bat [modify] https://crrev.com/1b9a43aab385070fd75263a9e0f4b4312b5f510e/download_from_google_storage.bat
,
May 24 2017
,
May 24 2017
,
May 26 2017
No pitchfork?
,
May 26 2017
Nothing so far! Seems like either there are no more Cygwin developers or they have added "depot_tools" to PATH ages ago :) Next step is to get rid of "~dp0python" => "python" in all of the scripts.
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d commit 2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d Author: Dan Jacques <dnj@chromium.org> Date: Thu Jun 01 19:48:49 2017 [.bat] Remove "depot_tools" override. Currently, all ".bat" entry points use "~dp0python" to ensure that the Python that is used to execute the tool is the one in depot_tools. This prevents any sort of system override. Remove this override so that PATH solely determines which Python is used. To accommodate users who invoked these tools without Python on the PATH, we still still add "depot_tools" as a catch-all PATH suffix. Some tools were also not using DOS-style line endings. This CL fixes this. BUG= chromium:714293 , chromium:724902 TEST=None Change-Id: I06e9583a668c767196a2a335547aded868f2a2b5 Reviewed-on: https://chromium-review.googlesource.com/517236 Commit-Queue: Daniel Jacques <dnj@chromium.org> Reviewed-by: Robbie Iannucci <iannucci@chromium.org> Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/depot-tools-auth.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/cit.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/gn.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/clang-format.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/fetch.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/commit_queue.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/clang_format_merge_driver.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/roll-dep-svn.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/roll-dep.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/apply_issue.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/gclient.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/cpplint.bat [modify] https://crrev.com/2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d/download_from_google_storage.bat
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/96fa295ab453feee177e3b6cf0992d576cf87af5 commit 96fa295ab453feee177e3b6cf0992d576cf87af5 Author: Daniel Jacques <dnj@chromium.org> Date: Thu Jun 01 20:44:29 2017 Revert "[.bat] Remove "depot_tools" override." This reverts commit 2f5f0b7a99c9df653b090e3ef00d13a2cab00a8d. Reason for revert: <INSERT REASONING HERE> Original change's description: > [.bat] Remove "depot_tools" override. > > Currently, all ".bat" entry points use "~dp0python" to ensure that the > Python that is used to execute the tool is the one in depot_tools. This > prevents any sort of system override. > > Remove this override so that PATH solely determines which Python is > used. To accommodate users who invoked these tools without Python on the > PATH, we still still add "depot_tools" as a catch-all PATH suffix. > > Some tools were also not using DOS-style line endings. This CL fixes > this. > > BUG= chromium:714293 , chromium:724902 > TEST=None > > Change-Id: I06e9583a668c767196a2a335547aded868f2a2b5 > Reviewed-on: https://chromium-review.googlesource.com/517236 > Commit-Queue: Daniel Jacques <dnj@chromium.org> > Reviewed-by: Robbie Iannucci <iannucci@chromium.org> > Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> > TBR=maruel@chromium.org,iannucci@chromium.org,vadimsh@chromium.org,dnj@chromium.org No-Presubmit: true No-Tree-Checks: true No-Try: true BUG= chromium:714293 , chromium:724902 Change-Id: I822abdd4e02abd32d2f4789fb16d5a7f78fdd02d Reviewed-on: https://chromium-review.googlesource.com/521867 Reviewed-by: Daniel Jacques <dnj@chromium.org> Commit-Queue: Daniel Jacques <dnj@chromium.org> [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/depot-tools-auth.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/cit.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/gn.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/clang-format.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/fetch.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/commit_queue.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/clang_format_merge_driver.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/roll-dep-svn.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/roll-dep.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/apply_issue.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/gclient.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/cpplint.bat [modify] https://crrev.com/96fa295ab453feee177e3b6cf0992d576cf87af5/download_from_google_storage.bat
,
Jun 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/74809c1b318e7ae7cd472990728e76c8b5816f4c commit 74809c1b318e7ae7cd472990728e76c8b5816f4c Author: Dan Jacques <dnj@chromium.org> Date: Thu Jun 01 21:05:21 2017 [.bat] Remove "depot_tools" override. (#2) Second attempt at landing. Fix quotes around python in "gclient.bat". Currently, all ".bat" entry points use "~dp0python" to ensure that the Python that is used to execute the tool is the one in depot_tools. This prevents any sort of system override. Remove this override so that PATH solely determines which Python is used. To accommodate users who invoked these tools without Python on the PATH, we still still add "depot_tools" as a catch-all PATH suffix. Some tools were also not using DOS-style line endings. This CL fixes this. BUG= chromium:714293 , chromium:724902 TEST=None Change-Id: I0fceb99c8adb96e72dac706819be032d400aad37 Reviewed-on: https://chromium-review.googlesource.com/521704 Commit-Queue: Daniel Jacques <dnj@chromium.org> Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> Reviewed-by: Scott Graham <scottmg@chromium.org> Reviewed-by: Nodir Turakulov <nodir@chromium.org> [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/depot-tools-auth.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/cit.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/gn.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/clang-format.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/fetch.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/commit_queue.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/clang_format_merge_driver.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/roll-dep-svn.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/roll-dep.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/apply_issue.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/gclient.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/cpplint.bat [modify] https://crrev.com/74809c1b318e7ae7cd472990728e76c8b5816f4c/download_from_google_storage.bat
,
Jun 1 2017
Could this have caused the failures in: https://luci-milo.appspot.com/buildbot/chromium.win/Win%207%20Tests%20x64%20%281%29/24874 https://luci-milo.appspot.com/buildbot/chromium.win/Win7%20%2832%29%20Tests/20706 See the end of https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin7__32__Tests%2F20706%2F%2B%2Frecipes%2Fsteps%2Fnacl_integration%2F0%2Fstdout for example, which says "ImportError: No module named win32api", which looks to me like the kind of artifact we'd see from changing Python environments. I'm having a hard time determining when the changes here actually began affecting the bots in question.
,
Jun 9 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by no...@chromium.org
, May 22 2017Status: Assigned (was: Untriaged)