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

Issue 684096 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit 26 days ago
Closed: Feb 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Replace V8 build_overrides with GN default_args

Project Member Reported by brettw@chromium.org, Jan 23 2017

Issue description

GN has a new default_args features for configuring repository-specific flags. We should switch the V8 flags to use this.
 

Comment 1 by brettw@chromium.org, Jan 24 2017

Plan:

1. Add current build overrides to declare_args in v8's BUILD.gn file, conditionally protected by a !defined() check. This will allow V8's build to be used with both new and old systems. V8's build_overrides/v8.gni can now be emptied.

2. Roll V8 into everywhere.

3. Update callers to simultaneously clear out their build_overrides/v8.gni and set their appropriate default values for these args.

4. Stop using build_overrides/v8.gni from v8's build

5. Roll V8 into everywhere.

6. Delete all remaining build_overrides/v8.gni
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 24 2017

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

commit 7b20d2e9e9113b3ad2ee1db858e8d16d2b947058
Author: brettw <brettw@chromium.org>
Date: Tue Jan 24 01:43:19 2017

Remove unnecessary uses of build_overrides/v8.gni.

I think this file used to have more stuff in it like v8_use_external_startup_data. This flag has now moved to v8/gni/v8.gni.

BUG= 684096 

Review-Url: https://codereview.chromium.org/2652853002
Cr-Commit-Position: refs/heads/master@{#445584}

[modify] https://crrev.com/7b20d2e9e9113b3ad2ee1db858e8d16d2b947058/BUILD.gn
[modify] https://crrev.com/7b20d2e9e9113b3ad2ee1db858e8d16d2b947058/chrome/BUILD.gn
[modify] https://crrev.com/7b20d2e9e9113b3ad2ee1db858e8d16d2b947058/chrome/android/BUILD.gn
[modify] https://crrev.com/7b20d2e9e9113b3ad2ee1db858e8d16d2b947058/content/shell/android/BUILD.gn

Project Member

Comment 3 by bugdroid1@chromium.org, Jan 24 2017

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

commit 3d6e10d81b19b35794e834f6bd602d892b2bf2f9
Author: pdfium-deps-roller <pdfium-deps-roller@chromium.org>
Date: Tue Jan 24 02:16:40 2017

Roll src/third_party/pdfium/ 4022f87eb..7a82a6294 (1 commit).

https://pdfium.googlesource.com/pdfium.git/+log/4022f87eb871..7a82a6294a34

$ git log 4022f87eb..7a82a6294 --date=short --no-merges --format='%ad %ae %s'
2017-01-23 brettw Remove obsolete include of build_overrides/v8.gni

BUG= 684096 

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls

TBR=dsinclair@chromium.org

Review-Url: https://codereview.chromium.org/2649923004
Cr-Commit-Position: refs/heads/master@{#445602}

[modify] https://crrev.com/3d6e10d81b19b35794e834f6bd602d892b2bf2f9/DEPS

Project Member

Comment 4 by bugdroid1@chromium.org, Jan 24 2017

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

commit 8a08c1ded65a1e9271afc3872f88180edae0c5ce
Author: brettw <brettw@chromium.org>
Date: Tue Jan 24 18:04:37 2017

Explicitly set some V8 build flags

These are currently implicitly set if undefined in v8/BUILD.gn. As part of
moving to the new default_args system, this will need to be explicit to roll
the new V8 in.

This should be a build no-op.

BUG= 684096 

Review-Url: https://codereview.chromium.org/2648383003
Cr-Commit-Position: refs/heads/master@{#445757}

[modify] https://crrev.com/8a08c1ded65a1e9271afc3872f88180edae0c5ce/build_overrides/v8.gni

Project Member

Comment 5 by bugdroid1@chromium.org, Jan 24 2017

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

commit cc77cea884ab97d4eab6abc8c7f809377941a382
Author: brettw <brettw@chromium.org>
Date: Tue Jan 24 22:16:47 2017

Explicitly set v8_enable_gdbjit_default for v8 overrides.

This is a fix for https://codereview.chromium.org/2648383003 where
v8_enable_gdbjit should actually be v8_enable_gdbjit_default. This
was pointed out in the review for https://codereview.chromium.org/2654663003/

BUG= 684096 
R=machenbach@chromium.org

Review-Url: https://codereview.chromium.org/2652083002
Cr-Commit-Position: refs/heads/master@{#445835}

[modify] https://crrev.com/cc77cea884ab97d4eab6abc8c7f809377941a382/build_overrides/v8.gni

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 25 2017

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

commit 04a0e487ece54c7e1568f53e3dc8b247dcf6ca74
Author: brettw <brettw@chromium.org>
Date: Wed Jan 25 21:29:59 2017

Use default_args for configuring V8.

Removes the values in build_overrides/v8.gni and uses the new default_args in
the "//.gn" file to set these arguments.

Once all places are updated, we can remove build_overrides/v8.gni.

BUG= 684096 

Review-Url: https://codereview.chromium.org/2648233006
Cr-Commit-Position: refs/heads/master@{#446124}

[modify] https://crrev.com/04a0e487ece54c7e1568f53e3dc8b247dcf6ca74/.gn
[modify] https://crrev.com/04a0e487ece54c7e1568f53e3dc8b247dcf6ca74/build_overrides/v8.gni

This feature causes error when running "gn gen" with "target_os" set to "ios".

$ gn gen --args='target_os="ios"' out/xxx
ERROR at //.gn:24:28: Build argument has no effect.
  v8_extra_library_files = [
                           ^
The variable "v8_extra_library_files" was set as a build argument
but never appeared in a declare_args() block in any buildfile.

To view all possible args, run "gn args --list <builddir>"
Done. Made 2833 targets from 577 files in 1191ms

Would it be possible to fix this in gn (as iOS does not want to depends on v8 just to silence the warning)? Maybe do not warn about unused args if they are declared in default_args.

Comment 8 by brettw@chromium.org, Jan 26 2017

sdefresne: This makes sense.
Is there any plan to address this issue? Should we revert the CL?
I'll do a mitigation today. It can't be easily reverted (requires a V8 roll).
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 27 2017

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

commit 08d54e1ba0047c6b42710d99a3687caadbdc3df8
Author: brettw <brettw@chromium.org>
Date: Fri Jan 27 21:28:42 2017

Add dummy V8 build args to prevent for the iOS build.

This is to prevent unused build overrides on iOS where V8 is not used but the default_arg overrides in //.gn are still present.

Longer-term we should probably not warn for unused args in the dotfile and then these can be removed.

BUG= 684096 
TBR=sdefresne

Review-Url: https://codereview.chromium.org/2656903006
Cr-Commit-Position: refs/heads/master@{#446779}

[modify] https://crrev.com/08d54e1ba0047c6b42710d99a3687caadbdc3df8/ios/BUILD.gn

Project Member

Comment 12 by bugdroid1@chromium.org, Feb 7 2017

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

commit e97ef561379aadc367fcde3ebc5b37fa4001796f
Author: brettw <brettw@chromium.org>
Date: Tue Feb 07 22:36:58 2017

Remove build_overrides/v8.gni.

This is no longer used by V8 and has been replaced by the default_args variable
in the //.gn file.

BUG= 684096 

Review-Url: https://codereview.chromium.org/2660523002
Cr-Commit-Position: refs/heads/master@{#448749}

[delete] https://crrev.com/46dad8ad40bd4d9e579974ee96568dd9b5cf6551/build_overrides/v8.gni

Status: Fixed (was: Started)
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 8 2017

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

commit 3273e13c90a9aca77955c9a386bc66f57dded38f
Author: mthiesse <mthiesse@chromium.org>
Date: Wed Feb 08 00:15:33 2017

Revert of Remove build_overrides/v8.gni. (patchset #2 id:20001 of https://codereview.chromium.org/2660523002/ )

Reason for revert:
build_overrides/v8.gni is still used by clank/java/BUILD.gn

Original issue's description:
> Remove build_overrides/v8.gni.
>
> This is no longer used by V8 and has been replaced by the default_args variable
> in the //.gn file.
>
> BUG= 684096 
>
> Review-Url: https://codereview.chromium.org/2660523002
> Cr-Commit-Position: refs/heads/master@{#448749}
> Committed: https://chromium.googlesource.com/chromium/src/+/e97ef561379aadc367fcde3ebc5b37fa4001796f

TBR=machenbach@chromium.org,brettw@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 684096 

Review-Url: https://codereview.chromium.org/2684833002
Cr-Commit-Position: refs/heads/master@{#448813}

[add] https://crrev.com/3273e13c90a9aca77955c9a386bc66f57dded38f/build_overrides/v8.gni

Project Member

Comment 15 by bugdroid1@chromium.org, Feb 9 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/77f15355248c15d8b4c6822a23732eda6e0b9142

commit 77f15355248c15d8b4c6822a23732eda6e0b9142
Author: Brett Wilson <brettw@chromium.org>
Date: Thu Feb 09 21:38:30 2017

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 9 2017

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

commit b54d63099501ccd240c5ab1d4e8f2ddbb0015b2d
Author: brettw <brettw@chromium.org>
Date: Thu Feb 09 23:12:37 2017

Remove build_overrides/v8.gni.

This is no longer used by V8 and has been replaced by the default_args variable
in the //.gn file.

BUG= 684096 

Review-Url: https://codereview.chromium.org/2660523002
Cr-Commit-Position: refs/heads/master@{#449452}

[delete] https://crrev.com/24835036681c326052b092b9697333ad9f4c2a9b/build_overrides/v8.gni

FYI: I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=7141 for similar work in WebRTC. 
Hi brettw@, I am not sure if I missed something but default_args seems to break building chromium for chromeos.

I followed simple chrome instruction to build chromium for chromeos.

http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chromium-browser

But I failed at gn gen step after entering sdk:

(sdk kevin R58-9282.0.0) cychiang@cychiang-z840 ~/work/chromium/src $ gn gen out_$SDK_BOARD/Release --args="$GN_ARGS"
ERROR at //.gn:30:16: Assignment had no effect.
default_args = {
               ^
You set the variable "default_args" here and it was unused before it went
out of scope.



Could you please take a look?

Thanks!
That error probably means your GN binary is out-of-date.
Hi brettw@, I found that I was on the wrong branch in src/v8 so it did not use default_args. After running gclient sync again the problem was gone. Thanks again!
Project Member

Comment 21 by bugdroid1@chromium.org, Apr 6 2017

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

commit 7ce21e2df98546eb664d6cbcd11a592d1599dd48
Author: sdefresne <sdefresne@chromium.org>
Date: Thu Apr 06 15:15:23 2017

[ios] Fix warning when running "gn gen".

Fix the following warning (incorrectly labelled as an error):
  ERROR at //.gn:44:25: Build argument has no effect.
    v8_enable_inspector = true
                          ^---
  The variable "v8_enable_inspector" was set as a build argument
  but never appeared in a declare_args() block in any buildfile.

BUG= 684096 

Review-Url: https://codereview.chromium.org/2799203003
Cr-Commit-Position: refs/heads/master@{#462476}

[modify] https://crrev.com/7ce21e2df98546eb664d6cbcd11a592d1599dd48/ios/BUILD.gn

I'm seeing the same error that comment #21 on a regular Windows checkout/build.
Project Member

Comment 23 by bugdroid1@chromium.org, Jun 7 2017

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

commit 994145519d481aa8f691a06299486fb533128832
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed Jun 07 08:35:02 2017

Fix a warning when running "gn gen" with target_os="ios".

Add declaration of gn argument v8_check_microtasks_scopes_consistency
to src/ios/BUILD.gn to silence the warning when running "gn gen". Add
a comment to .gn to remind people to keep in sync src/ios/BUILD.gn.

BUG= 684096 

Change-Id: If9a083a0474366ec1eb488fcb8fee6f739d96bea
Reviewed-on: https://chromium-review.googlesource.com/525574
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477582}
[modify] https://crrev.com/994145519d481aa8f691a06299486fb533128832/.gn
[modify] https://crrev.com/994145519d481aa8f691a06299486fb533128832/ios/BUILD.gn

Project Member

Comment 24 by bugdroid1@chromium.org, Dec 11 2017

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

commit a37803c95fe4014438ae1687a8ff2301c40e3ca9
Author: Petr Hosek <phosek@chromium.org>
Date: Mon Dec 11 23:43:45 2017

Don't print warning for unused default args

We will still produce an error if --fail-on-unused-args is used.

Bug:  684096 
Change-Id: I4a0640ad83072750d555c8de9e6391f76942ae38
Reviewed-on: https://chromium-review.googlesource.com/792606
Commit-Queue: Petr Hosek <phosek@chromium.org>
Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523254}
[modify] https://crrev.com/a37803c95fe4014438ae1687a8ff2301c40e3ca9/tools/gn/args.cc
[modify] https://crrev.com/a37803c95fe4014438ae1687a8ff2301c40e3ca9/tools/gn/args.h
[modify] https://crrev.com/a37803c95fe4014438ae1687a8ff2301c40e3ca9/tools/gn/setup.cc

Sign in to add a comment