Design proposal: Introducing toolchain_import to gn

[Sven Zheng]

Hi gn dev,

I want to make a design proposal for gn. Please review and leave feedback. Doc access has been given to all chromium@ accounts. If you can't access it, please request.

https://docs.google.com/document/d/1YH9NYmFhlSwU-6eEg08FqPoUo1hq7uSG0bYIBWLvhVA/edit?usp=sharing

An example for the new keyword:

Say we have a toolchain defined

some_toolchain(“demo_toolchain”) {

  toolchain_args {

    toolchain_import = “//build/args/demo.gni”

    use_goma = true

  }

}

The //build/args/demo.gni contains:

demo_flag = “demo”

use_goma = false

The definition will be equivalent to:

some_toolchain(“demo_toolchain”) {

  toolchain_args {

    demo_flag = “demo”

    use_goma = true

  }

}

[Roland McGrath]

You can use `import` inside a scope object:

toolchain_args = {

  import("//build/args/demo.gni")

  use_goma = true
}

How does your new feature differ from just doing that?

[Sven Zheng]

Please correct me if I'm wrong. It's different in several ways:

1 The gni file here only contains key value pairs.

It's similar to what you define args.gn file, like:

use_some_flag = "some content"

other_flag = true

But for 'import', a new variant needs to be defined in declare_args().

2 Won't raise error for conflicts

For 'import', it will raise error. But for this 'toolchain_import', it will ignore if the key already existed.

[Dirk Pranke]

Hmm, I guess it didn't occur to me that maybe you could do an import directly in toolchain_args already. Is the scope of the import confined to just toolchain_args correctly?

On Tue, Nov 23, 2021 at 5:28 PM Sven Zheng <sven...@chromium.org> wrote:

Please correct me if I'm wrong. It's different in several ways:

1 The gni file here only contains key value pairs.

It's similar to what you define args.gn file, like:

use_some_flag = "some content"

other_flag = true

But for 'import', a new variant needs to be defined in declare_args().

I'm not following you here, Sven?

 

2 Won't raise error for conflicts

For 'import', it will raise error. But for this 'toolchain_import', it will ignore if the key already existed.

I'm not sure if I'm following you here, either, so I'm not sure what behavior you think should work and what should be an error?

At a glance, I would think Roland's suggestion would just do what you want.

-- Dirk

On Tue, Nov 23, 2021 at 4:50 PM Roland McGrath <mcgr...@chromium.org> wrote:

You can use `import` inside a scope object:

toolchain_args = {

  import("//build/args/demo.gni")

  use_goma = true
}

How does your new feature differ from just doing that?

On Tue, Nov 23, 2021 at 3:54 PM Sven Zheng <sven...@chromium.org> wrote:

Hi gn dev,

I want to make a design proposal for gn. Please review and leave feedback. Doc access has been given to all chromium@ accounts. If you can't access it, please request.

https://docs.google.com/document/d/1YH9NYmFhlSwU-6eEg08FqPoUo1hq7uSG0bYIBWLvhVA/edit?usp=sharing

An example for the new keyword:

Say we have a toolchain defined

some_toolchain(“demo_toolchain”) {

  toolchain_args {

    toolchain_import = “//build/args/demo.gni”

    use_goma = true

  }

}

The //build/args/demo.gni contains:

demo_flag = “demo”

use_goma = false

The definition will be equivalent to:

some_toolchain(“demo_toolchain”) {

  toolchain_args {

    demo_flag = “demo”

    use_goma = true

  }

}

--
To unsubscribe from this group and stop receiving emails from it, send an email to gn-dev+un...@chromium.org.

[Roland McGrath]

You can indeed do import inside any scope and it's just like defining those variables in that scope any other way.

What I overlooked is that the proposal apparently plumbs things through without making them build arguments with `declare_args()`, if I'm following Sven.

So I think what this is actually equivalent to is something like BUILDCONFIG.gn doing:

