Proposal: stop using sources_assignment_filter in chrome BUILD.gn

[Sylvain Defresne]

GN has support for filtering assignments to "sources" variables of every target. This mis-feature was there to help the migration process from GYP, however the GN developers would like to remove the feature as it is confusing and has negative impact on performance (see discussion on gn-dev).

As Chrome is the largest (or sole) user of this feature, it first must be fixed to no longer use it. The migration can be done incrementally by looking at assignments to sources and moving filtered files to target specific assignments.

For example, the following assignment:

sources = [
  "file_version_info.h",
  "file_version_info_mac.h"
  "file_version_info_mac.mm",
  "file_version_info_win.cc",
  "file_version_info_win.h",
]

would become:

sources = [
  "file_version_info.h"
]

if (is_mac) {
  sources += [

    "file_version_info_mac.h",

    "file_version_info_mac.mm",

  ]

}

if (is_win) {

  sources += [

    "file_version_info_win.cc",

    "file_version_info_win.h",

  ]

}

The migration can be performed incrementally (as the code with conditionals will work fine if sources_assignment_filter is used). A modified version of GN can be used to print the list of files that are filtered by the rules to help with the migration.

To get a sense of the kind of change this would cause to the BUILD.gn files, I've done the conversion of //base and uploaded it as changelist 1879458. I've also created issue 1018739 to track the removal if we decide to proceed.

Before moving further, I think we (chromium developers) should discuss whether we want to do this migration or not. So WDYT?

-- Sylvain

[Christian Biesinger]

I think this is a great change to make. It makes it easier to understand build files because you don't have to be aware of the magic that removes some files from the list.

Christian

--
--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJauWCEnkMfjVfwd3Ny-QvV4f97GgNtt9aHObD0%2BZwYspR2LCg%40mail.gmail.com.

[Chris Hamilton]

My opinion is that the migration makes sense because the OS-dependent suffix filtering is a little bit too "magic" and it is surprising when you learn about that feature.  I think the code with the explicit conditionals is easier to read.

--

[Simeon Anfinrud]

Huge +1, I work on Android-specific code and I can't tell you how many times people have asked my why something wasn't compiling only for it to turn out that the sources_assignment_filter deep magic was invisibly messing with the source list.

[Nico Weber]

I believe there's widespread agreement that we should do this; it's just that doing it is a lot of work.

It's true that this can be done incrementally, but what's going to stop someone from adding back something relying on set_sources_assignments_filter to base/BUILD.gn after your change goes in? Or do you expect that this will happen rarely enough that we can convert build files much faster than regressions creep in?

How long would you estimate did it take you to do the CL for base/? How many BUILD files are there? (i.e. do we need tooling to be able to do this manually, or will we finish fast enough if we keep doing this manually?)

--

[Sylvain Defresne]

Those are great questions.

On Mon, Oct 28, 2019 at 5:24 PM Nico Weber <tha...@chromium.org> wrote:

I believe there's widespread agreement that we should do this; it's just that doing it is a lot of work.

It's true that this can be done incrementally, but what's going to stop someone from adding back something relying on set_sources_assignments_filter to base/BUILD.gn after your change goes in? Or do you expect that this will happen rarely enough that we can convert build files much faster than regressions creep in?

IIUC, each BUILD.gn file is parsed independently, so if we call set_sources_assignment_filter() with an empty list at the top of each BUILD.gn file after it has been converted, then it would not be possible to add back something dependending on the filter in the file. Combined with a presubmit to prevent removal or modification of those calls, this should be enough to protect against regression.

 

How long would you estimate did it take you to do the CL for base/? How many BUILD files are there? (i.e. do we need tooling to be able to do this manually, or will we finish fast enough if we keep doing this manually?)

The CL for base/ tooks me a couple of hours to write (plus an extra hour to create a version of GN that printed the list of filtered out files). In total it took me less than an afternoon to write the patch to GN and the CL.

