Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1561873002:
Allow python.bat files for finding Python in GN. (Closed)
DescriptionAllow python.bat files for finding Python in GN.
Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases.
This comes up in practice if you install depot tools on a clean machine and add depot_tools to your path. Python will be a batch file that redirects to a subdirectory. Googlers don't see this because corp images have Python installed on the path separately.
Committed: https://crrev.com/ed40c411145ea48d8d76e171ec4fa3ba84d130d0
Cr-Commit-Position: refs/heads/master@{#367858}
Patch Set 1 #Patch Set 2 : #Messages
Total messages: 17 (8 generated)
brettw@chromium.org changed reviewers: + agrieve@chromium.org
The CQ bit was checked by brettw@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561873002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561873002/20001
Description was changed from ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also, but only in ASCII cases as documented extensively in the source code. ========== to ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. ==========
Description was changed from ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. ========== to ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. This comes up in practice if you install depot tools on a clean machine and add depot_tools to your path. Python will be a batch file that redirects to a subdirectory. Googlers don't see this because corp images have Python installed on the path separately. ==========
Change lgtm, but it looks like you might be able to simplify by using the new
technique of using "cmd /c python" to replace the manual search through PATH
(replace FindWindowsPython with PythonBatToExe("python"))
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/06 01:23:02, agrieve wrote:
> Change lgtm, but it looks like you might be able to simplify by using the new
> technique of using "cmd /c python" to replace the manual search through PATH
> (replace FindWindowsPython with PythonBatToExe("python"))
That's what it used to do but I removed it in
https://codereview.chromium.org/1462393003 to save about 80ms on Windows. That
change broke the batch case, which is what I'm now trying to fix without forcing
everybody to have those 80ms.
The CQ bit was checked by brettw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561873002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561873002/20001
On 2016/01/06 18:09:35, brettw wrote:
> On 2016/01/06 01:23:02, agrieve wrote:
> > Change lgtm, but it looks like you might be able to simplify by using the
new
> > technique of using "cmd /c python" to replace the manual search through PATH
> > (replace FindWindowsPython with PythonBatToExe("python"))
>
> That's what it used to do but I removed it in
> https://codereview.chromium.org/1462393003 to save about 80ms on Windows. That
> change broke the batch case, which is what I'm now trying to fix without
forcing
> everybody to have those 80ms.
thanks for the context :)
Message was sent while issue was closed.
Description was changed from ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. This comes up in practice if you install depot tools on a clean machine and add depot_tools to your path. Python will be a batch file that redirects to a subdirectory. Googlers don't see this because corp images have Python installed on the path separately. ========== to ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. This comes up in practice if you install depot tools on a clean machine and add depot_tools to your path. Python will be a batch file that redirects to a subdirectory. Googlers don't see this because corp images have Python installed on the path separately. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. This comes up in practice if you install depot tools on a clean machine and add depot_tools to your path. Python will be a batch file that redirects to a subdirectory. Googlers don't see this because corp images have Python installed on the path separately. ========== to ========== Allow python.bat files for finding Python in GN. Previously gn would look for python.exe on the path. This adds the ability to handle python.bat files also in many cases. This comes up in practice if you install depot tools on a clean machine and add depot_tools to your path. Python will be a batch file that redirects to a subdirectory. Googlers don't see this because corp images have Python installed on the path separately. Committed: https://crrev.com/ed40c411145ea48d8d76e171ec4fa3ba84d130d0 Cr-Commit-Position: refs/heads/master@{#367858} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ed40c411145ea48d8d76e171ec4fa3ba84d130d0 Cr-Commit-Position: refs/heads/master@{#367858} |
