Add support for params files for darwin

[keith]

Clang has supported params files for a while now. This updates the cc
toolchain for darwin to use them.

The logic for processing response files is mostly copied from
rules_swift where similar processing is done.

[keith]

@oquenchil I would love this to make the 3.8 cut if possible

[keith]

rebased and added a test for the new behavior

[keith]

Looks like I actually have to make some more fixes here and add the objc link actions to here:

bazel/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java

Lines 515 to 540 in a3a00f2

	@VisibleForTesting
	boolean canSplitCommandLine() {
	if (toolchain == null || !toolchain.supportsParamFiles()) {
	return false;
	}
	switch (linkType) {
	// On Unix, we currently can't split dynamic library links if they have interface outputs.
	// That was probably an unintended side effect of the change that introduced interface
	// outputs.
	// On Windows, We can always split the command line when building DLL.
	case NODEPS_DYNAMIC_LIBRARY:
	case DYNAMIC_LIBRARY:
	return (interfaceOutput == null
	|| featureConfiguration.isEnabled(CppRuleClasses.TARGETS_WINDOWS));
	case EXECUTABLE:
	case STATIC_LIBRARY:
	case PIC_STATIC_LIBRARY:
	case ALWAYS_LINK_STATIC_LIBRARY:
	case ALWAYS_LINK_PIC_STATIC_LIBRARY:
	return true;
	default:
	return false;
	}
	}

Right off the bat it looks like that breaks when passing -rpath @loader_path because something in bazel assumes @loader_path is a params file. I'll have to debug this more next week.

[keith]

There's an issue where currently the params files conflict if you run a build for multiple architectures at once. I would have expected this to have been solved for C++ even. Looking into it

[trybka]

That's surprising--I would have thought they would have been differentiated by the output configuration dir?

[keith]

Same, it's very easy to repro tho:

% cat BUILD
apple_binary(
    name = "bin",
    minimum_os_version = "10.0",
    platform_type = "ios",
    deps = [":lib"],
)

objc_library(
    name = "lib",
    srcs = ["main.m"],
)

$ bazel build :bin --ios_multi_cpus=i386,x86_64
ERROR: file 'bin_bin-2.params' is generated by these conflicting actions:
Label: //:bin
RuleClass: apple_binary rule
Configuration: ed93f65d05ded6845d66048ac3ade22d7d88b5de42c13efee80d216689eeaa76
Mnemonic: FileWrite
Action key: 447c15515b5910cd244d50bb7f2bd0e703d82814e00b3099e9e802e6e0a36f68, c7d79bd92b4d69d8111e4e57bc225fa82e4fd84fd65dfeeba124fa50f90107ab
Progress message: Writing file bin_bin-2.params
PrimaryInput: (null)
PrimaryOutput: File:[[<execution_root>]bazel-out/apl-darwin_x86_64-fastbuild/bin]bin_bin-2.params
Owner information: ConfiguredTargetKey{label=//:bin, config=BuildConfigurationValue.Key[ed93f65d05ded6845d66048ac3ade22d7d88b5de42c13efee80d216689eeaa76]}
MandatoryInputs: are equal
Outputs: are equal
ERROR: Analysis of target '//:bin' failed; build aborted: for bin_bin-2.params, previous action: action 'Writing file bin_bin-2.params', attempted action: action 'Writing file bin_bin-2.params'

[keith]

I think it's because the configuration of the params files is the same, since it's the exec configuration not the target configuration?

[keith]

So I discovered that in some case a different configuration was being used for the link actions, which fixes this params file issue. I had to make the change for fully linked static libraries and for multi arch executables. The big question now is what other things might this break?

[keith]

I'll have to check the windows tests, but everything for darwin passes now. But I would love some assurance from someone who knows this logic better whether or not the configuration changes make any sense.

[keith]

The windows test failures appear to have been a fluke. Fixed after a rebase. So this might be good to go

[oquenchil]

LGTM

[keith]

Thanks for the review! Btw, is this being imported? (I never know if that's implicit with the +1)

[lberki]

It should be. Generally, it's the "sponsor" of the pull request who does it, who is @oquenchil in this case.

[brentleyjones]

How do we know who the sponsor is? I will admit that the current process confuses me as well, it's not clear to me when it's being imported. Can we get a label or something?

[keith]

Thanks everyone!

[oquenchil]

Unfortunately, this had to be rolled back because it broke tests internally.

[keith]

Here's the update for rules_go bazel-contrib/rules_go#2699

[keith]

I think once those 2 issues are resolved we should be good to give it another go

[oquenchil]

I merged the first one, please ping this thread when the rules_go PR is merged, then I will submit this again.

Regarding testing with downstream, the information is here:
https://github.com/bazelbuild/continuous-integration/blob/master/docs/downstream-testing.md

You have to request access but they will definitely grant it to you.

[Yannic]

Downstream: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1747#ad818f41-8ad0-4dad-8ced-485006558e9d

[keith]

Ok rules_go is merged, the fix for rules_cc is here bazelbuild/rules_cc#89 because that repo duplicates the wrappers I changed here

[keith]

I re-ran a downstream build after the rules_go fix merged https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1755#_ going to look at tensorflow once more

[keith]

Ok I figured it out, tensorflow is building with Xcode 10.3, which doesn't support params files to libtool. AFAIK 10.3 isn't really considered supported at this point (since Apple doesn't allow uploads to the store with it) so I've submitted this change attempting to bump the Xcode version it uses: bazelbuild/continuous-integration#1057

[keith]

@oquenchil this should be ready for you again! Tensorflow passed now that the Xcode version has been bumped https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1757

So the last thing is this rules_cc change! bazelbuild/rules_cc#89

[oquenchil]

LGTM

[keith]

Rebased to resolve the conflict!

[keith]

Rebased to resolve my other conflict 😛

[keith]

I started another downstream build because I assume we have some time here https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1760#7612b6a6-0422-4580-b2a3-e78791c31297

[keith]

Seems like incompatible_restrict_string_escapes and some other changes broke a lot of downstream projects. Nothing stands out as related to this change anymore.

[keith]

Thanks for all the help!