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

Issue 605154 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 604971



Sign in to add a comment

Switch SimpleChrome builders to use GN

Project Member Reported by steve...@chromium.org, Apr 20 2016

Issue description

Cc: phajdan.jr@chromium.org d...@chromium.org
+phajdan.jr@chromium.org, dnj@chromium.org for guidance

Blockedon: 604971
Status: Available (was: Assigned)
'gn gen' is currently failing for daisy,  issue 604971 , so we need to wait for that to be resolved before converting the daisy builder.

I've confirmed that 'gn gen' at least apepars to work for amd64-generic and x86-generic.

there's a couple of issues to consider in how we want to do this:

1) Do you want to invoke GYP or GN directly, or use the MB wrapper? the latter may provide some benefits, but not a lot, since you'll be passing the list of defines in.

2) Where do you want to control whether the bot uses GYP or GN? via a config in the .ebuild, or a setting on the bot, or a setting in the chrome checkout? Most chromium bots do the last thing, but this may be less convenient for CrOS infra.
1) I don't know what the 'MB wrapper' is.

2) I think we need to make it a setting in the bot, or change to the bot configuration file. I would like to switch a single SimpleChrome builder to GN initially. (Plus we can't switch daisy over until issue 605971 is resolved).


Cc: rjkroege@chromium.org
in comment #5, stevenjb wrote:
> 1) I don't know what the 'MB wrapper' is.

Good question (statement?) :).

The MB wrapper is a new python tool I wrote to help w/ the migration from GYP to GN in Chromium. 

https://chromium.googlesource.com/chromium/src/+/master/tools/mb

It's designed to hide the details of whether a given bot is using GYP or GN from the rest of the build infrastructure, and to handle the mapping between GYP and GN flags via entries in:

https://chromium.googlesource.com/chromium/src/+/master/tools/mb/mb_config.pyl

Nearly all of the non-CrOS bots have been updated so that they do not invoke build/gyp_chromium during `gclient runhooks`, and invoke mb in a separate step instead:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Builder/builds/38916/steps/generate_build_files/logs/stdio

This wrapper also implements various other things that the build infrastructure needs, like generating runtime dependencies for swarming from GN, etc.

It's possible we don't need much of that functionality in the simplechrome builds.

If we can assume that both GYP_DEFINES and GN_ARGS are set in the environment by cros, then if we can also get a third env var that says whether we want to use GYP or GN, then we can modify MB to just look at the env vars and do the right thing in this case, and this will fit most seamlessly into the rest of the chromium build recipes, so I'd probably suggest we start with that.
So... I spent a little bit of time browsing through mb.py, mb_config.pyl, and builders.pyl, attempting to tease out how the recipe for "ChromiumOS x86-generic Compile" is generated, and I confess it was... daunting.

I will probably need to be walked through this.

I think, what I would like is for us to be able to is:

1. Replace 'cros_chrome_sdk_gyp' in the mb_config.pyl entry for "ChromiumOS x86-generic Compile" (initially just that one) with 'cros_chrome_sdk_gn'.

2. Create a 'cros_chrome_sdk_gn' entry, which I guess would look like ['gn', 'error'] ?

3. Make that "just work". GN_ARGS should be defined correctly in the 'cros chrome-sdk' environment. We will need to run 'gn gen $BUILDIR --args=$GN_ARGS'. Everything should then 'just work'.

dpranke@ - will you have time to put something together or walk me through it? I'd love to get x86-generic switch over sooner than later.

Cheers,
Steven

Yup, I can put together something for you later today. From comment #7, it sounds like you're thinking that whether to use GYP or GN would be controlled by the entry in the mb_config.pyl file, rather than by a separate env var that was set in the ebuild.

Is that correct? 
That looked to be reasonable to me?

I don't see us going back and forth, this will hopefully be a one way trip. We just need to be able to switch them over one at a time because:

a) I'd rather just risk breaking one initially :)

b)  issue 604971  will prevent us from switching over "ChromiumOS daisy Compile" until fixed.

Okay. 

I'm less certain how this is going to play out on the downstream bots; maybe we won't even call MB there ...
What do you mean by "downstream bots"?
actual cros-managed bots like (I think) https://build.chromium.org/p/chromiumos/waterfall that are doing full ebuilds.
Ah. Those (builders using the chroot) are a separate beast altogether. I will create a separate issue for converting those builders once we get the ebuilds working, but as I understand it we still have a fair bit of work to do there.

Here's what I have in mind; I think it should more-or-less Just Work:

https://codereview.chromium.org/1913703002/

We can try landing this and enabling MB on 'ChromiumOS x86-generic Compile' and see what happens, and go from there, if you like.
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 25 2016

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

commit 73ed0d6aa6f1c9c9a045090574d92493dc12307d
Author: dpranke <dpranke@chromium.org>
Date: Mon Apr 25 19:18:34 2016

Add support for cros chrome_sdk (simplechrome) to MB.

When we build Chrome for ChromeOS using the simplechrome
workflow, all of the GYP_DEFINES and gn args that are needed
are determined by the cros wrapper scripts, and rather than
duplicate that logic in the MB mixins, it's easier to just
pass the data through.

However, we don't want this passthrough mechanism to be abused,
so we implement so that this only works if the bot is configured
to allow for passthrough *and* the GYP_DEFINES or GN_ARGS env
vars contain chromeos=1 or target_os="chromeos" as appropriate.

