updating build/download_nacl_toolchains.py GYP_DEFINES code for GN

[Paweł Hajdan, Jr.]

Currently https://cs.chromium.org/chromium/src/build/download_nacl_toolchains.py?q=download_nacl_toolchains.py&sq=package:chromium&dr has several dependencies on GYP_DEFINES.

As part of https://bugs.chromium.org/p/chromium/issues/detail?id=756688 , I'd like to remove them.

I believe we could use gclient conditionals for this.

In general, I suggest we err on the side of downloading the nacl toolchain if in doubt, to minimize chance of breakage.

The current code seems to specify following:

- skip download if nacl is disabled

- skip download for android

- also download pnacl if --optional-pnacl is passed and environment BUILDBOT_BUILDERNAME contains both "pnacl" and "sdk" (whoa!)

- if not on arm, skip nacl_arm_newlib

Since all this happens at gclient runhooks, we'd need gclient variables to control this.

In addition, we may want to emit the gclient variables for use in GN using gclient_gni_file and gclient_gni_args, so that we can perform additional sanity checks in GN.

To keep things simple, I suggest we introduce a gclient var called "download_nacl_toolchains", enabled by default. Then add a condition to only execute download_nacl_toolchains.py hook if the flag is enabled.

This will make it possible to explicitly disable downloading the toolchain in specific cases, but in practice would likely make us download it on more occasions than we currently do. My understanding is there'd be no big harm from that, but feel free to share any information that'd indicate otherwise.

This is just an initial informal thread, and if more discussion or specifics is needed, I'd likely move to a short doc. Please let me know if there are any questions you'd like answered.

Paweł

[Dirk Pranke]

+dschuff ...

The --optional-pnacl thing is interesting; I wasn't aware of that before. Derek or Bradley, can you comment on what's going on there and what's still needed? We need to move away from looking at env vars (particularly builder names) in the scripts.

I think "download_nacl_toolchains" sounds fine, though I would like for us to make sure we're thinking about naming conventions for these flags.

-- Dirk 

[Nico Weber]

It was my impression that the gclient conditionals are supposed to match gn args. Is that not the case?

If so, could we just key this off enable_nacl?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEoffTAXczssXU%3D5FENfFDV8Usk2g-hLx1H_qAJ2-5r_hmzQxQ%40mail.gmail.com.

[Derek Schuff]

[from the right address this time]

--optional-pnacl dates back from before pnacl was launched, but after nacl was launched, and was designed to avoid anyone having to download more toolchains if they weren't going to use them. We don't need it anymore, we can probably just condition everything off of enable_nacl, and get all the toolchains or none of them.

[Dirk Pranke]

On Fri, Sep 22, 2017 at 1:41 PM, Nico Weber <tha...@chromium.org> wrote:

It was my impression that the gclient conditionals are supposed to match gn args. Is that not the case?

If so, could we just key this off enable_nacl?

Given that a single checkout may be used for multiple builds that may have significantly different configurations, the dependency (in a sense) goes the other way. 

