New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 756688 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 769304
issue skia:7072
issue 773933

Blocking:
issue 718157



Sign in to add a comment

Make disable_nacl a gclient arg and remove the GYP_DEFINE

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

Issue description

Now that we have support for conditionals in gclient, we can remove NaCl's use of GYP_DEFINES with a proper gclient arg that will skip the download of the NaCl DEP and not download the NaCl toolchains.
 
Blocking: 718157
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/b495bf501777c639131f54328d83b38a3163035e

commit b495bf501777c639131f54328d83b38a3163035e
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Tue Sep 26 08:08:19 2017

gclient: evaluate variables before passing them to GN

This helps make sure they have proper types (e.g. True
instead of "True" for booleans).

Also see https://chromium-review.googlesource.com/c/chromium/src/+/681854
for context.

Bug:  756688 ,  570091 
Change-Id: I1e4d26df724e8e94cc3daba361191856f80a1b2c
Reviewed-on: https://chromium-review.googlesource.com/681705
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

[modify] https://crrev.com/b495bf501777c639131f54328d83b38a3163035e/testing_support/fake_repos.py
[modify] https://crrev.com/b495bf501777c639131f54328d83b38a3163035e/gclient.py
[modify] https://crrev.com/b495bf501777c639131f54328d83b38a3163035e/tests/gclient_smoketest.py

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/manifest-internal/+/d6d535228550176f613d55ae8a97b4a4398dc7bd

commit d6d535228550176f613d55ae8a97b4a4398dc7bd
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Wed Sep 27 03:43:57 2017

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/manifest/+/3898395a5256c3b71169043b88b202ce74a402f4

commit 3898395a5256c3b71169043b88b202ce74a402f4
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Wed Sep 27 03:43:57 2017

Update depot_tools pin

This matches recipe CL:685754

CQ-DEPEND=CL:*464052
BUG= chromium:756688 
TEST=none

Change-Id: I4feea5a9ec5fde8cdf00ed5c9255aa80a9d6dc38
Reviewed-on: https://chromium-review.googlesource.com/685874
Commit-Ready: Don Garrett <dgarrett@chromium.org>
Tested-by: Don Garrett <dgarrett@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/3898395a5256c3b71169043b88b202ce74a402f4/full.xml

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/build/+/fd256d225e65971edfa6b5c72a813e5d2056d0bc

commit fd256d225e65971edfa6b5c72a813e5d2056d0bc
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Wed Sep 27 07:34:21 2017

Roll chromite pin 897602a:b495bf5

Bug:  chromium:756688 
Change-Id: Ib6541fa28164e7ea242fa326956619d18c33973e
Reviewed-on: https://chromium-review.googlesource.com/685754
Reviewed-by: Don Garrett <dgarrett@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>

[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/basic_compressed.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/pre_git_cache_release.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/unknown_config.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/swarming_builder.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipe_modules/chromite/api.py
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/chromiumos_paladin_buildbucket.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/chromiumos_paladin.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/chromiumos_coverage.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/chromiumos_chromium_builder.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/release_branch_one_param.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/pre_cq_buildbucket_config.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/release.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/external.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/ndk/ndk_buildbot.expected/basic.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/release_branch_two_params.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/chromiumos_coverage_variant.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot_tryjob.expected/internal.json
[modify] https://crrev.com/fd256d225e65971edfa6b5c72a813e5d2056d0bc/scripts/slave/recipes/cros/cbuildbot.expected/chromiumos_paladin_manifest_failure.json

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 27 2017

Blockedon: 769304
Project Member

Comment 9 by bugdroid1@chromium.org, Sep 27 2017

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

commit b5425ffc5acfadaef7b2bdb10bc9e45b6688c65c
Author: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Date: Wed Sep 27 15:08:29 2017

Revert "Add checkout_nacl gclient flag (default true)"

This reverts commit f036ddd72e5ca3de3c819e1d7c8163c2de8e60f2.

Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=769304

Original change's description:
> Add checkout_nacl gclient flag (default true)
> 
> See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/tO8kIrDtQc8/bCRO-UXxBQAJ for discussion.
> 
> Bug:  756688 
> Change-Id: I80f0526b278a813b398f6f81baacf36552da1792
> Reviewed-on: https://chromium-review.googlesource.com/681854
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#504598}

TBR=dschuff@chromium.org,phajdan.jr@chromium.org,dpranke@chromium.org

Change-Id: I4040923ea80defb25803406485d846fe0c82046d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  756688 
Reviewed-on: https://chromium-review.googlesource.com/687414
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504676}
[modify] https://crrev.com/b5425ffc5acfadaef7b2bdb10bc9e45b6688c65c/BUILD.gn
[modify] https://crrev.com/b5425ffc5acfadaef7b2bdb10bc9e45b6688c65c/DEPS
[modify] https://crrev.com/b5425ffc5acfadaef7b2bdb10bc9e45b6688c65c/build/.gitignore
[modify] https://crrev.com/b5425ffc5acfadaef7b2bdb10bc9e45b6688c65c/build/config/features.gni

