Starlarkify objc/CompilationAttributes.java

[keith]

No description provided.

[keith]

This and data weren't covered by tests for expansions, hoping internal google CI helps validate some of this

[pzembrod]

I'll take a close look what the internal Google tests can check here.

There's one more thing I noticed: in ObjcStarlarkInternal.createCompilationAttributes() the call to CompilationAttributes.Builder.addCompileOptionsFromRuleContext() (which sets linkopts, copts, additional_linker_inputs and defines) is only called if the rule has copts.
After digging a bit into the history of this, I currently think that this was not an intended coupling of conditions; the straightforward linear copying that you implemented makes more sense to me.

We can discuss tweaks if any problems surface from this.

[keith]

Minor error text change with fail vs Java. Same behavior and I tried to nearly match it

[pzembrod]

So what you are doing here is the corresponding operation to what's happening in CompilationAttributes.Builder.addHeadersFromRuleContext(), right? The picking of the key from the artifact/label pairs returned by CcCommon.getHeaders()?

Unless I'm mistaken, that effectively moves this further down in the chain, changing the content of compilation_attributes.hdrs from artifacts to artifact/label pairs. And since compilation_attributes in compilation_support.bzl is not just passed to objc_common.create_context_and_provider(), but also returned, both by build_common_variables() and register_compile_and_archive_actions_for_j2objc(), so the change doesn't look contained within compilation_support.bzl.

Wouldn't it be better to make the conversion from artifact/label pairs to artifact right when hdrs is initialized in _create_compilation_attributes()? Or am I missing something?

[keith]

hrm yea good point, pushed that. I assume long term once we can drop this objc specific stuff we'll want the pairs version, but I think you're right it's nicer to not mess with now

[pzembrod]

I'll handle the PR import myself.

[pzembrod]

I'll take a close look what the internal Google tests can check here.

There's one more thing I noticed: in ObjcStarlarkInternal.createCompilationAttributes() the call to CompilationAttributes.Builder.addCompileOptionsFromRuleContext() (which sets linkopts, copts, additional_linker_inputs and defines) is only called if the rule has copts.
After digging a bit into the history of this, I currently think that this was not an intended coupling of conditions; the straightforward linear copying that you implemented makes more sense to me.

We can discuss tweaks if any problems surface from this.

[pzembrod]

We had to roll this back as it did break one internal test after all. Will investigate.

[pzembrod]

The breaking test was test_linkopts_passed_to_binary() in https://github.com/bazelbuild/rules_apple/blob/master/test/ios_application_test.sh#L168, and the failure message was

in linkopts attribute of ios_application rule //app:app: label '//app:linker_input' in
$(location) expression is not a declared prerequisite of this rule

The immediate cause of the failure was the change in behaviour I mentioned in #24009 (comment), that in the native code linkopts, copts, additional_linker_inputs and defines are only set if the rule has copts whereas in the initial Starlark code they were all set independently of each other. I still believe that to be the right behaviour, but reproducing the native behaviour in Starlark does fix the test so that's what I'll do for now.

I'll investigate this further.

[pzembrod]

In the end, the fix to the broken test was not to reproduce the old native code behaviour where setting of linkopts, additional_linker_inputs and defines depended on the presence of copts, but to add additional_linker_inputs to the declared prerequisites in ObjcStarlarkInternal.expandAndTokenize().