New issue
Advanced search Search tips

Issue 756691 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 652016



Sign in to add a comment

Remove uses of any env vars when running gclient hooks

Project Member Reported by dpranke@chromium.org, Aug 18 2017

Issue description

Now that we have proper support for arguments and conditionals in gclient, we should remove any dependencies on env vars from the hooks, and make sure they are using flags instead.

This command, when run from the directory above src/ (on a Mac), gives the following list of files that reference environ:

$ grep -l environ $(cd src && grep "\.py" DEPS buildtools/DEPS | sort | uniq | sed -e "s/^DEPS://" -e "s/'action': \['python', //" -e "s/,.*$//" -e "/#/d" -e "s/\]$//" -e "s/'//g" | awk '{print $1}')
src/third_party/depot_tools/download_from_google_storage.py
src/build/android/play_services/update.py
src/third_party/gvr-android-sdk/test-apks/update.py
src/third_party/WebKit/Source/devtools/scripts/local_node/node.py
src/third_party/depot_tools/download_from_google_storage.py
src/build/get_syzygy_binaries.py
src/build/download_nacl_toolchains.py
src/build/landmines.py
src/third_party/binutils/download.py
src/build/linux/sysroot_scripts/install-sysroot.py
src/build/mac_toolchain.py
src/build/vs_toolchain.py
src/third_party/instrumented_libraries/scripts/download_binaries.py
src/tools/clang/scripts/update.py

it's possible some of these references may still be needed and appropriate, but many can be removed.

See also bug 718157 for removing the GYP_DEFINES uses explicitly.
 
Blockedon: 652016
Looking at landmines.py:

There is only one use of the env vars throughout all our landmines config scripts:
https://cs.chromium.org/chromium/src/build/get_landmines.py?q=get_landmines.py&sq=package:chromium&dr&l=78

One option would be to just naively remove it. This would clobber all chromium android arm32 builds once. If that's acceptable, it'd be the easiest path forward. In the new version we'd just drop support for differentiating target_arch.

If that's not acceptable, we'd need to find a way to pass target_arch to the landmines script.
That's acceptable. landmine changes are quite rare.
Looked closer. It's a little bit more. Win, mac, and linux platform are defined through the host. Only android through the OS setting in gyp_defines:
https://cs.chromium.org/chromium/src/build/landmine_utils.py?sq=package:chromium&dr&q=package:%5E(chromium)$+file:(/%7C%5E)landmine_utils(%5C.(swig%7Cpy%7Cspt)$%7C/(__init__%5C.(swig%7Cpy%7Cspt))?$)&l=79

I could remove the whole android condition (and the entries in the get_landmines script). This would once clobber all android bots and from then on, we'd not be able to do android-specific clobber - only host-os-linux specific. Not required so often... once every 2 years it seems :)
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 16 2018

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

commit d76f98f52683e21606b35d15b156e28a72cb85f4
Author: Michael Achenbach <machenbach@chromium.org>
Date: Fri Feb 16 22:40:01 2018

[build] Simplify landmines and strip off gyp-dependent options

This prepares removing the option to make landmines target-platform
dependent, as this depends on gyp environment variables.

Instead we'll be able to only depend on host_os.

This will cause clobber on all Android builds, as Android-specific
landmines will no longer be supported.

In follow-ups, we'll remove use of landmine_utils.platform() in other
client projects and then remove the definition here.

Bug: 756691
Change-Id: I08c1f4ecd05f5f4331bf1ab9eedda22419af2942
Reviewed-on: https://chromium-review.googlesource.com/924114
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537436}
[modify] https://crrev.com/d76f98f52683e21606b35d15b156e28a72cb85f4/build/get_landmines.py
[modify] https://crrev.com/d76f98f52683e21606b35d15b156e28a72cb85f4/build/landmine_utils.py

Project Member

Comment 6 by bugdroid1@chromium.org, Feb 16 2018

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

commit 77290e0d7f3750f52bc11e121922d79de3cf9c0d
Author: Michael Achenbach <machenbach@chromium.org>
Date: Fri Feb 16 23:28:14 2018

[build] Prepare removal of gyp-defines from landmine

Depends on: https://crrev.com/c/924114

TBR=yangguo@chromium.org

Bug: chromium:756691
Change-Id: Ibbc3bd35e8727296c1539edc961e3184830575ff
Reviewed-on: https://chromium-review.googlesource.com/924609
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51341}
[modify] https://crrev.com/77290e0d7f3750f52bc11e121922d79de3cf9c0d/tools/get_landmines.py

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/libyuv/libyuv.git/+/29ba52ed28dfe25c018c070781e20f0780b18b06

commit 29ba52ed28dfe25c018c070781e20f0780b18b06
Author: Michael Achenbach <machenbach@chromium.org>
Date: Fri Feb 16 23:38:18 2018

[build] Prepare removal of gyp-defines from landmine

Depends on: https://crrev.com/c/924114

Bug: chromium:756691
Change-Id: I4fb6dcbdf0d5e134ef3756b231d8f98d9d41b403
Reviewed-on: https://chromium-review.googlesource.com/924617
Reviewed-by: Frank Barchard <fbarchard@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>

[modify] https://crrev.com/29ba52ed28dfe25c018c070781e20f0780b18b06/tools_libyuv/get_landmines.py

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 19 2018

The following revision refers to this bug:
  https://webrtc.googlesource.com/src.git/+/500874effdada6f847092e98fe6f4a0b87d5c8b4

commit 500874effdada6f847092e98fe6f4a0b87d5c8b4
Author: Michael Achenbach <machenbach@chromium.org>
Date: Mon Feb 19 22:09:45 2018

[build] Prepare removal of gyp-defines from landmines

Depends on Chromium to roll: https://crrev.com/c/924114

This will clobber all Android builds once, since after this, we can't
make Android-specific landmines anymore.

Bug: chromium:756691
Change-Id: Ic7588329e567e3f6e596b04de8f990dc720eb153
Reviewed-on: https://webrtc-review.googlesource.com/54721
Reviewed-by: Patrik Höglund <phoglund@webrtc.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#22091}
[modify] https://crrev.com/500874effdada6f847092e98fe6f4a0b87d5c8b4/tools_webrtc/get_landmines.py

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 20 2018

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

commit d2fc4b94138c365bbb5d7fee3b885b2cf81cd5f8
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Feb 20 23:27:59 2018

[build] Remove gyp usage from landmines script

References from client projects to removed methods have been removed in
previous CLs.

Bug: 756691
Change-Id: I405ec09301455884f00348aadf82e865e85df721
Reviewed-on: https://chromium-review.googlesource.com/925683
Reviewed-by: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537929}
[modify] https://crrev.com/d2fc4b94138c365bbb5d7fee3b885b2cf81cd5f8/build/landmine_utils.py
[modify] https://crrev.com/d2fc4b94138c365bbb5d7fee3b885b2cf81cd5f8/build/landmines.py

Landmine scripts are now gyp-free.

Note, they are not env-variable free. They still reference CHROMIUM_OUT_DIR.
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 21 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/d13d68dccec6beffb55aae98a8e8021370483428

commit d13d68dccec6beffb55aae98a8e8021370483428
Author: Justin Cohen <justincohen@google.com>
Date: Wed Feb 21 05:33:08 2018

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 21 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/a6343d2fcc298697f0f3ec49a2b7ec4bbe03374b

commit a6343d2fcc298697f0f3ec49a2b7ec4bbe03374b
Author: Justin Cohen <justincohen@google.com>
Date: Wed Feb 21 15:53:31 2018

Sign in to add a comment