Pruning non-reachable targets when running "gn gen"

[Sylvain Defresne]

It appears that when running "gn gen" for each targets found in the BUILD.gn files loaded, a corresponding ninja rule is generated, even if there is no path between the target and the root target defined in //.gn.

In particular, this means that when building with target_os="ios", the build ends up defining targets that are not used (e.g. libjpeg) or worse that do not even compile (e.g. webrtc). Those targets are then build if we run ninja without a target, i.e. "ninja -C out/Default". The build only succeed if we explicitly pass the root target, i.e. "ninja -C out/Default gn_all".

So I'd like to propose a change to "gn gen" to prune all targets that are unreachable before generating the ninja files. This would mean that both ninja commands (i.e. using the explicit root target or no target) should result in the same build.

WDYT?

-- Sylvain

[Sylvain Defresne]

BTW, see https://codereview.chromium.org/1886443002#msg2 for an example of such target that is unreachable but still built if ninja is invoked without any explicit target.

-- Sylvain

[Brett Wilson]

We've talked about changing this several times.

I did it this way originally because that's what I thought GYP did. It turns out GYP seems to do this because many of the root targets list wildcards deps (GN doesn't support these) like 'content/content.gyp:*' that effectively brings in all targets into the build whether you reference them in the dependency tree or not. The result is that I think people expect to be able to write random tests or helper targets and have them appear as possible things to run Ninja with.

GN now has several ways to control this. GN generates a target called "all" where if you want to build everything GN saw in the build (in the default toolchain) Ninja will build that. It has a separate concept of the default target where if you make a target called "//:default" it will mark that as the default target in Ninja so when you build with no explicit target that set of stuff will get built. The Chrome build does not set a default target, so the automatically-produced "all" target becomes the default.

We could make a default target and get the behavior you want, but "all" would still exist and refer to everything, and it would be broken. I think the "all" target actually matches the GYP behavior. If you do "ninja all" in a GYP target, I suspect you'll get similar errors (Dirk can probably confirm, I think he implemented GN's current "all" behavior). I think the ability to write a target and being able to build it (as long as GN knows about that file) is generally what people on our team expect so I would personally prefer not to change it.

There's a separate question about whether we should add a default target that's more like what you want. I'm not convinced. It's confusing to have a target be broken in the "all" build. In GYP I believe there are a bunch of bitrotted targets that are never compiled because we never build it's version of "all" on any bot (I think, Dirk may have to correct me). 

This also masks some other problems. In the "jpeg" problematic patch you linked, there is only one target in that BUILD file (there are two configs there too, but they seem to be only referenced by that target). Theoretically this file should never get loaded in an iOS build. So somebody is referring to jpeg, probably a target somewhere else that shouldn't be compiled on iOS, etc. This suggests there are large portions of the build being brought in inappropriately on iOS.

I think this is actually the important bit here. When we're loading build files inappropriately on some platforms, we should just stop doing that rather than trying to define default targets such that those targets are no-ops.

When a file is inappropriate on iOS (or any other platform) we should add

  assert(!is_ios)

at the top. Now it's clear to people editing the file that this isn't an iOS thing, and also tells people immediately when they screw up and bring a file in that shouldn't. When there is a file with some targets that are appropriate for iOS, and some targets that aren't, we should wrap those in conditionals that clearly document the platforms they're appropriate for. Now it's also clear which parts of the file refer to a given platform. 

The property that the current setup forces us to do this seems to be a benefit rather than a bug, and I don't think we should be wallpapering over targets that don't compile on some platforms: If it's not obvious to GN, it's not obvious to people editing the file either).

Brett

[Nico Weber]

On Sun, Apr 17, 2016 at 1:19 PM, Brett Wilson <bre...@chromium.org> wrote:

We've talked about changing this several times.

I did it this way originally because that's what I thought GYP did. It turns out GYP seems to do this because many of the root targets list wildcards deps (GN doesn't support these) like 'content/content.gyp:*' that effectively brings in all targets into the build whether you reference them in the dependency tree or not. The result is that I think people expect to be able to write random tests or helper targets and have them appear as possible things to run Ninja with.

GN now has several ways to control this. GN generates a target called "all" where if you want to build everything GN saw in the build (in the default toolchain) Ninja will build that. It has a separate concept of the default target where if you make a target called "//:default" it will mark that as the default target in Ninja so when you build with no explicit target that set of stuff will get built. The Chrome build does not set a default target, so the automatically-produced "all" target becomes the default.

We could make a default target and get the behavior you want, but "all" would still exist and refer to everything, and it would be broken. I think the "all" target actually matches the GYP behavior. If you do "ninja all" in a GYP target, I suspect you'll get similar errors (Dirk can probably confirm, I think he implemented GN's current "all" behavior). I think the ability to write a target and being able to build it (as long as GN knows about that file) is generally what people on our team expect so I would personally prefer not to change it.

