Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2287603003:
Move gyp-win-tool to the GN Windows toolchain. (Closed)
DescriptionMove gyp-win-tool to the GN Windows toolchain.
Renames it tool_wrapper.py but otherwise keeps it unchanged from the original
https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_tool.py
I changed internal references to itself to tool_wrapper.py, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check.
Changes the build to refer to the file in-place rather than copying it to the build directory which was confusing.
The tool runs itself in the link_manifest step. Since the file is no longer copied to what will be the current directory when linking, I believe this will be broken. But the GN build does not use the manifest linking command. To keep changes minimal, I kept this code in for now.
Committed: https://crrev.com/6afaa104b1fa8c6caee4e53b2e37142832ec9f9e
Cr-Commit-Position: refs/heads/master@{#414859}
Patch Set 1 #Patch Set 2 : .py extension #Patch Set 3 : tool_wrapper #Patch Set 4 : fixes #Patch Set 5 : fix #
Total comments: 4
Patch Set 6 : fix #
Dependent Patchsets: Messages
Total messages: 40 (17 generated)
Description was changed from ========== Move gyp-win-tool to the GN Windows toolchain. Renames it gn_win_tool but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... ========== to ========== Move gyp-win-tool to the GN Windows toolchain. Renames it gn_win_tool but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to gn_win_tool, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. ==========
brettw@chromium.org changed reviewers: + dpranke@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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move gyp-win-tool to the GN Windows toolchain. Renames it gn_win_tool but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to gn_win_tool, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. ========== to ========== Move gyp-win-tool to the GN Windows toolchain. Renames it gn_win_tool but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to gn_win_tool, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. There is likely dead code in this script that we will need to clean up later. ==========
.py extension
Description was changed from ========== Move gyp-win-tool to the GN Windows toolchain. Renames it gn_win_tool but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to gn_win_tool, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. ========== to ========== Move gyp-win-tool to the GN Windows toolchain. Renames it tool_wrapper.py but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to tool_wrapper.py, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. Changes the build to refer to the file in-place rather than copying it to the build directory which was confusing. The tool runs itself in the link_manifest step. Since the file is no longer copied to what will be the current directory when linking, I believe this will be broken. But the GN build does not use the manifest linking command. To keep changes minimal, I kept this code in for now. ==========
tool_wrapper
fixes
fix
brettw@chromium.org changed reviewers: + scottmg@chromium.org
sending to scottmg instead.
brettw@chromium.org changed reviewers: - dpranke@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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:23: tool_wrapper_path = rebase_path("tool_wrapper.py", root_build_dir) We are going to still copy it? So now using it from both places? Where is it copied now?
https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:23: tool_wrapper_path = rebase_path("tool_wrapper.py", root_build_dir) On 2016/08/26 20:54:36, scottmg wrote: > We are going to still copy it? So now using it from both places? Where is it > copied now? It's not copied. This is rebasing the path "tool_wrapper.py" (relative to the current directory) to be relative to the build directory.
lgtm https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:23: tool_wrapper_path = rebase_path("tool_wrapper.py", root_build_dir) On 2016/08/26 20:57:07, brettw (ping on IM after 24h) wrote: > On 2016/08/26 20:54:36, scottmg wrote: > > We are going to still copy it? So now using it from both places? Where is it > > copied now? > > It's not copied. This is rebasing the path "tool_wrapper.py" (relative to the > current directory) to be relative to the build directory. OK, I was misled by the prominent comment directly above the rebase_path. :)
https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... File build/toolchain/win/BUILD.gn (right): https://codereview.chromium.org/2287603003/diff/80001/build/toolchain/win/BUI... build/toolchain/win/BUILD.gn:23: tool_wrapper_path = rebase_path("tool_wrapper.py", root_build_dir) Whoops, fixed!
fix
The CQ bit was checked by brettw@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2287603003/#ps100001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
I think rsesek@ ended up splitting gyp-mac-tool out into separate individual scripts, which I think is cleaner and clearer (since there seems to be little to nothing being shared between commands here), so maybe we should do that instead? lgtm to land this as a transitional step though.
Also, just noting one minor thing, changes to this script will be a bit confusing now, as we don't have a dependency on it, but we do on everything(-ish?) else in build/toolchain/win. I'm not sure if we want one, but it can e.g. change the environment for linking, etc. so people might be confused if it doesn't cause everything to rebuild when it's touched. Or maybe they'll just be happy that they didn't have to wait for everything to rebuild.
On 2016/08/26 21:04:35, Dirk Pranke wrote: > I think rsesek@ ended up splitting gyp-mac-tool out into separate individual > scripts, which I think is cleaner and clearer (since there seems to be little to > nothing being shared between commands here), so maybe we should do that instead? > > lgtm to land this as a transitional step though. Yeah, might be break-up-able and would be clearer. The shared part is the environment block loading that's windows-only and is shared between ninja and a few of the commands that have to either inspect or modify the environment the tools are run in.
Splitting it is a good idea. I was quite pleased we don't use gyp-mac-tool any more (I just went to go do the same patch for Mac).
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by brettw@chromium.org
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 ========== Move gyp-win-tool to the GN Windows toolchain. Renames it tool_wrapper.py but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to tool_wrapper.py, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. Changes the build to refer to the file in-place rather than copying it to the build directory which was confusing. The tool runs itself in the link_manifest step. Since the file is no longer copied to what will be the current directory when linking, I believe this will be broken. But the GN build does not use the manifest linking command. To keep changes minimal, I kept this code in for now. ========== to ========== Move gyp-win-tool to the GN Windows toolchain. Renames it tool_wrapper.py but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to tool_wrapper.py, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. Changes the build to refer to the file in-place rather than copying it to the build directory which was confusing. The tool runs itself in the link_manifest step. Since the file is no longer copied to what will be the current directory when linking, I believe this will be broken. But the GN build does not use the manifest linking command. To keep changes minimal, I kept this code in for now. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Move gyp-win-tool to the GN Windows toolchain. Renames it tool_wrapper.py but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to tool_wrapper.py, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. Changes the build to refer to the file in-place rather than copying it to the build directory which was confusing. The tool runs itself in the link_manifest step. Since the file is no longer copied to what will be the current directory when linking, I believe this will be broken. But the GN build does not use the manifest linking command. To keep changes minimal, I kept this code in for now. ========== to ========== Move gyp-win-tool to the GN Windows toolchain. Renames it tool_wrapper.py but otherwise keeps it unchanged from the original https://chromium.googlesource.com/external/gyp.git/+/master/pylib/gyp/win_too... I changed internal references to itself to tool_wrapper.py, and split the internal strings containing the word "Copyright" (which it uses to filter tool output) to avoid triggering the copyright presubmit check. Changes the build to refer to the file in-place rather than copying it to the build directory which was confusing. The tool runs itself in the link_manifest step. Since the file is no longer copied to what will be the current directory when linking, I believe this will be broken. But the GN build does not use the manifest linking command. To keep changes minimal, I kept this code in for now. Committed: https://crrev.com/6afaa104b1fa8c6caee4e53b2e37142832ec9f9e Cr-Commit-Position: refs/heads/master@{#414859} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6afaa104b1fa8c6caee4e53b2e37142832ec9f9e Cr-Commit-Position: refs/heads/master@{#414859}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2287833002/ by jianli@chromium.org. The reason for reverting is: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b....
Message was sent while issue was closed.
FYI: Findit try jobs (rerunning failed compile or tests) identified this CL at revision 414859 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
Oops, I should have searched for gyp-win-tool: https://cs.chromium.org/chromium/src/native_client/src/trusted/win/BUILD.gn?q... Also delete these references on reland: https://cs.chromium.org/chromium/build/scripts/slave/build_directory.py?q=gyp... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
