Remove templating of osx crosstool

[keith]

Previously this file was templated so that
cxx_builtin_include_directories could be set on each machine. For macOS
the only paths you should need here are ones to Xcode, which with this
change would be required to live in /Applications. This mirrors the
behavior of Google's internal crosstool.

[kastiglione]

Can this ever be a cc from a locally built toolchain? For example if I build clang locally and want to use that, it seems that this change prevents me from doing that.

[aragos]

Correct. Also, clang could be installed in /Library/Developer/CommandLineTools which isn't covered by this new stricter check.

[aragos]

Besides removing some complexity, does this change actually fix a problem? Because the logic exists for a reason and removing it should be traded off for some better value.

[aragos]

Correct. Also, clang could be installed in /Library/Developer/CommandLineTools which isn't covered by this new stricter check.

[keith]

Thanks for the feedback. The reason I submitted this was:

1. This is what the internal Google crosstool is doing, so if that works internally I didn't know why it wouldn't work externally. Changing to be more like it (I assume) should help them converge
2. This removes a thing people need to think about for something like Add override point for cc_autoconf #7883

Also, clang could be installed in /Library/Developer/CommandLineTools which isn't covered by this new stricter check.

If this is the only place clang is installed, I believe this will end up going through the unix setup, which appears to whitelist / for this same field:

bazel/tools/cpp/cc_toolchain_config.bzl

Lines 1202 to 1203 in 716e19c

	elif (ctx.attr.cpu == "darwin"):
	cxx_builtin_include_directories = ["/"]

(although I'm not sure why this file and https://github.com/bazelbuild/bazel/blob/716e19c77154cec4daa32e33a4310cac07d31a00/tools/cpp/cc_toolchain_config.bzl.tpl exist so maybe this isn't the case)

Should we do that instead?

[keith]

@aragos any thoughts?

[aragos]

Added @trybka as I think he's working in the same space and may be submitting a change accomplishing something similar to yours.

[trybka]

Is it actually reasonable to expect external users to always have Xcode under /Applications?

One of the most common complaints we get is when users have it under /Users/Me/Applications.

Of course, internally we can mandate that they move it. But can we force the same thing externally?

(This is a question for the Bazel folks) @aragos

[aragos]

I don't have a strong opinion on restricting the directories to /Applications/. Honestly I don't feel like the extra safety is worth the trouble. The only thing that would make sense is to first detect installed xcodes and then use their known locations - but that would go out of date and put us into similar unhappy situations as with xcode_locator so let's not do that.

[keith]

@aragos updated!

[aragos]

As you can see above this change fails some tests. Please address the failures and then I'll import it.

[keith]

Both of these tests are failing because they expected a build failure because of these paths being invalidated. Which I guess means it wasn't checking this codepath https://github.com/keith/bazel/blob/59254f64fba89c822543c347669b538177cd35f8/tools/cpp/cc_toolchain_config.bzl#L1202-L1203 do you have a recommendation for a solution to them?

[aragos]

This test fails as expected:

bazel/src/test/py/bazel/cc_import_test.py

Line 244 in 3f7f255

self.AssertExitCode(exit_code, 1, stderr)

- you've made it legal for all files on the filesystem to be included, regardless of whether they're declared as an input or not. That's the tradeoff we chose to make all possible Xcode locations valid. The test will have to be skipped for darwin builds as a result (although I'm not sure how to do that within the python integration test framework, you'll have to find out).

Not sure why this failure is happening, it may be unrelated to your change (or not :): http://go/gh/bazelbuild/bazel/blob/0b03a28701d297c9ffe976c54580ceff32eec1b7/src/test/shell/bazel/cc_integration_test.sh#L106, further investigation/rerun is needed.

[keith]

I've opted out of that test, we'll see if the other one fails again

[keith]

@hlopko since you added the test that's failing here can you help me understand if this is still an issue with this change?

[keith]

@hlopko friendly ping

[keith]

This was done in a different way here ac3d06b

It's still using the previous include dirs, so if we'd prefer / like what Google uses it could still be a follow up. But given the non-response here I'm going to close anyways.