Comment 10 by rmis...@google.com, Sep 27 2017

Looks like Skia bots which build chromium started failing after the change in  https://chromium-review.googlesource.com/681854. The error is:

ERROR at //build/config/features.gni:28:17: Left side of && operator is not a boolean.
  enable_nacl = checkout_nacl &&
                ^------------
Type is "string" instead.
See //BUILD.gn:12:1: whence it was imported.
import("//build/config/features.gni")
^-----------------------------------
Error (ret code: 1) calling "['/b/work/src/buildtools/mac/gn', 'gen', 'out/CommandBufferForSkia', '--args=is_component_build=false is_debug=false ']" in /b/work/src
step returned non-zero exit code: 1


More details in https://bugs.chromium.org/p/skia/issues/detail?id=7072


Looks like  https://chromium-review.googlesource.com/681854 was reverted but does not look like it was reverted for the same error as Skia saw.
Blockedon: skia:7072
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 28 2017

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

commit ccc0c36006da95b6c8a681abd48609180eafdfcf
Author: Dirk Pranke <dpranke@chromium.org>
Date: Thu Sep 28 22:15:11 2017

Move "enable_nacl" out of //build/config/features.gni.

Ideally //build/config/features.gni would be empty (or, at least,
would only reference features defined inside of //build).

This CL moves the `enable_nacl` flag out of that file, and into
its own dedicated file, so that it has a much smaller scope and
we can help ensure that it is not referenced or needed by other
projects that are using //build but aren't building Chromium.

This work will be useful as part of the work we're doing to
make NaCl something that can be disabled directly in DEPS
(without needing to depend on GYP_DEFINES to do so).

TBR=bradnelson@chromium.org

R: brettw@chromium.org, bradleynelson@chromium.org, dschuff@chromium.org, phajdan.jr@chromium.org
Bug:  756688 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I3af65646803912db67d421d35b4dfe9c2e0b703e
Reviewed-on: https://chromium-review.googlesource.com/688314
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505188}
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/build/config/features.gni
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/build/config/nacl/config.gni
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/build/config/nacl/rules.gni
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/app/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/app/mash/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/browser/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/browser/extensions/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/browser/nacl_host/test/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/browser/resources/chromeos/chromevox/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/common/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/installer/linux/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/installer/mini_installer/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/renderer/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/test/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/chrome/test/data/nacl/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/broker/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/browser/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/common/BUILD.gn
[add] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/features.gni
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/loader/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/loader/sandbox_linux/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/components/nacl/renderer/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/extensions/common/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/extensions/shell/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/extensions/shell/installer/linux/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/mash/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/native_client_sdk/src/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/ppapi/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/ppapi/native_client/nacl_test_data.gni
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/ppapi/native_client/src/untrusted/pnacl_support_extension/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/ppapi/tests/extensions/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/remoting/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/remoting/client/plugin/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/remoting/tools/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/remoting/tools/javascript_key_tester/BUILD.gn
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/remoting/webapp/build_template.gni
[modify] https://crrev.com/ccc0c36006da95b6c8a681abd48609180eafdfcf/tools/ipc_fuzzer/message_lib/BUILD.gn

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 10 2017

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

commit 75c1d5ecd30560b94e713881f69ef18d831b3adf
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Tue Oct 10 11:13:27 2017

Use native python bool for checkout_nacl default value

See https://chromium-review.googlesource.com/c/chromium/src/+/698164

Bug:  756688 
Change-Id: I51272d505b1bdafe1f992f568693009f105e8ae0
Reviewed-on: https://chromium-review.googlesource.com/708694
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507637}
[modify] https://crrev.com/75c1d5ecd30560b94e713881f69ef18d831b3adf/DEPS

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 11 2017

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

