Include CPU in darwin platform string

[keith]

Bazel uses the CPU to determine the --apple_platform_type to build for
in some cases. Previously on a macOS machine the CPU would be returned
as just darwin, which isn't considered a valid macOS CPU, instead we
should return what we support and include the architecture.

[keith]

cc @philwo since you were mentioned on the TODO

[keith]

An alternative (or addition to this) could be to add darwin the the list of macOS CPUs here

bazel/src/main/java/com/google/devtools/build/lib/rules/apple/ApplePlatform.java

Lines 55 to 56 in 0ccee07

	private static final ImmutableSet<String> MACOS_TARGET_CPUS =
	ImmutableSet.of("darwin_x86_64");

let me know if that would make sense to do too.

I could also do what is done for Windows and return unknown in this case, I don't know if bazel supports 32 bit macOS systems but I assume it does not.

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/AutoCpuConverter.java

Lines 44 to 46 in 7c70d82

	default:
	// We only support x64 Windows for now.
	return "unknown";

[keith]

It looks like some other stuff also assumes it's just darwin

bazel/tools/cpp/lib_cc_configure.bzl

Lines 180 to 181 in edf7375

	if os_name.startswith("mac os"):
	return "darwin"

which also appears to be causing the test failures. I'm leaning more towards adding darwin to MACOS_TARGET_CPUS but I'm still debugging the tests

[keith]

I pushed a possible fix here, I'm not entirely sure this is the right option. Running this locally doesn't actually work but I believe it's because the tests invoke my globally installed bazel somehow, instead of the one that was just build, and therefore get the old local_config_cc

[aragos]

Some considerations: As you mention above I don't think there is more than a single macOS identifier, all the supported machines run on an x86 64bit darwin. The fact that two values exist for the same purpose has historical reasons (people didn't agree on what was the "correct" one). I don't think supporting both is the way to go though, updating everything to just use one version seems saner.

In any case, some tests appear to be failing on buildkite with your changes, please make sure that's green (go to "details" above, click on the failing pipeline (macOS JDK8), you can see the detailed test logs by going to that pipeline's "Artifacts" tab).

[keith]

Which one should I unify on though? Likely unifying on the current one would be easier. But either way removing one will be a breaking change I guess? Specifically this might break people who have config that selects on Darwin as the cpu? I was still debugging why the tests are failing, but now I'll wait for a decision here before fixing them.

…

-- Keith Smiley

On Jan 14, 2019, at 06:58, aragos ***@***.***> wrote: Some considerations: As you mention above I don't think there is more than a single macOS identifier, all the supported machines run on an x86 64bit darwin. The fact that two values exist for the same purpose has historical reasons (people didn't agree on what was the "correct" one). I don't think supporting both is the way to go though, updating everything to just use one version seems saner. In any case, some tests appear to be failing on buildkite with your changes, please make sure that's green (go to "details" above, click on the failing pipeline (macOS JDK8), you can see the detailed test logs by going to that pipeline's "Artifacts" tab). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[keith]

@trybka while working on this fix I'm hitting a new issue that @sergiocampama thinks you might be able to help with.

This test:

bazel/src/test/java/com/google/devtools/build/lib/rules/cpp/CcCommonTest.java

Lines 690 to 702 in 682a0d1

	@Test
	public void testCcLibraryWithDashStatic() throws Exception {
	checkWarning(
	"badlib",
	"lib_with_dash_static",
	// message:
	"in linkopts attribute of cc_library rule //badlib:lib_with_dash_static: "
	+ "Using '-static' here won't work. Did you mean to use 'linkstatic=1' instead?",
	// build file:
	"cc_library(name = 'lib_with_dash_static',",
	" srcs = [ 'ok.cc' ],",
	" linkopts = [ '-static' ])");
	}

Fails on macOS with this change because of this conditional:

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

Lines 249 to 252 in 00d2050

	if (ApplePlatform.isApplePlatform(ccToolchain.getTargetCpu()) && result.contains("-static")) {
	ruleContext.attributeError(
	"linkopts", "Apple builds do not support statically linked binaries");
	}

Before my change ApplePlatform.isApplePlatform("darwin") would return false, which seemed to have incorrectly made this test pass. Now that I'm fixing that by changing the CPU in this case to one that isApplePlatform returns true for, this test fails.

So I have 2 questions:

1. Why can't apple platforms support static cc_library targets?
2. If there's a un-fixable reason for 1, how should I fix this test?

[keith]

@aragos I've updated this PR to only support darwin_x86_64 in case we have to support other architectures in the future. We'll see if there are any new test failures besides the ones mentioned above

[keith]

I might be able to change this to just darwin_x86_64, I'm not entirely sure, but also if we eventually have to support a new darwin, I assume this is what we'll want

[keith]

It looks like the only failing test is the one I commented about above, so once we have a solution to that this should be green

[keith]

I've pushed one possible solution for the test failure I asked about above 7c16844

This commit separates the failing test for darwin and validates that it errors with that message in this case, instead of having just the warning

[hlopko]

I don't think we can accept this without going through incompatible flag. This can (and will) break existing code. Since we are planning to migrate C++ (and objc transitively) rules to platforms (https://www.bazel.build/roadmaps/cpp.html#c-rules-use-platforms-2019q1) I think the better course of action is to make https://source.bazel.build/bazel/+/5609d14ece35fc57084bb66161c7b579cd836c5e:tools/platforms/BUILD more robust, and force users to migrate only once. Wdyt?

[keith]

What do you mean by making that platform configuration more robust?

[keith]

Also I guess a non-breaking change we could make here instead would be to add darwin to the acceptable list of macOS apple platforms. This would fix the underlying issue and just not set us up for the future as much

[keith]

I've submitted the darwin addition option here: #7151

[hlopko]

By more robust I mean having enough constraint_values to express all reasonable cases. But I should first sync with @aragos because he has plans that involve --cpu and platforms and related universes...

[keith]

Sounds good, I'm hoping we can merge the alternative fix here #7151 and then punt on this portion of it until it's a better time

[keith]

The other PR has landed, so the original core issue is fixed here. We can revisit this in the future