If we need to be less strict later we can alway change things.

R=stevenjb@chromium.org
BUG= 605154 

Review URL: https://codereview.chromium.org/1913703002

Cr-Commit-Position: refs/heads/master@{#389532}

[modify] https://crrev.com/73ed0d6aa6f1c9c9a045090574d92493dc12307d/tools/mb/mb.py
[modify] https://crrev.com/73ed0d6aa6f1c9c9a045090574d92493dc12307d/tools/mb/mb_config.pyl

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 25 2016

Labels: merge-merged-2716
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73ed0d6aa6f1c9c9a045090574d92493dc12307d

commit 73ed0d6aa6f1c9c9a045090574d92493dc12307d
Author: dpranke <dpranke@chromium.org>
Date: Mon Apr 25 19:18:34 2016

Add support for cros chrome_sdk (simplechrome) to MB.

When we build Chrome for ChromeOS using the simplechrome
workflow, all of the GYP_DEFINES and gn args that are needed
are determined by the cros wrapper scripts, and rather than
duplicate that logic in the MB mixins, it's easier to just
pass the data through.

However, we don't want this passthrough mechanism to be abused,
so we implement so that this only works if the bot is configured
to allow for passthrough *and* the GYP_DEFINES or GN_ARGS env
vars contain chromeos=1 or target_os="chromeos" as appropriate.

If we need to be less strict later we can alway change things.

R=stevenjb@chromium.org
BUG= 605154 

Review URL: https://codereview.chromium.org/1913703002

Cr-Commit-Position: refs/heads/master@{#389532}

[modify] https://crrev.com/73ed0d6aa6f1c9c9a045090574d92493dc12307d/tools/mb/mb.py
[modify] https://crrev.com/73ed0d6aa6f1c9c9a045090574d92493dc12307d/tools/mb/mb_config.pyl

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 26 2016

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

commit 78685fb274d05ee48e5425dc2ce4f5ba7b2dce81
Author: stevenjb <stevenjb@chromium.org>
Date: Tue Apr 26 22:17:53 2016

Use GN in simple chrome builders

BUG= 605154 

Review URL: https://codereview.chromium.org/1918713005

Cr-Commit-Position: refs/heads/master@{#389914}

[modify] https://crrev.com/78685fb274d05ee48e5425dc2ce4f5ba7b2dce81/tools/mb/mb_config.pyl

Project Member

Comment 18 by bugdroid1@chromium.org, Apr 27 2016

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

commit 570d6f823fd119974967652479bf570e5002a362
Author: dpranke <dpranke@chromium.org>
Date: Wed Apr 27 06:07:18 2016

Revert of Use GN in simple chrome builders (patchset #1 id:1 of https://codereview.chromium.org/1918713005/ )

Reason for revert:
Well, I fixed a bunch of things, but it got late and I'm still hitting bugs, so I'm going to try to flip this back to GYP and see if that works, at least (through MB).

https://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20amd64-generic%20Compile/builds/17407

The last error makes it look like we might be missing a few targets that chromiumos_preflight needs, but I might've also just gotten the build labels wrong in the GN build.

I suggest we try again tomorrow and flip the trybots at the same time so we can be consistent and see the trybots working first.

Original issue's description:
> Use GN in simple chrome builders
>
> BUG= 605154 
>
> Committed: https://crrev.com/78685fb274d05ee48e5425dc2ce4f5ba7b2dce81
> Cr-Commit-Position: refs/heads/master@{#389914}

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

Review URL: https://codereview.chromium.org/1921003009

Cr-Commit-Position: refs/heads/master@{#389998}

[modify] https://crrev.com/570d6f823fd119974967652479bf570e5002a362/tools/mb/mb_config.pyl

Blocking: 432967
Status: Fixed (was: Available)
Project Member

Comment 21 by bugdroid1@chromium.org, Jun 20 2016

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

commit f67895d75bae253dda0f2c08beabdb3bcdc03252
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Mon May 16 19:09:19 2016

Use gn instead of GYP in SimpleChrome stage

BUG= chromium:605154 
TEST=SimpleChromeWorkflow stage succeeds and uses GN

Change-Id: I925d3bdfb251fbd63888d7122ca4e04009609fce
Reviewed-on: https://chromium-review.googlesource.com/344831
Reviewed-by: David James <davidjames@chromium.org>
Tested-by: Steven Bennetts <stevenjb@chromium.org>

[modify] https://crrev.com/f67895d75bae253dda0f2c08beabdb3bcdc03252/cbuildbot/stages/chrome_stages.py

Project Member

Comment 22 by bugdroid1@chromium.org, Jun 22 2016

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

commit 3ebb69aba9d917e130a5f10d365454e4f06d2f8e
Author: Steven Bennetts <stevenjb@chromium.org>
Date: Tue Jun 21 16:56:37 2016

Use gn instead of GYP in SimpleChrome stage (take 2)

BUG= chromium:605154 
TEST=SimpleChromeWorkflow stage succeeds and uses GN

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

[modify] https://crrev.com/3ebb69aba9d917e130a5f10d365454e4f06d2f8e/cbuildbot/stages/chrome_stages.py

This is fixed for real now - we are using GN in the SimpleChrome stage of the PFQ builders also now, hooray!

Awesome!
Blocking: -432967

Sign in to add a comment