How do I make every executable target depend on some target?

[Nico Weber]

Hi,

https://codereview.chromium.org/682843002/ added the ASan build to gn. It changed BUILDCONFIG to automatically add a dep to //build/config/sanitizers:deps for every component(). It did not do this for any executables, and many executables fail to link since they're missing this dependency.

Is there a way in gn to say "Make everything depend on this target"? Or is every executable() supposed to link this dependency manually? (If so, why is it auto-added to components?)

Thanks,

Nico

[Brett Wilson]

The way to do this is to add a default deps entry in the set_defaults call for executables in BUILDCONFIG. But this would force all executables to define their deps like "deps +=" instead of "deps =" which we have wanted to avoid. test() and component() are both templates so we can do whatever we want. There actually aren't a huge number of executables floating around that aren't tests, so this hasn't warranted more invasive changes.

So yes, every executable that needs to be run on the sanitizer bots should have this. Bruce was recently looking at something similar.

Brett

[Nico Weber]

Not just executable running on the bots need this: executables without this fail to link, breaking bots that build 'all'.

Maybe gn could ignore default deps for = vs +=?

[Brett Wilson]

On Mon, May 2, 2016 at 12:29 PM, Nico Weber <tha...@chromium.org> wrote:

Not just executable running on the bots need this: executables without this fail to link, breaking bots that build 'all'.

Maybe gn could ignore default deps for = vs +=?

That seems like a really weird and confusing attribute for a language to have.

Brett

[Nico Weber]

I'm always confused about when I'm currently supposed to use = or += actually. Is the rule roughly "use = if nothing else sets (say) cflags in the current scope (config, executable, ...), else use +="? (If so, then that's be somewhat consistent with that rule: If a config sets cflags and a target locally wants to set it too, I think the target currently needs to use =. So if set_defaults() is treated as a config, then this would be fairly natural. (But as I said, I don't really understand the current rule, so maybe that's silly.)

[Brett Wilson]

+= appends to an existing value. When you do it, that value must exist . This is a pretty normal programming language.

You mention configs. GN internally concatenates lists from various places to come up with the flags for the final target. This is separate from the programming language aspect of the program.

There are two places where the GN language differs from a normal language in terms of variable assignments. The first is in the "can't overwrite a nonempty list with a nonempty list" error. This is easy to understand (it's a hard error if you run into it, the error message tells you how to work around it), and it catches a large class of mistakes.

The second is the sources assignment filter where some things are magically filtered out for sources for platform file names. This has been horribly confusing and I regret adding it. Having a rule "operator = does an append instead of a concatenation for a variable with a specific name" is even beyond that.

GN has a way to do what you want with set_defaults. The set_defaults, which is used for configs. People are confused about why you can't do "=" for configs, but in some cases you really do need to deal with the fact that there are things there (like when you need to remove something, or add something to the beginning instead of the end.

The philosophy fo the language is that there is as little as possible happening behind your back, even if it forces you to do more work. A similar example is that people sometimes ask for -= to not error if the item isn't in the list. They want to be able to remove something if it's there. But my answer has always been if this is causing a big problem for you, the code is designed incorrectly. You should not be in the position of removing something you don't know is there. This has forced a somewhat different style in the writing of the build files that I think is positive.

A different solution would be to allow one to override built-in target types with templates. So BUILDCONFIG would define a template that wraps the built-in executable and modifies the inputs in various ways (much like the component template does). I have resisted adding this because I feel like in the long run it will be abused to do too many things. We would end up with common.gypi-like behavior where lots of magic things get set when you can't see them, and it's a tempting hammer to use for a large class of features that would probably be done other ways. I think it's better to not open this box.

Just adding the dependencies is really the simplest solution here. We very rarely add new executables so I don't think the burden will be high once it is configured.

Brett

[Dirk Pranke]

Isn't specifying a default deps in BUILDCONFIG and requiring every executable to use "+=" the right thing to do here, for exactly the reasons you gave?

I.e., we really want failing to include the default deps to be a hard error., and so using `deps =` rather than `deps +=` is wrong, even if `+=` is confusing.

IMO, having inconsistencies between `deps =` and `configs +=` is also confusing; specifying defaults for both for all targets across the board would at least be consistent, right (though, at this point, a hassle to fix).

-- Dirk

[Brett Wilson]

On Mon, May 2, 2016 at 2:46 PM, Dirk Pranke <dpr...@chromium.org> wrote:

Isn't specifying a default deps in BUILDCONFIG and requiring every executable to use "+=" the right thing to do here, for exactly the reasons you gave?

I'm not against that like I am against making the operators behave differently for different variables. But the configs thing already confuses people so I'm not enthusiastic about it either. It probably comes down to how the trybots are configured and how obvious the error messages are.

Brett

[Brett Wilson]

Also, in practice, this will be a super difficult change to land.

Brett

[Andrew Grieve]

I put a bit of thought into this while working on Android asan: https://bugs.chromium.org/p/chromium/issues/detail?id=578198

I think probably the most workable way to accomplish this is to allow us to define a template("executable") {} in BUILDCONFIG.gn that overrides but forwards to the native executable() target.

[Dirk Pranke]

While I agree that creating a template() that was used instead of using executable() directly would work around this problem, I think that's all it is, a workaround.

A workaround that is admittedly useful, for this and other things, of course. I've thought for a long time that we shouldn't let people use any of the built-in

target types because you can't impose global conventions on them very easily. But, that also seems like a failing of GN itself, perhaps.

Brett, I'm guessing you're saying that this would be a super difficult change to land because there would be executables() declared in repos that would have

to be DEPS'ed in, and you'd have to do that at the same time you changed //build, so it would be one big massive roll (and the upstream repos would be broken

in the meantime)?

-- Dirk

[Brett Wilson]

On Mon, May 2, 2016 at 7:23 PM, Dirk Pranke <dpr...@chromium.org> wrote:

While I agree that creating a template() that was used instead of using executable() directly would work around this problem, I think that's all it is, a workaround.

A workaround that is admittedly useful, for this and other things, of course. I've thought for a long time that we shouldn't let people use any of the built-in

target types because you can't impose global conventions on them very easily. But, that also seems like a failing of GN itself, perhaps.

Brett, I'm guessing you're saying that this would be a super difficult change to land because there would be executables() declared in repos that would have

to be DEPS'ed in, and you'd have to do that at the same time you changed //build, so it would be one big massive roll (and the upstream repos would be broken

in the meantime)?

Yes, exactly.

Brett

[bruce...@google.com]

I just finished adding manifests to all of the executables. It wasn't totally trivial, but it wasn't a crazy amount of work either. The third_party gn files are the most annoying since they each require their own CL, sometimes just for one or two binaries. I didn't bother rolling any of them because I figured that most of them would be rolled for other purposes soon enough - I'm running a build now to see if any need attention.

The main crucial thing for making this manageable was having an easy way to find executables that were missing manifests, which I did by doing a full build and then running a batch file that iterated over *.exe. Alternately you could scan the BUILD.gn files for default_exe_manifest and insert your new dependency in the appropriate location relative to it.

If we end up having to add a deps entry to all executables frequently then it's onerous, but if it is sufficiently rare then it's okay.

[Nico Weber]

For future reference: thomasanderson did something like this in https://bugs.chromium.org/p/chromium/issues/detail?id=845700 (using the template workaround discussed on this thread).