In other words, if you don't download the nacl toolchains, you can't set enable_nacl to true in any build dir (since it simply won't work). However, if you did download the nacl toolchains, that shouldn't mean that you can't still set enable_nacl to false in one build dir and enable_nacl=true in another.

Things will work similarly for a single checkout that might be used for both linux and android builds, and so on.

You might think that we could make it so that every build in a given checkout might be required to have to same set of "gclient/gn args" (i.e., args that affect both gclient and gn), and that life would be significantly simpler as a result, but I'm not actually sure that that's desirable or even viable, given that we need many of the conditions for cross-compiles (e.g., linux sysroots *and* android SDKs).

This is why I am concerned about naming conventions (or, put differently, maybe I should've been clearer about the underlying issues that the naming conventions reflect).

One of my design goals with the conditional work was to make it easier to have a single checkout that worked for multiple (significantly different) builds, not harder, because our checkouts just keep getting bigger and bigger. There are other ways to work around having multiple checkouts (e.g., using git-cache to avoid multiple copies of the repos), and so I could be convinced that this is unnecessary complexity, but I suspect that we will displease some people no matter what we end up with.

-- Dirk

[Nico Weber]

Hm, we already have .gclient to configure what we target (which I think most people don't edit a lot), and args.gn (which I think is heavily used). I think it'd be good to try to not introduce a 3rd mechanism.

For example, maybe DEPS pulling could be done by a build step instead of by `gclient sync`, then each build output directory would do this correctly on its own based on its arg (and they'd all download to the same place, so that if two build dirs need e.g. the android sdk, we'd only download it once)?

[Dirk Pranke]

On Fri, Sep 22, 2017 at 2:49 PM, Nico Weber <tha...@chromium.org> wrote:

Hm, we already have .gclient to configure what we target (which I think most people don't edit a lot), and args.gn (which I think is heavily used). I think it'd be good to try to not introduce a 3rd mechanism.

The gclient conditional values are set in .gclient (alongside target_os). Given that we also have dependencies on N different env vars today, this work is actually trying to reduce the number of mechanisms.

For example, maybe DEPS pulling could be done by a build step instead of by `gclient sync`, then each build output directory would do this correctly on its own based on its arg (and they'd all download to the same place, so that if two build dirs need e.g. the android sdk, we'd only download it once)?

I'm not sure I fully follow you here, but this sounds like you'd do something like a `git clone` at first plus N different `gclient syncs` (one for each build directory) and hope (?) that they were all compatible? What if they weren't?

[Nico Weber]

On Fri, Sep 22, 2017 at 6:27 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Fri, Sep 22, 2017 at 2:49 PM, Nico Weber <tha...@chromium.org> wrote:

Hm, we already have .gclient to configure what we target (which I think most people don't edit a lot), and args.gn (which I think is heavily used). I think it'd be good to try to not introduce a 3rd mechanism.

The gclient conditional values are set in .gclient (alongside target_os). Given that we also have dependencies on N different env vars today, this work is actually trying to reduce the number of mechanisms.

For example, maybe DEPS pulling could be done by a build step instead of by `gclient sync`, then each build output directory would do this correctly on its own based on its arg (and they'd all download to the same place, so that if two build dirs need e.g. the android sdk, we'd only download it once)?

I'm not sure I fully follow you here, but this sounds like you'd do something like a `git clone` at first plus N different `gclient syncs` (one for each build directory) and hope (?) that they were all compatible? What if they weren't?

Can we have incompatible syncs? I thought each sync config basically adds stuff, so I would've thought we can't really have conflicts -- you just might have to download some packages for one config that you don't need for the other (but that don't hurt for the other either). Is that not correct?

[Dirk Pranke]

On Fri, Sep 22, 2017 at 3:29 PM, Nico Weber <tha...@chromium.org> wrote:

On Fri, Sep 22, 2017 at 6:27 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Fri, Sep 22, 2017 at 2:49 PM, Nico Weber <tha...@chromium.org> wrote:

Hm, we already have .gclient to configure what we target (which I think most people don't edit a lot), and args.gn (which I think is heavily used). I think it'd be good to try to not introduce a 3rd mechanism.

The gclient conditional values are set in .gclient (alongside target_os). Given that we also have dependencies on N different env vars today, this work is actually trying to reduce the number of mechanisms.

For example, maybe DEPS pulling could be done by a build step instead of by `gclient sync`, then each build output directory would do this correctly on its own based on its arg (and they'd all download to the same place, so that if two build dirs need e.g. the android sdk, we'd only download it once)?

I'm not sure I fully follow you here, but this sounds like you'd do something like a `git clone` at first plus N different `gclient syncs` (one for each build directory) and hope (?) that they were all compatible? What if they weren't?

Can we have incompatible syncs? I thought each sync config basically adds stuff, so I would've thought we can't really have conflicts -- you just might have to download some packages for one config that you don't need for the other (but that don't hurt for the other either). Is that not correct?

It is theoretically possible to have incompatible configurations (e.g., to check out a repo at two different revisions), but we don't want to support that, and part of this work has been making it possible to enforce that we don't have this going forward.

That said, I was talking more about the incompatibility between "enable_nacl=true" and "enable_nacl=false". And, trying to synthesize what to do from N different build directories like this is possible (and was considered; I think I even started with that in mind, but it was more than a year ago, so I don't really recall).

But, it's a bigger jump from how we do things today, since we don't have `git clone` and `gclient sync` as two different steps, either for developers or on the bots. I do want to get to something like that model eventually, but we have a bunch of work to do to get there, and switching to conditionals (the way they are currently implemented) is a step along that path. In the meantime, I do think that having "checkout args" and "N sets of build args" is an improvement over "checkout args" and "N sets of build args" and "M different env vars that might be set inconsistently in L different environments."  I think once we've switched everything over to using conditionals consistently, and can talk about `git clone` potentially replacing `gclient sync` as a first step, we'll be able to have a more informed discussion about which we'd prefer.

-- Dirk

[Paweł Hajdan, Jr.]

I posted https://chromium-review.googlesource.com/c/chromium/src/+/681854 so we have something specific to discuss.

There's a bit of ugliness that the variable is passed to GN as a string. I was thinking we could provide a way for gclient_gn_args to specify types, and that way pass the variable as a bool.

Some questions in this thread seem to have scope larger than just handling nacl GYP_DEFINES. I suggest either to comment on the design doc, or fork the thread.

Paweł

[Dirk Pranke]

On Mon, Sep 25, 2017 at 8:25 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I posted https://chromium-review.googlesource.com/c/chromium/src/+/681854 so we have something specific to discuss.

Thanks!

 

There's a bit of ugliness that the variable is passed to GN as a string. I was thinking we could provide a way for gclient_gn_args to specify types, and that way pass the variable as a bool.

Yeah, let's see if we can figure out a way to avoid that (in a different thread), before it gets too embedded and hard to change.

-- Dirk

[Paweł Hajdan, Jr.]

On Mon, Sep 25, 2017 at 7:03 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Mon, Sep 25, 2017 at 8:25 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

There's a bit of ugliness that the variable is passed to GN as a string. I was thinking we could provide a way for gclient_gn_args to specify types, and that way pass the variable as a bool.

Yeah, let's see if we can figure out a way to avoid that (in a different thread), before it gets too embedded and hard to change.

See https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/681705 .

Also posted an update on https://chromium-review.googlesource.com/c/chromium/src/+/681854 and started a private email thread on this.

Paweł