New issue
Advanced search Search tips

Issue 826218 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 4
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 718157



Sign in to add a comment

build/vs_toolchain.py still requires gyp

Project Member Reported by yangguo@chromium.org, Mar 27 2018

Issue description

Background is that V8 wants to remove gyp, including DEPS entry. That does not work because vs_toolchain.py is run as a gclient hook on Windows.

This is somewhat similar to  issue 807986 
 
Specifically, this happens when calling v8/tools/clang/scripts/update.py, which calls build/vs_toolchain.py, which imports gyp.
Cc: thomasanderson@chromium.org
Owner: ----
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/864adc09a78df14ad21a848141c086353cf6e094

commit 864adc09a78df14ad21a848141c086353cf6e094
Author: Yang Guo <yangguo@chromium.org>
Date: Wed Mar 28 05:42:28 2018

[node] include gyp in gclient sync.

Windows toolchain still needs relies on gyp.

R=sergiyb@chromium.org

Bug: v8:6105,  chromium:826218 
Change-Id: If7fba3cf986daa23a748681c3e6f1527af68b622
Reviewed-on: https://chromium-review.googlesource.com/980494
Reviewed-by: Sergiy Byelozyorov <sergiyb@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52259}
[modify] https://crrev.com/864adc09a78df14ad21a848141c086353cf6e094/tools/node/fetch_deps.py

Comment 4 by nisse@chromium.org, May 16 2018

Cc: nisse@chromium.org
Is anything still using GYP_DEFINES and other GYP_* environment variables? I'm tempted to make a cl to simply delete any code mentioning them and see if anything breaks.

The code in build/vs_toolchain.py seems to manipulate these variables, and import gyp in order to parse the value of GYP_DEFINES.
I suggest we gradually opt out of using those GYP variables to not break the fleet. I added a switch here to enable that: https://crrev.com/c/1004577 - but note that the switch is incomplete and e.g. doesn't touch code in chromium's test runner. There are still a few of suspicious code paths like https://chromium.googlesource.com/chromium/tools/build/+/589e2e4b45071c0/scripts/slave/recipe_modules/chromium/api.py#612

I opted V8 out here: https://crrev.com/c/1004578

V8 infra never broke due to this, so I assume we don't depend on it any longer. But there's always a risk that some script depends on it on a not regularly used code path. E.g. some update routine that first goes wrong when some local cache is purged after weeks...

Comment 6 by nisse@chromium.org, May 17 2018

Here's an experimental cl to delete setting of GYP_DEFINES (but keep GYP_MSVS_* unchanged): https://chromium-review.googlesource.com/c/chromium/src/+/1063611

What's needed to find what it breaks (if anything)?
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/741f5154b4a7b218924cd389d7730c6f909d16b6

commit 741f5154b4a7b218924cd389d7730c6f909d16b6
Author: Niels Möller <nisse@chromium.org>
Date: Mon Jun 11 09:07:50 2018

Delete code to manipulate GYP_DEFINES

This drops the gyp dependency from vs_toolchain.py. Setting of other
GYP_MSVS_* environment variables left unchanged.

Bug:  826218 
Change-Id: I86e20065e95e17f85566440009df40eaf27069ac
Reviewed-on: https://chromium-review.googlesource.com/1063611
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Niels Möller <nisse@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565949}
[modify] https://crrev.com/741f5154b4a7b218924cd389d7730c6f909d16b6/build/vs_toolchain.py

Can gyp now be removed from v8's DEPS file?
Blocking: 718157
Cc: brucedaw...@chromium.org yangguo@chromium.org
 Issue 648423  has been merged into this issue.
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a3016e82fc64133dd6fad10b6f835599c2a78275

commit a3016e82fc64133dd6fad10b6f835599c2a78275
Author: Nico Weber <thakis@chromium.org>
Date: Fri Aug 31 19:05:31 2018

stop depsing in gyp

made possible by https://chromium-review.googlesource.com/c/chromium/src/+/1063611

Bug:  826218 
Change-Id: I1c0a47b439b5a0ee187ffc12a1d20aff203f7301
Reviewed-on: https://chromium-review.googlesource.com/1198025
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588108}
[modify] https://crrev.com/a3016e82fc64133dd6fad10b6f835599c2a78275/.gitignore
[modify] https://crrev.com/a3016e82fc64133dd6fad10b6f835599c2a78275/DEPS

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 31

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a2b6b8e797fe4091b994626e77069b206be9ab38

commit a2b6b8e797fe4091b994626e77069b206be9ab38
Author: Nico Weber <thakis@chromium.org>
Date: Fri Aug 31 21:51:38 2018

Remove some unused code.

gyp is no longer needed on sys.path after
https://chromium-review.googlesource.com/c/chromium/src/+/1063611

Bug:  826218 
Change-Id: I5ee268cab2f75b4fd6bddd62685f14dee4669626
Reviewed-on: https://chromium-review.googlesource.com/1199572
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588182}
[modify] https://crrev.com/a2b6b8e797fe4091b994626e77069b206be9ab38/build/vs_toolchain.py

Project Member

Comment 13 by bugdroid1@chromium.org, Sep 3

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/ab1ddf2b7e3923ebef0939959f42ecece3464088

commit ab1ddf2b7e3923ebef0939959f42ecece3464088
Author: Nico Weber <thakis@chromium.org>
Date: Mon Sep 03 14:13:18 2018

stop depsing in gyp

made possible by https://chromium-review.googlesource.com/c/chromium/src/+/1063611

Bug:  chromium:826218 
Change-Id: Id3123de5705c91beb0a5eb87ca4490fe55a7de01
Reviewed-on: https://chromium-review.googlesource.com/1201002
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55591}
[modify] https://crrev.com/ab1ddf2b7e3923ebef0939959f42ecece3464088/.gitignore
[modify] https://crrev.com/ab1ddf2b7e3923ebef0939959f42ecece3464088/DEPS

Owner: nisse@chromium.org
Status: Fixed (was: Available)
Recent gclient sync output:

WARNING: 'src\tools\gyp' is no longer part of this client.  It is recommended that you manually remove it.


Yay!

 Issue 712883  has been merged into this issue.
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9

commit f73f0df88a4468ec6ce57a1587fc7f667dfce7a9
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Sep 24 13:52:49 2018

Remove references to gyp from DEPS and docs

Bug:  826218 
Change-Id: I176e1aeb0b24b21c6b4e5ee40910dce2bce52c95
Reviewed-on: https://chromium-review.googlesource.com/1239461
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#593522}
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/android_test_instructions.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/angle_in_chromium.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/ccache_mac.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/eclipse.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/emacs.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/gpu/debugging_gpu_related_code.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/gpu/gpu_testing.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/gpu/pixel_wrangling.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/ipc_fuzzer.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/linux_chromium_arm.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/linux_debugging.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/linux_hw_video_decode.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/linux_profiling.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/linux_suid_sandbox_development.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/memory-infra/README.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/using_a_linux_chroot.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/vanilla_msysgit_workflow.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/docs/windows_split_dll.md
[modify] https://crrev.com/f73f0df88a4468ec6ce57a1587fc7f667dfce7a9/tools/DEPS

Sign in to add a comment