```

declare_args() {

  toolchain_imports = []

}

foreach(file, toolchain_imports) {
  import(file)

}
```

It is nice to avoid the wart of `toolchain_imports` being available as a build argument that can be set in `args.gn`.  IMHO it would also be very nice to avoid the wart of it not being set in the default toolchain evaluation of BUILDCONFIG.gn, but I don't think that can be avoided without deeper semantic changes to GN (which I think we've discussed before).

I'm not sure there's any special reason to restrict the feature to imports rather than just a scope.  That is, the equivalent of BUILDCONFIG.gn doing:

```

declare_agrs() {

  toolchain_globals = {}

}

forward_variables_from(toolchain_globals, "*")

```

You can of course always do:

```

  toolchain_globals = {

    import("file.gni")

  }

```

if you like.

But then again the proposal example says it's equivalent to just setting variables in `toolchain_args`, which only works with declared build arguments.  So perhaps I don't understand the proposal.  The example use case looks to me like it's just looking for: 

```

  toolchain_args = {

    import("file.gni")
    override_some_setting = true

  }

```

and I'd expect that to work today.

[Dirk Pranke]

On Tue, Nov 23, 2021 at 7:24 PM Roland McGrath <mcgr...@chromium.org> wrote:

You can indeed do import inside any scope and it's just like defining those variables in that scope any other way.

What I overlooked is that the proposal apparently plumbs things through without making them build arguments with `declare_args()`, if I'm following Sven.

I'm not seeing that in the proposal; if I read it correctly, toolchain_import produced behavior identical to just specifying the args directly with toolchain_args. So, I think you're right and maybe just using import() directly is enough, but I'll wait for Sven to reply to see if we're on the same page yet.

-- Dirk

[Sven Zheng]

Thank you Dirk and Roland for the reply. Here's the real world example, if I change the design doc's 'toolchain_import = "//build/args/chromeos/amd64-generic.gni"' to 'import("//build/args/chromeos/amd64-generic.gni")' statement.

$ autoninja -C out/lacrosdut chrome
ninja: Entering directory `out/lacrosdut'
[0/1] Regenerating ninja files
ERROR at //build/toolchain/cros/BUILD.gn:198:5: Value collision.
    import("//build/args/chromeos/amd64-generic.gni")
    ^-----------------------------------------------
This import contains "rbe_cros_cc_wrapper"
See //build/args/chromeos/amd64-generic.gni:52:23: defined here.
rbe_cros_cc_wrapper = "/usr/local/google/home/svenzheng/chromium2/src/build/args/chromeos/rewrapper_amd64-generic"
                      ^-------------------------------------------------------------------------------------------
Which would clobber the one in your current scope
See //build/toolchain/rbe.gni:26:25: defined here.
  rbe_cros_cc_wrapper = ""
                        ^-
Executing import should not conflict with anything in the current
scope unless the values are identical.
See //BUILD.gn:260:7: which caused the file to be included.
      "//third_party/catapult/telemetry:bitmaptools($host_toolchain)",

If I comment out the rbe_cros_cc_wrapper, the failure will become:

$ autoninja -C out/lacrosdut chrome
ninja: Entering directory `out/lacrosdut'
[0/1] Regenerating ninja files
ERROR at //build/toolchain/cros/BUILD.gn:198:5: Value collision.
    import("//build/args/chromeos/amd64-generic.gni")
    ^-----------------------------------------------
This import contains "use_goma"
See //build/args/chromeos/amd64-generic.gni:62:12: defined here.
use_goma = false
           ^----
Which would clobber the one in your current scope
See build arg file (use "gn args <out_dir>" to edit):8:10: defined here.
use_goma=true
         ^---
Executing import should not conflict with anything in the current
scope unless the values are identical.
See //BUILD.gn:260:7: which caused the file to be included.
      "//third_party/catapult/telemetry:bitmaptools($host_toolchain)",
      ^--------------------------------------------------------------