There's a separate question about whether we should add a default target that's more like what you want. I'm not convinced. It's confusing to have a target be broken in the "all" build. In GYP I believe there are a bunch of bitrotted targets that are never compiled because we never build it's version of "all" on any bot (I think, Dirk may have to correct me). 

(The clobber bots all build all. Some trybots do build all. The clang tot builds all build all. This works fine. However, 'all' in gyp isn't all targets. That's for historical reasons and I consider it a bug in gyp (https://bugs.chromium.org/p/chromium/issues/detail?id=503241). Not that this is terribly important for the discussion at hand :-) )

The android build uses to have a default target that wasn't 'all' long ago. We changed that because it was confusing, for the reasons you mention.

 

This also masks some other problems. In the "jpeg" problematic patch you linked, there is only one target in that BUILD file (there are two configs there too, but they seem to be only referenced by that target). Theoretically this file should never get loaded in an iOS build. So somebody is referring to jpeg, probably a target somewhere else that shouldn't be compiled on iOS, etc. This suggests there are large portions of the build being brought in inappropriately on iOS.

I think this is actually the important bit here. When we're loading build files inappropriately on some platforms, we should just stop doing that rather than trying to define default targets such that those targets are no-ops.

When a file is inappropriate on iOS (or any other platform) we should add

  assert(!is_ios)

at the top. Now it's clear to people editing the file that this isn't an iOS thing, and also tells people immediately when they screw up and bring a file in that shouldn't. When there is a file with some targets that are appropriate for iOS, and some targets that aren't, we should wrap those in conditionals that clearly document the platforms they're appropriate for. Now it's also clear which parts of the file refer to a given platform. 

The property that the current setup forces us to do this seems to be a benefit rather than a bug, and I don't think we should be wallpapering over targets that don't compile on some platforms: If it's not obvious to GN, it's not obvious to people editing the file either).

I agree with all of this.

[Dirk Pranke]

On Sun, Apr 17, 2016 at 10:32 AM, Nico Weber <tha...@chromium.org> wrote:

On Sun, Apr 17, 2016 at 1:19 PM, Brett Wilson <bre...@chromium.org> wrote:

We've talked about changing this several times.

I did it this way originally because that's what I thought GYP did. It turns out GYP seems to do this because many of the root targets list wildcards deps (GN doesn't support these) like 'content/content.gyp:*' that effectively brings in all targets into the build whether you reference them in the dependency tree or not. The result is that I think people expect to be able to write random tests or helper targets and have them appear as possible things to run Ninja with.

GN now has several ways to control this. GN generates a target called "all" where if you want to build everything GN saw in the build (in the default toolchain) Ninja will build that. It has a separate concept of the default target where if you make a target called "//:default" it will mark that as the default target in Ninja so when you build with no explicit target that set of stuff will get built. The Chrome build does not set a default target, so the automatically-produced "all" target becomes the default.

We could make a default target and get the behavior you want, but "all" would still exist and refer to everything, and it would be broken. I think the "all" target actually matches the GYP behavior. If you do "ninja all" in a GYP target, I suspect you'll get similar errors (Dirk can probably confirm, I think he implemented GN's current "all" behavior). I think the ability to write a target and being able to build it (as long as GN knows about that file) is generally what people on our team expect so I would personally prefer not to change it.

There's a separate question about whether we should add a default target that's more like what you want. I'm not convinced. It's confusing to have a target be broken in the "all" build. In GYP I believe there are a bunch of bitrotted targets that are never compiled because we never build it's version of "all" on any bot (I think, Dirk may have to correct me). 

