[WIP] Objc modular imports #6243
Conversation
ob
requested review from
allevato,
DavidGoldman,
dmaclach,
lberki and
sergiocampama
as code owners
iirina
added
the
z-team-Apple
| @@ -380,7 +380,7 @@ public void testProvidesObjcHeadersWithDotMFiles() throws Exception { | |||
| assertThat(getArifactPaths(target, HEADER)) | |||
| .containsExactly("objc/a.h", "objc/b.h", "objc/f.m"); | |||
| assertThat(getArifactPaths(depender, HEADER)) | |||
| .containsExactly("objc/a.h", "objc/b.h", "objc/f.m", "objc2/d.h", "objc2/e.m"); | |||
| .containsExactly("objc/a.h", "objc/b.h", "objc/f.m", "objc/private.h", "objc2/d.h", "objc2/e.m"); | |||
There was a problem hiding this comment.
This is checking the header field of the ObjcProvider, right? AFAIK, it definitely should not be propagating private headers that are only listed in srcs.
There was a problem hiding this comment.
I was going by this comment from #5954.
There was a problem hiding this comment.
Hmm, that's a fair point (a public header including a private header). I'd have to dig deeper to see why that hasn't been an issue today, unless people just so habitually glob .h files into hdrs and .m files into srcs that it doesn't ever come up. 🤷♂️
As long as the HEADER field is only used to determine which header files end up in downstream actions' sandboxes, that should be fine. But if we ever query that field anywhere else with the assumption that that's the "public interface" of the library, we'd need to figure out what to do there. I don't know if that's an issue anywhere currently, though.
Module map creation uses the hdrs attribute, not the provider, I believe, so that should be unaffected.
|
One thing to consider is that with too many dependencies this has the potential to exceed the maximum command line argument. I should probably pass all the flags as a response file. |
The CreateSourceAction function is called _per file_, so this was adding the flags multiple times. Call it before the loop so that the flags are only added once.
|
I am a bit confused why here |
|
This is no longer being pursued in this form, I'll take another crack once the C++ rules are in StarLark. |
This adds support for using modular imports (
@import module) fromobjc_libraryrules.Implementation note:
I again had to remove the extraneous
CcCompilationContextmentioned in #5954, and still have the same question from that PR.I also cleaned up what looked like unused code (that's why there are two commits).