FAILED: build.ninja

As I said, the current 'import' would report failure, but 'toolchain_import' would only set the arg if the current 'toolchain_args' doesn't.

[Roland McGrath]

That doesn't look like what I'd expect from using import inside a scope object.  But it's really hard to evaluate without seeing your real GN code.

[Sven Zheng]

This is the prototype cl:

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

gn args:

import("//build/args/chromeos/amd64-generic-crostoolchain.gni")
target_os="chromeos"
is_chromeos_device=true
chromeos_is_browser_only=true
cros_host_sysroot="//build/linux/debian_sid_amd64-sysroot"
cros_v8_snapshot_sysroot="//build/linux/debian_sid_amd64-sysroot"
use_goma=true
is_debug=false
also_build_ash_chrome_dut=true

[Roland McGrath]

Sorry, but I still don't see the file //build/args/chromeos/amd64-generic.gni anywhere so I can't tell what it's doing.

[Sven Zheng]

It's generated when run 'gclient sync'. Mostly set args defined in https://source.chromium.org/chromium/chromium/src/+/main:build/toolchain/cros_toolchain.gni;l=38;drc=9869e86fd9079a6ab4ea23aa03d724580678b356

Here's what my file looks like:

$ cat build/args/chromeos/amd64-generic-crostoolchain.gni
cros_board = "amd64-generic"
cros_host_cc = "/usr/local/google/home/svenzheng/chromium2/src/third_party/llvm-build/Release+Asserts/bin/clang"
cros_host_cxx = "/usr/local/google/home/svenzheng/chromium2/src/third_party/llvm-build/Release+Asserts/bin/clang++"
cros_host_extra_cflags = " -Wno-unknown-warning-option"
cros_host_extra_cppflags = ""
cros_host_extra_cxxflags = " -Wno-unknown-warning-option"
cros_host_extra_ldflags = " --unwindlib=libgcc"
cros_host_ld = "/usr/local/google/home/svenzheng/chromium2/src/third_party/llvm-build/Release+Asserts/bin/clang++"
cros_sdk_version = "14335.0.0"
cros_target_ar = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+target_toolchain/bin/llvm-ar"
cros_target_cc = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+target_toolchain/bin/x86_64-cros-linux-gnu-clang"
cros_target_cxx = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+target_toolchain/bin/x86_64-cros-linux-gnu-clang++"
cros_target_extra_cflags = "-pipe -march=x86-64 -msse3 -ffunction-sections -fdata-sections -faddrsig -Wno-unknown-warning-option"
cros_target_extra_cppflags = ""
cros_target_extra_cxxflags = "-pipe -march=x86-64 -msse3 -ffunction-sections -fdata-sections -D__google_stl_debug_vector=1 -faddrsig -Wno-unknown-warning-option -stdlib=libc++"
cros_target_extra_ldflags = "-Wl,-O2 -Wl,--as-needed -Wl,--gc-sections -Wl,--icf=all -Wl,-mllvm -Wl,-generate-type-units -stdlib=libc++"
cros_target_ld = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+target_toolchain/bin/x86_64-cros-linux-gnu-clang++"
cros_target_nm = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+target_toolchain/bin/llvm-nm"
cros_target_readelf = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+target_toolchain/bin/llvm-readelf"
cros_v8_snapshot_cc = "/usr/local/google/home/svenzheng/chromium2/src/third_party/llvm-build/Release+Asserts/bin/clang"
cros_v8_snapshot_cxx = "/usr/local/google/home/svenzheng/chromium2/src/third_party/llvm-build/Release+Asserts/bin/clang++"
cros_v8_snapshot_extra_cflags = " -Wno-unknown-warning-option"
cros_v8_snapshot_extra_cppflags = ""
cros_v8_snapshot_extra_cxxflags = " -Wno-unknown-warning-option"
cros_v8_snapshot_extra_ldflags = " --unwindlib=libgcc"
cros_v8_snapshot_ld = "/usr/local/google/home/svenzheng/chromium2/src/third_party/llvm-build/Release+Asserts/bin/clang++"
custom_toolchain = "//build/toolchain/cros:target"
host_pkg_config = "x86_64-pc-linux-gnu-pkg-config"
host_toolchain = "//build/toolchain/cros:host"
is_clang = true
system_libdir = "lib64"
target_cpu = "x64"
target_sysroot = "/usr/local/google/home/svenzheng/chromium2/src/build/cros_cache/chrome-sdk/symlinks/amd64-generic+14335.0.0+sysroot_chromeos-base_chromeos-chrome.tar.xz"
v8_snapshot_toolchain = "//build/toolchain/cros:v8_snapshot"

