On second thought, this probably isn't a GN bug. //base adds these flags to all_dependent_configs for Linux and Windows so GN's behavior here is correct.

Okay, I'll close this then.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1316080967dfa9cd57b11269e25aedceb170514

commit d1316080967dfa9cd57b11269e25aedceb170514
Author: martijn <martijn@martijnc.be>
Date: Wed Jan 04 08:45:51 2017

Stop propagation of dependent configs between toolchains.

In the following scenario:

//foo:a (toolchain1) -> //foo:b (toolchain2) -> //foo:c (toolchain2)

Any dependent configs from //foo:b or //foo:c will not get added to //foo:a
because it is part of a different toolchain but any dependent config
from //foo:c will still get added to //foo:b because both targets are part of
the same toolchain.

BUG=675224
R=dpranke@chromium.org

Review-Url: https://codereview.chromium.org/2605963002
Cr-Commit-Position: refs/heads/master@{#441341}

[modify] https://crrev.com/d1316080967dfa9cd57b11269e25aedceb170514/tools/gn/target.cc
[modify] https://crrev.com/d1316080967dfa9cd57b11269e25aedceb170514/tools/gn/target_unittest.cc

My CL appears to have broken the iOS build (when GN is rolled [1]) because parts of it (egtests [2] & ochamcrest) rely on the old behaviour.

//ios/third_party/earl_grey:earl_grey has public_configs which now no longer propagate between toolchains (ios_clang_x64 & ios_clang_x86).

When you build iOS with additional_target_cpus = ["x86"] like the bots do, the build will fail because the Earl Grey include directory is missing from the targets in the ios_clang_x86 toolchain.

This cross-toolchain dependency seems deliberate [3], the dependency is explicitly set for the default toolchain (ios_clang_x64).

For example, in;
gn desc out/test "//ios/chrome/test/earl_grey:ios_chrome_web_egtests_arch_executable(//build/toolchain/mac:ios_clang_x86)" --tree

You'll find the following cross toolchain dependency where the config now gets dropped:
//ios/third_party/earl_grey:earl_grey+link
  //ios/third_party/earl_grey:earl_grey+link(//build/toolchain/mac:ios_clang_x64)

This dependency used to give the ios_clang_x86 targets the following ios_clang_x64 configs;
//ios/third_party/earl_grey:config(//build/toolchain/mac:ios_clang_x64)
//ios/third_party/earl_grey:earl_grey_public_config(//build/toolchain/mac:ios_clang_x64)
//ios/third_party/earl_grey:earl_grey_framework_headers_config(//build/toolchain/mac:ios_clang_x64)
//ios/third_party/ochamcrest:ochamcrest_public_config(//build/toolchain/mac:ios_clang_x64)
//ios/third_party/ochamcrest:ochamcrest_framework_headers_config(//build/toolchain/mac:ios_clang_x64)


Is the old behaviour expected in some cases and should I revert the commit in comment #5? Or should the iOS build config be changed to not rely on this behaviour? https://codereview.chromium.org/2610813008 reverts part of the previous CL and fixes the problem by reintroducing the old behaviour in part. But that seems weird and could cause the same problem I had originally.

I'm not really sure how to proceed here, or what the most appropriate solution is.

[1] https://codereview.chromium.org/2609413004
[2] https://cs.chromium.org/chromium/src/ios/third_party/earl_grey/ios_eg_test.gni?sq=package:chromium&l=1
[2] https://cs.chromium.org/chromium/src/build/config/ios/rules.gni?sq=package:chromium&dr=C&l=1084

I don't fully understand what's going on in the earl grey templates, but I'm pretty sure the iOS build should be changed to not rely on this behavior. 

+sdefresne in case I'm missing something.

Yes, the behaviour is expected on iOS.

When you build a fat binary, all applications and libraries are build with two different toolchains and then aggregated together using lipo. The fat binaries are generated in the default_toolchain. When building a framework (i.a. a dynamic library packaged as a bundle) with the goal to link against it, then you want all the targets to link with the fully build target that only exists for the default_toolchain.

So, what happens is that we have:

EarlGrey.frameworks depends on EarlGrey.x86.dylib and EarlGrey.x64.dylib (this is is not the real name), but then any target that wants to link against either need to link against EarlGrey.frameworks.

Then we have the tests themselves, they are both build for x86 and x64 in separate toolchains but need to depends on the fully build framework (for two reason, one is that the dylib in the framework has a different rpath, the other is that the framework also contains header file and to build properly the include paths are relative to the framework, i.e. #import <EarlGrey/EarlGrey.h> instead of #import "ios/third_party/earl_grey/src/EarlGrey/EarlGrey.h").

TL;DR: we need those cross-toolchain dependencies on iOS. But we know when they are required, so if this is causing issue, maybe we can have another "configs" variable to control them? public_cross_toolchain_configs?

I think there's probably some aspect of the way lipo works that I'm still not understanding. I guess I'm not sure why the tests aren't declaring the dependency on the framework directly (which is I think what you're suggesting ...).

I'm going to reassign this to you, sdefresne@. Let's figure out what the right thing to do is.

The issue is that we have the following (simplified) targets.

  EarlGrey.frameworks   (only exists for default_toolchain, define public_configs)
    \--> EarlGrey.lipo  (only exists for default_toolchain
          \--> EarlGrey.x86.dylib
          \--> EarlGrey.x64.dylib

  ios_chrome_integration_egtests            (only exists for default_toolchain)
   \--> ios_chrome_integration_egtests.lipo (only exists for default_toolchain)
         \--> ios_chrome_integration_egtests.x86
         |     \--> EarlGrey.framework($default_toolchain)
         \--> ios_chrome_integration_egtests.x64
               \--> EarlGrey.framework($default_toolchain)

i.e. when building ios_chrome_integration_egtests for the secondary toolchain, the target depends on the target defining the EarlGrey framework that only exists in $default_toolchain. But $current_toolchain != $default_toolchain at that point. With https://chromium.googlesource.com/chromium/src.git/+/d1316080967dfa9cd57b11269e25aedceb170514 the public_configs defined in EarlGrey.frameworks target is not propagated to ios_chrome_integration_egtests.x86 (since it has a different toolchain).

> I think there's probably some aspect of the way lipo works that I'm still not understanding. I guess I'm not sure why the tests aren't declaring the dependency on the framework directly (which is I think what you're suggesting ...).

On the contrary, I'm telling they do declare the dependency to get the public_configs, but the CL stop propagating that public_configs and thus break the build.

Since I understand the need to not propagate the public_configs between some targets/toolchain in some situation but iOS depends on this, we need to find a way. The simplest would probably be to add a boolean flag to config object saying that they can be propagated across toolchain (or a list of toolchain they can be propagated to).

WDYT?

Okay, if I'm understanding this correctly, can we just modify the ios_eg_test() template to explicitly include the dependency on "//ios/third_party/earl_grey:config" as well on https://cs.chromium.org/chromium/src/ios/third_party/earl_grey/ios_eg_test.gni?rcl=0&l=29 ?

Adding the config to ios_eg_test() won't be enough as other targets (source_set) do depends on //ios/third_party/earl_grey and need the config too to get the correct cflags. I can manually add the additional config on every target that depends on //ios/third_party/earl_grey, but then this is what public_configs is supposed to be used for.

Moreover, this mean that all ios_framework_bundle will require to use both the dependency on the framework and to manually list the config defined by the framework in the list of configs. I think this is broken.

This is why I think we should have a way to say that a config should be propagated through toolchain.

This is blocking the GN roll for fixing flakiness on buildbot runs which is very important. Are we going to be able to fix this today? If not, we should revert the change temporarily so I can roll.

I think we might be at the point where you (brettw@) should figure out what we want to do here. If we can do that today, we should, else we should revert temporarily until we do fix it.

Yuck, I just stared at this for a while and I don't know what to do since the toolchains weren't quite designed for this. Both behaviors seem clearly wrong in some cases.

Does it seem like maybe this should be a toolchain setting? For most toolchains we would never want this. For these mac ones, would we always want this?

It looks like for the ios_framework_bundle I can redefine the config for all toolchains and fix the dependencies in the template itself. This should solve the build on iOS and should allow to land the gn roll (I've got a CL that I'm locally testing, https://codereview.chromium.org/2627983002).

This makes the template even harder to understand and force me to duplicate listing the different dependencies in multiple locations but appears to work for the moment.

For the longer term, maybe we can add a "valid_for_toolchain" property on config. If unset, the config is only valid for the toolchain it is declared in, otherwise it is propagated for all toolchain listed in that property. WDYT?

Are you sure this is a property of configs and not toolchains? It looks like iOS library boundaries will have this problem, so it would apply to all public configs that cross such boundaries. I don't think a config has the knowledge about how it would be used.

I suspect we want all public configs to always cross boundaries between the default toolchain and fat binary variants.

Looking at the CL you sent it looks like this applies to specific configs. But say you have a "+link" dependency that crosses a toolchain, and there is a third_party library used in the library that is exposed in its headers. Wouldn't targets that depend on the "+link" cross-toolchain dependency need the third_party library's configs?

No, I'm not sure.

It currently only happens for config() defined by ios_framework_bundle() that define both the config and the target that depends on them, so in that case the config know where it is being used.

Making it a property of the toolchain make sense too.

Comment #19 was in replay to #17.

In my CL, the $target_name+link does forward the public_configs it inherits. Since they are defined when the template is called, they have the correct toolchain (they also get them from the default_toolchain via the public_deps of $target_name+link($current_toolchain) on $target_name+link$(default_toolchain)).

TL;DR: the only deps/configs that currently cross toolchains are the one in ios_framework_bundle. I agree it may be better to have a fix that works even for config() that don't know how they are used.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72c855683a26d8290f0366a06455d7ec38283aae

commit 72c855683a26d8290f0366a06455d7ec38283aae
Author: sdefresne <sdefresne@chromium.org>
Date: Wed Jan 11 18:50:49 2017

Avoid propagating configs across toolchain.

gn will be modified to no longer propagate public_configs across
toolchains (as this is a problem with some platforms). Refactor
ios_framework_bundle template to define the configs for all the
toolchains and to put the references directly (instead of having
secondary toolchain get them through the dependency).

BUG=675224

Review-Url: https://codereview.chromium.org/2627983002
Cr-Commit-Position: refs/heads/master@{#442967}

[modify] https://crrev.com/72c855683a26d8290f0366a06455d7ec38283aae/build/config/ios/rules.gni

Does this need to stay restricted?  I'd like to discuss its details on gn-dev.

Strange, I don't know why this is restricted, but I don't see any reason that it needs to be.

Setting defect without priority to Pri-2.