proposal for changing sources_assignment_filter

[Dirk Pranke]

Hi all,

I've filed crbug.com/gn/118 for this, but cross-posting the idea here ...

set_sources_assignment_filter() is something of a misfeature in GN (ideally you wouldn't use it), but it's unlikely Chromium in particular will ever get around to actually using it, and the feature is in fact kinda handy for simplifying things.

However, it can have a weird side effect, in that if you define the filter list such that you filter on directory names (e.g., "win/*"), you can end up filtering out stuff you might not have wanted to filter out (e.g., everything if your checkout lives under /src/win/, or if you have a build output directory called //out/win).

I haven't been able to think of a reason you'd want sources_assignment_filter to apply to path components outside of the source tree, or for the components in the output directory, so what if we just changed the code to ignore those?

Does anyone see a downside to such a change, apart from it being non-backwards-compatible?

-- Dirk

[Brett Wilson]

I'd rather just delete the feature completely. I think it should be pretty straightforward.

It would probably take somebody on Chrome to do this since I think it's the only project using the feature. I would probably recommend hacking the GN source code to print out all the files it filters out, then committing changes to put those in the proper platform conditionals. I bet almost most of the targets that take advantage of the filter also have conditionals for other files for the same platforms. .

Brett

[Mirko Bonadei]

For what it's worth I would also suggest to just delete the feature completely. I think there is value in having things explicitly declared in BUILD.gn files, especially when someone new to the projects looks at them (in that case being explicit helps with the learning curve).

Agreed that the current situation should also be easy to undo.

[James Robinson]

We stopped using this feature early in the Mojo (standalone) and Fuchsia projects.  It took a small amount of work to make sure all of the rules we used (especially third_party) worked properly but it has proven very lightweight to maintain and a much simpler life to live in.  This includes projects where we use the same BUILD.gn files that Chromium uses.  Today there are 2 third_party projects in the Fuchsia tree that use non-empty assignment filters, one to filter a single file and another to filter out tests from a common list.

I realize the Chromium project's much much larger, but I think it'd be a worthwhile task to just stop using the feature.  It was useful in the early GN days to share file lists with GYP, but that hasn't been relevant for years now.

- James

On Mon, Oct 21, 2019 at 5:24 PM 'Dirk Pranke' via gn-dev <gn-...@chromium.org> wrote:

[Dirk Pranke]

Hm.

To me, I can see real value in having a single sources list where stuff gets auto-filtered out.

However, I can also see how it's confusing, and I can definitely see where it would be confusing if you *also* have the conditionals anyway.

E.g.

source_set("foo") {

  sources = [

    "foo.cc",

    "foo_win.cc",

    "foo_mac.cc",

    "foo_linux.cc",

  ]

}

is just cleaner than having to add three more if blocks. But if the if-blocks are there anyway -- and I grant that they probably are in every list in Chromium -- it's probably a net negative.

Realistically, I'm just unsure that we'll ever actually get rid of things, and I'd really like to eliminate the confusion and unnecessary bugs that crop up in the meantime.

So, apart from suggesting that we get rid of the feature altogether, does anyone see a different reason not to make the change I'm suggesting? 

-- Dirk

[James Robinson]

On Tue, Oct 22, 2019 at 12:10 PM Dirk Pranke <dpr...@google.com> wrote:

Hm.

To me, I can see real value in having a single sources list where stuff gets auto-filtered out.

However, I can also see how it's confusing, and I can definitely see where it would be confusing if you *also* have the conditionals anyway.

E.g.

source_set("foo") {

  sources = [

    "foo.cc",

    "foo_win.cc",

    "foo_mac.cc",

    "foo_linux.cc",

  ]

}

is just cleaner than having to add three more if blocks. But if the if-blocks are there anyway -- and I grant that they probably are in every list in Chromium -- it's probably a net negative.

I think the if blocks are much clearer, personally.  I haven't met many folks who can tell you if a _posix or _linux file is included in an Android build in the Chromium tree without looking it up.

I don't have an opinion on the semantics of the feature, or changing it.

- James

[Dirk Pranke]

On Tue, Oct 22, 2019 at 12:13 PM James Robinson <jam...@google.com> wrote:

On Tue, Oct 22, 2019 at 12:10 PM Dirk Pranke <dpr...@google.com> wrote:

Hm.

To me, I can see real value in having a single sources list where stuff gets auto-filtered out.

However, I can also see how it's confusing, and I can definitely see where it would be confusing if you *also* have the conditionals anyway.

E.g.

source_set("foo") {

  sources = [

    "foo.cc",

    "foo_win.cc",

    "foo_mac.cc",

    "foo_linux.cc",

  ]

}

is just cleaner than having to add three more if blocks. But if the if-blocks are there anyway -- and I grant that they probably are in every list in Chromium -- it's probably a net negative.

I think the if blocks are much clearer, personally.  I haven't met many folks who can tell you if a _posix or _linux file is included in an Android build in the Chromium tree without looking it up.

Well, yeah, I'll grant that not having a 1:1 mapping between suffix and where the file actually is used makes things much worse :).

-- Dirk

[Sylvain Defresne]

I also support removing the feature altogether as it makes life harder for "derivative" platforms. E.g. ios uses many "_mac.cc" files and the way to allow inclusion of those files is difficult. Also, you already have to have "if" when you have "_posix.cc" and a "_mac.cc" version of the same feature (since mac will include both). I also do not know if the feature interact with metadata.

About fixing the feature to ignore path outside of // and path below root_build_dir, I do think it would be good.

-- Sylvain

[Sylvain Defresne]

So I've converted chromium away from using set_sources_assignment_filter.

Would anyone object if I were to remove the feature from gn?

-- Sylvain

[Dirk Pranke]

No objection from me. Congrats on getting it done!

-- Dirk

[Brett Wilson]

Please make it so!

Brett

[Sylvain Defresne]

I found that there are still a few no-op calls to set_sources_assignment_filter to remove in third-party (from Chromium POV) repositories to take care,

I'll remove the support from gn as soon as possible.

-- Sylvain

[David Turner]

Le mer. 7 oct. 2020 à 17:19, Sylvain Defresne <sdef...@chromium.org> a écrit :

So I've converted chromium away from using set_sources_assignment_filter.

Would anyone object if I were to remove the feature from gn?

Yes, unfortunately, Fuchsia still uses it as of now (once in the Fuchsia build files,  and several times in third_party/ source trees). I've just filed https://fxbug.dev/61489 to keep track of removing these.

- Digit

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

[Sylvain Defresne]

It looks like fuchsia usage has now been reduced to calling set_sources_assignment_filter() with an empty list.

As a first step, I'm going to change set_sources_assignment_filter() to fail if called with non-empty list and warn that it is deprecated.

-- Sylvain