[Roland McGrath]

OK, if it looks like that--nothing but global assignments--then I
think errors on import for this case is a GN bug.  An explicit scope
object does not inherit from the lexical scope where it's defined, so
it doesn't make sense to give those override/shadow sorts of errors
IMHO.  I think there may be the same bug with the "don't overwrite
nonempty aggregates" error where it's also not really applicable, but
in that case there's a workaround of defining the scope entry with the
empty list or scope value first.  Since there is no such workaround
for the import error, this should be impetus to fix the whole class of
bugs uniformly IMHO.

I said that, and I think we should fix it.  But I suspect you might
actually be allowed to work around that with:
```
  scope = {
    forward_variables_from(read_file("file.gni", "scope"), "*")
  }
```
The read_file lacks the read/parse-only-once behavior of import, but
it lets you use the forward_variables_from "*" special exception to
the overwrite rules.

[Dirk Pranke]

I'm inclined to agree with Roland, but the code where this checking is being done is kinda central to GN and it's been around forever, so I'd want to check w/ brettw@ to see if he knows of a reason we want this to be an error.

-- Dirk

[Brett Wilson]

\On Wed, Dec 1, 2021 at 1:09 PM Dirk Pranke <dpr...@google.com> wrote:

I'm inclined to agree with Roland, but the code where this checking is being done is kinda central to GN and it's been around forever, so I'd want to check w/ brettw@ to see if he knows of a reason we want this to be an error.

This just means that we would only check for collisions of variables set on the scope directly, not inherited from the enclosing scopes, right? That sounds OK to change to me.

BRett

[Sven Zheng]

Thank you for all the suggestions. I'll need to do some more readings and experiments, then come back to reply.

[Roland McGrath]

On Wed, Dec 1, 2021 at 2:47 PM Brett Wilson <bre...@google.com> wrote:

\On Wed, Dec 1, 2021 at 1:09 PM Dirk Pranke <dpr...@google.com> wrote:

I'm inclined to agree with Roland, but the code where this checking is being done is kinda central to GN and it's been around forever, so I'd want to check w/ brettw@ to see if he knows of a reason we want this to be an error.

This just means that we would only check for collisions of variables set on the scope directly, not inherited from the enclosing scopes, right? That sounds OK to change to me.

To be precise, it would check only for actual collisions.  That is, in a context where the current scope does inherit lexically, then collisions with inherited values would be considered, since those values are considered "defined" in the current scope.  In a context where the current scope does not inherit lexically (i.e. `= { ... }` syntax and the like), then collisions with lexically-visible values not defined in the current scope would not be considered. 

[Brett Wilson]

If that's easy and clean to implement, that would be even better. But I think it will  be difficult to implement since it's the reader of the scope that controls whether to read the inherited flags and that's not known when the scope is being created (you can still read globals from within that scope, so the code inside it actually is inheriting.

Brett

[Sven Zheng]

Roland's proposal using 'read_file' works for me. I went with that approach. For the import bug, I think I'll leave it to gn experts.

Thank you all!