Using a crude regex to find BUILD.gn and .gni files with pattern looking like something that could be filtered out, I get a list of 500+ files. The command used is the following (I filtered out path containing ios/ as I know those files don't need sources_assignment_filter as they are only ever build and referenced when targeting ios):
$ git grep -l -E '(_(ios|win|mac|cocoa|chromeos|linux|posix)(\.|_unittest)|\b(ios|win|mac|cocoa|chromeos|linux|posix)/)' -- '*.gn' '*.gni'|grep -v 'ios/'

 

I would expect this list of files to be an upper bound of the files to be modified (since some of those BUILD.gn files are already in directories that match the name of one of the platforms, like chromeos/, so I would expect them to not depend on sources_assignment_filter). I don't know how complex each of those files will be to convert however, so I cannot yet determine whether we can do the conversion manually or if we need tooling.

-- Sylvain

[Andrew Grieve]

Android templates abuse this feature to perform list filtering, so we'll need to add a proper filter() function to GN before fully removing it (or just whitelist these usages).

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJauWCH2UfJWKgRbHd4kv9LNBF6BO2-h7qsFK2qfuA3X%2BdBuTA%40mail.gmail.com.

[Brett Wilson]

On Mon, Oct 28, 2019 at 9:51 AM Sylvain Defresne <sdef...@chromium.org> wrote:

Those are great questions.

On Mon, Oct 28, 2019 at 5:24 PM Nico Weber <tha...@chromium.org> wrote:

I believe there's widespread agreement that we should do this; it's just that doing it is a lot of work.

It's true that this can be done incrementally, but what's going to stop someone from adding back something relying on set_sources_assignments_filter to base/BUILD.gn after your change goes in? Or do you expect that this will happen rarely enough that we can convert build files much faster than regressions creep in?

IIUC, each BUILD.gn file is parsed independently, so if we call set_sources_assignment_filter() with an empty list at the top of each BUILD.gn file after it has been converted, then it would not be possible to add back something dependending on the filter in the file. Combined with a presubmit to prevent removal or modification of those calls, this should be enough to protect against regression.

 

How long would you estimate did it take you to do the CL for base/? How many BUILD files are there? (i.e. do we need tooling to be able to do this manually, or will we finish fast enough if we keep doing this manually?)

The CL for base/ tooks me a couple of hours to write (plus an extra hour to create a version of GN that printed the list of filtered out files). In total it took me less than an afternoon to write the patch to GN and the CL.

Using a crude regex to find BUILD.gn and .gni files with pattern looking like something that could be filtered out, I get a list of 500+ files. The command used is the following (I filtered out path containing ios/ as I know those files don't need sources_assignment_filter as they are only ever build and referenced when targeting ios):
$ git grep -l -E '(_(ios|win|mac|cocoa|chromeos|linux|posix)(\.|_unittest)|\b(ios|win|mac|cocoa|chromeos|linux|posix)/)' -- '*.gn' '*.gni'|grep -v 'ios/'

 

I would expect this list of files to be an upper bound of the files to be modified (since some of those BUILD.gn files are already in directories that match the name of one of the platforms, like chromeos/, so I would expect them to not depend on sources_assignment_filter). I don't know how complex each of those files will be to convert however, so I cannot yet determine whether we can do the conversion manually or if we need tooling.

 

My impression is that base/ should be the hardest file by far and if I was doing it wouldn't bother with tooling.

Brett

[Wez]

This seems a good idea to me.  As Brett suggests, getting owners to manually migrate their GN rules not to rely on the filters is probably less work overall than trying to automate it.

Is it possible for us to switch the files that rely on the current filter to be opt-in, rather than making it an opt-out once things are migrated? That would help prevent new usage creeping in via new BUILD files, and make the "magic" more visible in the meantime.

--

--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CABiGVV9jgSxQexceHOq1_7HSAc33XrPj_pv6ELMyvHpjG5jVXg%40mail.gmail.com.

[Brett Wilson]

On Mon, Oct 28, 2019 at 10:32 AM Wez <w...@chromium.org> wrote:

This seems a good idea to me.  As Brett suggests, getting owners to manually migrate their GN rules not to rely on the filters is probably less work overall than trying to automate it.

I would actually just have 1-2 people do it for the whole codebase. Trying to find owners willing to do this kind of thing over the whole codebase and harassing them until they actually do it is probably almost as much work.

 

Is it possible for us to switch the files that rely on the current filter to be opt-in, rather than making it an opt-out once things are migrated? That would help prevent new usage creeping in via new BUILD files, and make the "magic" more visible in the meantime.

Yes, you can remove the call to set_sources_assignment_filter() in BUILDCONFIG and add set_sources_assignment_filter(sources_assignment_filter) to individual build files. But if you're going to go through all the build files that use this feature, you might as well just fix them. Most individual files' changes are trivial. There are probably only a handful of exceptions.

Brett

[Matthew Menke]

Strong +1 to doing this.  The fact that I can't use out/android or out/chromeos for destination folders because of this feature also makes me sad.  This seems an easy enough migration for individual owned directories that it may be useful just to publicly ask people to migrate their stuff first, and then have the aforementioned 1-2 people do everything else after waiting a week or two?

At least this is enough of an improvement and a sufficiently fast one that I'm happy to spend some time fixing up the build.gn files that I'm an owner of to not rely on the filter.

[Dirk Pranke]

On Mon, Oct 28, 2019 at 6:28 PM 'Matthew Menke' via Chromium-dev <chromi...@chromium.org> wrote:

Strong +1 to doing this.  The fact that I can't use out/android or out/chromeos for destination folders because of this feature also makes me sad.

Side note: I filed https://crbug.com/gn/125 to fix this particular aspect of things, which I think we could do w/o needing to do all of the source_assignment_filter stuff.

Which (mostly) ended up spawning this discussion.

I support getting rid of the filter, though.

-- Dirk

 

--

--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/ddb15e91-9aee-42bb-aa1e-b889c210ab02%40chromium.org.

[Sylvain Defresne]

In order to close the loop, I just wanted to announce that Chromium no longer uses sources assignment filter (and soon the last no-op calls to set_sources_assignment will be removed).

All BUILD.gn files in chromium must now use an `if` statement to add platform specific files to sources.

This means that you can already use out/linux, out/android, out/ios, ...

Thanks to all the people that reviewed my CLs and helped with the conversion of individual gn files.

-- Sylvain

[Dirk Pranke]

Congrats! A large amount of painstaking work, but it'll make things much less magical and surprising in the build :).

-- Dirk

[Robert Sesek]

Wow—thank you for driving this project to completion, Sylvain!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAEoffTDrt7UU92owTKB7VE3DsuP_ugNLEhN91HW0S3Nj1zwEJQ%40mail.gmail.com.

[Wez]

Thanks for cleaning this up!

[Fabrice de Gans-Riberi]

Thanks for doing this work!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CALekkJcP7DpLMR2c12VhQsvDxPfPwLTPrS3mGk-H2nMUGrRntw%40mail.gmail.com.

[John Abd-El-Malek]

+1, thank you. The less magic and fewer ways of doing things in build files the better!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAJ0M83XsQ9EK2TecRGJ%3Dyh_0CvtouV4kXS9Txef_5FosUS-sYw%40mail.gmail.com.