commit da2f3313e4c8e4de9c1011d3ed3e80a1bea5e04c
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Wed Oct 11 16:24:39 2017

Revert "Add checkout_nacl gclient flag (default true) (attempt #2)"

This reverts commit 53c4c334b38e95ec2fea2498354ba4864cb54585
(https://chromium-review.googlesource.com/c/702434/).

TBR=dpranke,dschuff

Bug: 773185,  756688 
Change-Id: Id66e32b07b05957b4a1265da723b14fb92123d7f
Reviewed-on: https://chromium-review.googlesource.com/713374
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507998}
[modify] https://crrev.com/da2f3313e4c8e4de9c1011d3ed3e80a1bea5e04c/BUILD.gn
[modify] https://crrev.com/da2f3313e4c8e4de9c1011d3ed3e80a1bea5e04c/DEPS
[modify] https://crrev.com/da2f3313e4c8e4de9c1011d3ed3e80a1bea5e04c/build/.gitignore
[modify] https://crrev.com/da2f3313e4c8e4de9c1011d3ed3e80a1bea5e04c/components/nacl/features.gni

Project Member

Comment 16 by bugdroid1@chromium.org, Oct 11 2017

Labels: merge-merged-3237
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/483a04a0e5753f6ffa84d427d5fe7fcdbd58d851

commit 483a04a0e5753f6ffa84d427d5fe7fcdbd58d851
Author: Paweł Hajdan, Jr <phajdan.jr@chromium.org>
Date: Wed Oct 11 16:28:22 2017

Revert "Add checkout_nacl gclient flag (default true) (attempt #2)"

This reverts commit 53c4c334b38e95ec2fea2498354ba4864cb54585
(https://chromium-review.googlesource.com/c/702434/).

TBR=dpranke,dschuff

Bug: 773185,  756688 
Change-Id: Id66e32b07b05957b4a1265da723b14fb92123d7f
Reviewed-on: https://chromium-review.googlesource.com/713374
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#507998}(cherry picked from commit da2f3313e4c8e4de9c1011d3ed3e80a1bea5e04c)
Reviewed-on: https://chromium-review.googlesource.com/713434
Reviewed-by: Michael Moss <mmoss@chromium.org>
Cr-Commit-Position: refs/branch-heads/3237@{#6}
Cr-Branched-From: d6c19dbb202fcf130f58db70d52ff208818374c3-refs/heads/master@{#507841}
[modify] https://crrev.com/483a04a0e5753f6ffa84d427d5fe7fcdbd58d851/BUILD.gn
[modify] https://crrev.com/483a04a0e5753f6ffa84d427d5fe7fcdbd58d851/DEPS
[modify] https://crrev.com/483a04a0e5753f6ffa84d427d5fe7fcdbd58d851/build/.gitignore
[modify] https://crrev.com/483a04a0e5753f6ffa84d427d5fe7fcdbd58d851/components/nacl/features.gni

Blockedon: 773933
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 2 2017

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

commit 4dabe8070550cb4014d1f996aa1a7e130bef42e1
Author: Dirk Pranke <dpranke@chromium.org>
Date: Thu Nov 02 07:18:55 2017

Add checkout_nacl gclient flag (default true) (attempt #3).

This is a straight (merged) re-land of {#507355}, since
the bug in gclient that this tickled was fixed.

See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/tO8kIrDtQc8/bCRO-UXxBQAJ
for discussion.

TBR=phajdan.jr@chromium.org, bradnelson@chromium.org
BUG= 756688 

Change-Id: Ife1ed0f30285828e37170306e2723fd70f591d6d
Reviewed-on: https://chromium-review.googlesource.com/750363
Commit-Queue: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513416}
[modify] https://crrev.com/4dabe8070550cb4014d1f996aa1a7e130bef42e1/BUILD.gn
[modify] https://crrev.com/4dabe8070550cb4014d1f996aa1a7e130bef42e1/DEPS
[modify] https://crrev.com/4dabe8070550cb4014d1f996aa1a7e130bef42e1/build/.gitignore
[modify] https://crrev.com/4dabe8070550cb4014d1f996aa1a7e130bef42e1/components/nacl/features.gni

It's now a gclient arg (that change looks to have stuck).
Status: Fixed (was: Assigned)

Sign in to add a comment