Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745863002:
Only import gyp when required in vs_toolchain.py. (Closed)
|
Created:
3 years, 9 months ago by Lei Zhang Modified:
3 years, 9 months ago Reviewers:
brucedawson CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly import gyp when required in vs_toolchain.py.
Also replace some elifs after returns with ifs.
BUG=648423
Review-Url: https://codereview.chromium.org/2745863002
Cr-Commit-Position: refs/heads/master@{#456250}
Committed: https://chromium.googlesource.com/chromium/src/+/9b24fa5e6df163fc6aaea2fbc9150016af90a2ce
Patch Set 1 #
Total comments: 4
Patch Set 2 : More comments and newlines #Patch Set 3 : merge #Patch Set 4 : fix typo #
Total comments: 2
Patch Set 5 : ordering #Messages
Total messages: 26 (16 generated)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Description was changed from ========== Only import gyp when required in vs_toolchain.py. Also replace some elifs after returns with ifs. ========== to ========== Only import gyp when required in vs_toolchain.py. Also replace some elifs after returns with ifs. BUG=648423 ==========
Dry run: 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
Dry run: This issue passed the CQ dry run.
thestig@chromium.org changed reviewers: + brucedawson@chromium.org
I'd like to take a step towards getting rid of GYP by moving the GYP DEPS entry into the Windows-only section. However, I found gclient runhooks runs vs_toolchain.py on all platforms, which fails due to the missing gyp import. By moving the gyp import to only where it's needed, it's gated on a sys.platform check that limits it to Windows only. This may not be the best Python style, but we have done it occasionally when it made sense.
A few thoughts... https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:64: import gyp Feels like the comment above applies to this line, which makes it slightly confusing. Maybe add a comment about how this is to minimize the exposure to gyp? https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:343: return ['03a4e939cd325d6bc5216af41b92d02dda1366a6'] This is going to conflict with another CL that I just uploaded: https://codereview.chromium.org/2741783006/diff/1/build/vs_toolchain.py We could race to the finish line, or you could incorporate part or all of my change. It turns out that having a default case is a bad idea.
https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:343: return ['03a4e939cd325d6bc5216af41b92d02dda1366a6'] On 2017/03/10 23:31:49, brucedawson wrote: > This is going to conflict with another CL that I just uploaded: > > https://codereview.chromium.org/2741783006/diff/1/build/vs_toolchain.py > > We could race to the finish line, or you could incorporate part or all of my > change. It turns out that having a default case is a bad idea. Please land your CL first. I'm not in any rush here.
https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/2745863002/diff/1/build/vs_toolchain.py#newco... build/vs_toolchain.py:64: import gyp On 2017/03/10 23:31:49, brucedawson wrote: > Feels like the comment above applies to this line, which makes it slightly > confusing. Maybe add a comment about how this is to minimize the exposure to > gyp? Done.
The CQ bit was checked by thestig@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...
The CQ bit was checked by thestig@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ready for another look.
lgtm with one (optional) nit https://codereview.chromium.org/2745863002/diff/60001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/2745863002/diff/60001/build/vs_toolchain.py#n... build/vs_toolchain.py:350: if env_version == '2013': Nit - consider having the versions in the opposite order for consistency with _VersionNumber
https://codereview.chromium.org/2745863002/diff/60001/build/vs_toolchain.py File build/vs_toolchain.py (right): https://codereview.chromium.org/2745863002/diff/60001/build/vs_toolchain.py#n... build/vs_toolchain.py:350: if env_version == '2013': On 2017/03/11 01:31:05, brucedawson wrote: > Nit - consider having the versions in the opposite order for consistency with > _VersionNumber Done.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2745863002/#ps80001 (title: "ordering")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489196228108700,
"parent_rev": "e4fe160f57cd3d79526b4e7b5c2d20be7014eadc", "commit_rev":
"9b24fa5e6df163fc6aaea2fbc9150016af90a2ce"}
Message was sent while issue was closed.
Description was changed from ========== Only import gyp when required in vs_toolchain.py. Also replace some elifs after returns with ifs. BUG=648423 ========== to ========== Only import gyp when required in vs_toolchain.py. Also replace some elifs after returns with ifs. BUG=648423 Review-Url: https://codereview.chromium.org/2745863002 Cr-Commit-Position: refs/heads/master@{#456250} Committed: https://chromium.googlesource.com/chromium/src/+/9b24fa5e6df163fc6aaea2fbc915... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9b24fa5e6df163fc6aaea2fbc915... |
