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

Issue 766398 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Dec 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature



Sign in to add a comment

cros_chrome_sdk (simple chrome): Run gn gen

Project Member Reported by steve...@chromium.org, Sep 19 2017

Issue description

Multiple developers have requested the ability for Simple Chrome to run 'gn gen' automatically,

 
Labels: -Type-Bug -Pri-3 Pri-2 Type-Feature
Description: Show this description
Cc: akes...@chromium.org rcui@chromium.org vapier@chromium.org davidjames@chromium.org
Labels: Hotlist-CrOS-Gardener
There are a few options we need to consider:

1. Whether running 'gn gen' should be the default. Most developers probably want it enabled, especially if we provide a mechanism for adding developer args, however if we make it the default any existing args.gn files would be destructively overwritten.

2. How to provide additional developer arguments, e.g. 'dcheck_always_on', 'is_debug', 'symbol_level', 'optimize_webui' (formerly 'use_vulcanize'). This will be important for web UI developers and any developers wishing to use a debugger. There are a few options here:
a) Use a command line argument.
b) Use a Linux environment variable.
c) Use a Simple Chrome environment variable or command specified in ~/.chromite/chrome_sdk.bashrc

Comments / suggestions / preferences?

I think running gn gen should be default. Making the tool opinionated by default makes it simpler for the vast majority of developers, and advanced/custom scenarios have an escape hatch.

Having gn gen off by default means that when we tell the user about it, it is already too late. It is easy to forget to add the flag as well.

I'd be a little surprised if people heavily customized args.gn, given that the standard update mechanism is to overwrite the file. Power users who have customized args.gn likely know exactly where they deviate from the default, so reconfiguring it should be relatively painless.

If we don't turn the flag on by default, we're optimizing for a one-time cost, versus a greater mental burden long-term. A PSA will hopefully make this one-time cost minimal.

Comment 5 by derat@chromium.org, Sep 19 2017

I think I'm one of the devs who requested this, so I'll chime in.

1. Args are stored per-board, right? Developers who need to set them presumably already have them saved somewhere so they can restore them (e.g. for new boards).

2. I don't have strong opinions about giving people a way to modify $GN_ARGS vs. introducing a new environment variable for extra args, but I'd prefer to not have a command-line flag for this. I suspect that most developers will want to use the same args for every build instead of wanting to be able to specify them every time.
Anec-data: The 4 non-chromeos chrome developers I work with never customize args.gn for simplechrome.

The consensus for (1) seems to be make "gn gen" the default so I will go ahead and do that (and send a PSA when we update chromite).

The preference for (2) seems to be "not a command line arg".
FWIW, any Web UI developer will need to modify args.gn, and several such devs are Chrome devs using Simple Chrome (i.e. not "power users"). I am trying to find a fairly simple solution for them.

