Collect system includes from obcj dependencies

[AlessandroPatti]

System includes are not collected from Objc providers of the dependencies.

[allevato]

Do you have an example of a graph where this isn't working? I'm surprised we haven't hit any issues internally with it.

[AlessandroPatti]

Do you have an example of a graph where this isn't working? I'm surprised we haven't hit any issues internally with it.

I guess the reason why the issue went unnoticed is that system includes cannot be specified through any of the objc_library rule's attribute, so this happens only with a custom rule that exports an objc provider whose include_system field is not empty.

A graph like swift_library -> my_custom_rule should be enough to verify it.

[allevato]

Out of curiosity, have you checked to see if ObjcCompile actions will use the system includes from a CcInfo.compilation_context if you propagate that instead? If so, I'd recommend using that over fields in the Objc provider, which the Bazel team is working to get rid of.

If not, this patch looks fine (I'll need to test it internally), but I'd also like to decrease your dependence on something that we're trying to remove.

[AlessandroPatti]

have you checked to see if ObjcCompile actions will use the system includes from a CcInfo.compilation_context if you propagate that instead?

It uses it but it does not propagate it, so swift -> objc -> custom will not work because swift will never see the system includes coming from the CcInfo provider of custom.

[AlessandroPatti]

@allevato Is this ready to merge?

[allevato]

Sorry for the delay on this; in addition to being stretched a bit thin right now, I've been in the middle of refactoring these rules to support some future work that we need. I'll look at adding this change in as part of that so that you don't have to rebase it in on top of a completely changed implementation.

[AlessandroPatti]

Fine, I'll close this then. Thanks @allevato

[allevato]

So, unfortunately this isn't as straightforward as I'd hoped. Inside Google, we have some objc_library targets that have C++ dependencies that propagate include paths using system_include that cause problems when they're preserved; things have been working so far because we've dropped those include paths through the Objective-C graph edge.

I'll need to dig a bit more to figure out how to gracefully handle this case before we can roll it out, but I want to re-open this so I don't lose track of it.

[keith]

This objc provider field is gone with this incompatible flag bazelbuild/bazel#11359

If there's another field we should use for this now feel free to submit a new PR!