(The clobber bots all build all. Some trybots do build all. The clang tot builds all build all. This works fine. However, 'all' in gyp isn't all targets. That's for historical reasons and I consider it a bug in gyp (https://bugs.chromium.org/p/chromium/issues/detail?id=503241). Not that this is terribly important for the discussion at hand :-) )

The android build uses to have a default target that wasn't 'all' long ago. We changed that because it was confusing, for the reasons you mention.

 

This also masks some other problems. In the "jpeg" problematic patch you linked, there is only one target in that BUILD file (there are two configs there too, but they seem to be only referenced by that target). Theoretically this file should never get loaded in an iOS build. So somebody is referring to jpeg, probably a target somewhere else that shouldn't be compiled on iOS, etc. This suggests there are large portions of the build being brought in inappropriately on iOS.

I think this is actually the important bit here. When we're loading build files inappropriately on some platforms, we should just stop doing that rather than trying to define default targets such that those targets are no-ops.

When a file is inappropriate on iOS (or any other platform) we should add

  assert(!is_ios)

at the top. Now it's clear to people editing the file that this isn't an iOS thing, and also tells people immediately when they screw up and bring a file in that shouldn't. When there is a file with some targets that are appropriate for iOS, and some targets that aren't, we should wrap those in conditionals that clearly document the platforms they're appropriate for. Now it's also clear which parts of the file refer to a given platform. 

The property that the current setup forces us to do this seems to be a benefit rather than a bug, and I don't think we should be wallpapering over targets that don't compile on some platforms: If it's not obvious to GN, it's not obvious to people editing the file either).

I agree with all of this.

Brett and I talked about this a bit more in person as well this morning.

Generally, I agree with Brett and Nico here.

As long as we're talking about build files within a single project, I think it makes sense that "all" should really mean "everything in every file that I read (and saw)"; anything else seems potentially fragile and just kinda weird.

I think things get fuzzier when you start talking about build files from separate projects. I would be concerned if, for example, we started making webrtc or v8 builds have to be very sensitive to how they are included and used in chromium just so targets that chromium will never care about won't be built with flags that will never work for those targets. And, there's definitely a risk of this (and possibly we see this even today).

However, as Brett said, I think this often indicates that there's something wrong in the Chromium build files that is pulling in these things, and thus these build issues are actually pointing at underlying problems.

I think we should leave the behavior the way it is now, but keep this in the back of our mind and be on the lookout for examples where it really seems like we're having to do pointless things to make 'all' work.

-- Dirk

[Sylvain Defresne]

I think I understand why we want to keep the current behaviour.

Currently the case with webrtc is complex. It is not used by Chrome on iOS, but they do have target building for iOS. So I cannot add "assert(!is_ios)" in webrtc BUILD.gn files, nor even put the problematic targets behind "if (!is_ios)" as this will cause failures for the webrtc team.

Chrome on iOS uses the following targets:

third_party/webrtc/base:rtc_base

  -> third_party/webrtc:webrtc_common

  -> third_party/webrtc:common_config

  -> third_party/webrtc:common_inherited_config

This also causes all targets defined in third_party/webrtc and their dependency to be part of the build and cause build failure. If I understand correctly, what I should do is move those targets and config that are used to another BUILD.gn. I'll check with the webrtc team if they would accept such a change.

-- Sylvain

[Dirk Pranke]

If webrtc is not used by Chrome on iOS, why is anything in Chrome depending on anything in webrtc in iOS?

The theory I gave earlier says that that should be a mistake; if that's not a mistake, then we might need to

re-evaluate things.

-- Dirk

[Sylvain Defresne]

We use //third_party/webrtc/base:rtc_base via //jingle.

This depends on //third_party/webrtc:{common_config,common_inherited_config,webrtc_common}. But other targets in that file depends on many things from third_party/webrtc that are not used on iOS. They all build correctly on iOS (though we do not use them) except for //third_party/webrtc/base:rtc_base_objc.

It looks like webrtc has a variable to check whether it is built as part of chromium or not, so I'll send https://codereview.webrtc.org/1898283002 to not define that target when building Chromium for iOS.

Thank you for the help and for probing me to dig deeper,

-- Sylvain

[Dirk Pranke]

I thought iOS wasn't supposed to depend on jingle, either?

[Sylvain Defresne]

It does through //components/invalidation to get at //jingle:notifier. This dependency also exists with gyp.

-- Sylvain

[Brett Wilson]

I haven't looked at the broader picture, but it looks like //jingle/BUILD.gn has no dependencies outside of that directory on iOS. Is there another path?

Brett

[Sylvain Defresne]

Bumping this again as this is confusing developers of Chrome on iOS.

It looks like all the Chromium gn bots currently only build gn_all target (at least on iOS), thus the "all" target bit rots and ends up not working frequently on iOS (as most executable target simply do not work on iOS as the necessary frameworks are not linked). This did happen again today with some other target.

If the consensus is that it is acceptable that gn_all != "all" (which I understand because we do not really tell "gn gen" which target should be considered, just the root BUILD.gn file to load first), then can we at least fix the bots to build "all" since it is a superset of gn_all. That way, both "all" and gn_all will be known to build at all time, and user confusion will be avoided.

I can probably contribute time to get the "all" target working when building on iOS, but I cannot do it for the other targets, and I do not know how to fix the bots to build "all" instead of "gn_all" (IIUC, the analyse step requires a target name and does not accept the "all" target).

WDYT?

-- Sylvain

[Sylvain Defresne]

+rohitrao, +lpromero that have recently been hit by this.

-- Sylvain

[Dirk Pranke]

In theory, "gn_all" is supposed to build everything that "all" does, and it's a bug that it doesn't (crbug.com/560293).

Analyze also doesn't currently work correctly with the "all" target (crbug.com/555273).

Since "all" doesn't work with analyze, we have to use "gn_all" in the CQ. Thus, if you're building "all" downstream

you are indeed likely to hit issues. 

The correct thing to do is to fix both bugs, but 555273 is much more important. In the meantime, either build

just "gn_all" or accept that you might get broken builds ...

Apologies for the inconvenience; I haven't had the time to fix this issue yet because of other ship-GN-related things.

-- Dirk

[Sylvain Defresne]

Thank you for the pointers to the bugs. I'm okay if we have bug tracking this and plan to eventually address them.

Until then I'll point this thread to my teammates when their build fails :-)

-- Sylvain