I have a preference for 2a (mostly because it's flexible, doesn't require an extra file, and is simple to implement), but it has been suggested that 2c is an existing mechanism so would be preferable. However the only way that comes to mind to implement 2c is to run "gn gen" after processing ~/.chromite/chrome_sdk.bashrc, which seems like it might be complicated (and is something I don't really have cycles for at the moment).

In order to move things along I put up an implementation for just (1):

https://chromium-review.googlesource.com/c/chromiumos/chromite/+/675567

I will have to live with running with using --nogn-gen until we decide how best to implement (2).

Project Member

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

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/66680126d28265cd47d3a5551d03892ccc100c77

commit 66680126d28265cd47d3a5551d03892ccc100c77
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Thu Sep 28 21:29:25 2017

cros_chrome_sdk: Add --gn-gen option

This also adds 'optimize_webui' to |args_to_ignore|,
replacing 'use_vulcanize'.

BUG= chromium:766398 
TEST=Run 'cros chrome-sdk' with and without -gn-gen.

Change-Id: Ib8f54a28e6a1ae95afe65a50b4d37f271dc1ba2a
Reviewed-on: https://chromium-review.googlesource.com/675567
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Tested-by: Steven Bennetts <stevenjb@chromium.org>

[modify] https://crrev.com/66680126d28265cd47d3a5551d03892ccc100c77/cli/cros/cros_chrome_sdk.py

Project Member

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

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/2995d34347981e8537814415f48ece2e770eb9d7

commit 2995d34347981e8537814415f48ece2e770eb9d7
Author: Michael Giuffrida <michaelpg@chromium.org>
Date: Fri Sep 29 18:18:15 2017

Revert "cros_chrome_sdk: Add --gn-gen option"

This reverts commit 66680126d28265cd47d3a5551d03892ccc100c77.

Reason for revert: Breaks PFQ's TestSimpleChromeWorkflow stage.  crbug.com/770251 

Original change's description:
> cros_chrome_sdk: Add --gn-gen option
> 
> This also adds 'optimize_webui' to |args_to_ignore|,
> replacing 'use_vulcanize'.
> 
> BUG= chromium:766398 
> TEST=Run 'cros chrome-sdk' with and without -gn-gen.
> 
> Change-Id: Ib8f54a28e6a1ae95afe65a50b4d37f271dc1ba2a
> Reviewed-on: https://chromium-review.googlesource.com/675567
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
> Tested-by: Steven Bennetts <stevenjb@chromium.org>

Bug:  chromium:766398 
TBR=stevenjb@chromium.org
Change-Id: Idddaf908aaad49da076d4723e5e2bf61e7352ad8
Reviewed-on: https://chromium-review.googlesource.com/693114
Tested-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>

[modify] https://crrev.com/2995d34347981e8537814415f48ece2e770eb9d7/cli/cros/cros_chrome_sdk.py

Comment 10 Deleted

Comment 11 Deleted

Project Member

Comment 12 by bugdroid1@chromium.org, Nov 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/9ab0a618d8d4c174fac0c0b3f29183e0b4eec11d

commit 9ab0a618d8d4c174fac0c0b3f29183e0b4eec11d
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Nov 13 18:03:20 2017

cros_chrome_sdk: Automatically generate args.gn files

With this CL, entering simple chrome will automatically generate
the args.gn file instead of just warning the developer.

Additioanlly:
* Adds --gn-extra-args to provide additional gn args
* Adds --nogn-gen to skip auto generation of args.gn (shows a
  warning instead, identical to current behavior)
* Adds 'optimize_webui' to |args_to_ignore|, replacing 'use_vulcanize'.
* Correctly supports running from a location other than chrome/src

BUG= chromium:766398 
TEST=Run 'cros chrome-sdk' with/without --gn-extra-args, --nogn-gen.
     Also test from outside the chrome/src directory (bots do this)

Change-Id: I3d0cb27faf5300c8e86a0234c37075beb45a6bef
Reviewed-on: https://chromium-review.googlesource.com/693006
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Tested-by: Steven Bennetts <stevenjb@chromium.org>

[modify] https://crrev.com/9ab0a618d8d4c174fac0c0b3f29183e0b4eec11d/cli/cros/cros_chrome_sdk.py

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/575c52b3e1f9e893254d02f073e61437f99bf3a9

commit 575c52b3e1f9e893254d02f073e61437f99bf3a9
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue Nov 21 19:08:59 2017

cros_chrome_sdk: Add --gn-gen and make --nogn-gen the default

This CL:
* Adds --gn-gen so that devs and bots can start using it
* Keeps --nogn-gen so that bots can continue to use that
* Makes --nogn-gen the default

BUG= chromium:766398 
TEST=Run 'cros chrome-sdk' with/without --gn-extra-args, --{no}gn-gen.

Change-Id: I6456cfc89e44c83b9f4e57a546b444b1b4dc4930
Reviewed-on: https://chromium-review.googlesource.com/778049
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Tested-by: Steven Bennetts <stevenjb@chromium.org>

[modify] https://crrev.com/575c52b3e1f9e893254d02f073e61437f99bf3a9/cli/cros/cros_chrome_sdk.py

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 27 2017

Project Member

Comment 15 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/docs/+/5a79a1cdd9c5bec15a980be662e5d8e699c18706

commit 5a79a1cdd9c5bec15a980be662e5d8e699c18706
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue Nov 28 21:34:30 2017

Update simple_chrome_workflow.md wiht '--gn-gen'

For now we want to encourage developers to pass 'gn-gen' to
simple chrome (until it becomes the default). We also want to
document 'gn-gen' and 'nogn-gen' regardless.

BUG= chromium:766398 
TEST=../chromium/src/tools/md_browser/md_browser.py -d .

Change-Id: Ic9c1544e2fdf17eec706b9be8c62f5f8f11c3712
Reviewed-on: https://chromium-review.googlesource.com/791520
Commit-Ready: Steven Bennetts <stevenjb@chromium.org>
Tested-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>

[modify] https://crrev.com/5a79a1cdd9c5bec15a980be662e5d8e699c18706/simple_chrome_workflow.md

Project Member

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

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/38b7e69faa51c9595cd8a2b429a2b144173f650a

commit 38b7e69faa51c9595cd8a2b429a2b144173f650a
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon Dec 11 23:16:26 2017

cros chrome-sdk: Make gn-gen the default

Now that the bots pass --nogn-gen to 'cros chrome-sdk' we can make
--gn-gen the default for developers.

BUG= chromium:766398 
TEST=cros_chrome_sdk_unittest; manual testing of 'cros chrome-sdk'
     with '-gn-gen', '--nogn-gen', and with neither.

Change-Id: I16e4f58c9193ca6137585ed844d8ae5af8d5ce68
Reviewed-on: https://chromium-review.googlesource.com/815154
Commit-Ready: Steven Bennetts <stevenjb@chromium.org>
Tested-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>

[modify] https://crrev.com/38b7e69faa51c9595cd8a2b429a2b144173f650a/cli/cros/cros_chrome_sdk.py

Status: Fixed (was: Started)
Status: Archived (was: Fixed)

Sign in to add a comment