Allow tool_path to be unnormalized.

[benjaminp]

1. It's sometimes useful to use relative paths for crosstool tools. For example, one can share common wrapper scripts between crosstools.

2. The ActionConfig.Tool.tool_path field is allowed to contain unnormalized paths.

3. This fixes an ugly skyframe traceback that you used to get by trying to use an unnormalized path:

java.lang.IllegalStateException: java.lang.RuntimeException: Unrecoverable error while evaluating node 'ConfigurationFragmentKey(class=com.google.devtools.build.lib.rules.cpp.CppConfiguration, checksum=ee0f8e363e5740c160cf05602b0ed15f)' (requested by nodes 'f65da6650cd83ab54356239a660f2d61')
	at com.google.devtools.build.lib.skyframe.SkyframeExecutor.evaluateSkyKeys(SkyframeExecutor.java:1762)
	at com.google.devtools.build.lib.skyframe.SkyframeExecutor.getConfigurations(SkyframeExecutor.java:1594)
	at com.google.devtools.build.lib.skyframe.SkyframeExecutor.createConfigurations(SkyframeExecutor.java:1203)
	at com.google.devtools.build.lib.buildtool.BuildTool.buildTargets(BuildTool.java:201)
	at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:350)
	at com.google.devtools.build.lib.runtime.commands.BuildCommand.exec(BuildCommand.java:75)
	at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.execExclusively(BlazeCommandDispatcher.java:472)
	at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:199)
	at com.google.devtools.build.lib.server.GrpcServerImpl.executeCommand(GrpcServerImpl.java:855)
	at com.google.devtools.build.lib.server.GrpcServerImpl.access$2100(GrpcServerImpl.java:111)
	at com.google.devtools.build.lib.server.GrpcServerImpl$2.lambda$run$0(GrpcServerImpl.java:924)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.RuntimeException: Unrecoverable error while evaluating node 'ConfigurationFragmentKey(class=com.google.devtools.build.lib.rules.cpp.CppConfiguration, checksum=ee0f8e363e5740c160cf05602b0ed15f)' (requested by nodes 'f65da6650cd83ab54356239a660f2d61')
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:441)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:355)
	... 3 more
Caused by: java.lang.IllegalArgumentException: The include path '../external/root/bin/gcc' is not normalized.
	at com.google.devtools.build.lib.rules.cpp.CppToolchainInfo.computeToolPaths(CppToolchainInfo.java:805)
	at com.google.devtools.build.lib.rules.cpp.CppToolchainInfo.create(CppToolchainInfo.java:120)
	at com.google.devtools.build.lib.rules.cpp.CppConfiguration.create(CppConfiguration.java:214)
	at com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.create(CppConfigurationLoader.java:70)
	at com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.create(CppConfigurationLoader.java:42)
	at com.google.devtools.build.lib.skyframe.ConfigurationFragmentFunction.compute(ConfigurationFragmentFunction.java:64)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:363)
	... 4 more
java.lang.IllegalStateException: java.lang.RuntimeException: Unrecoverable error while evaluating node 'ConfigurationFragmentKey(class=com.google.devtools.build.lib.rules.cpp.CppConfiguration, checksum=ee0f8e363e5740c160cf05602b0ed15f)' (requested by nodes 'f65da6650cd83ab54356239a660f2d61')
	at com.google.devtools.build.lib.skyframe.SkyframeExecutor.evaluateSkyKeys(SkyframeExecutor.java:1762)
	at com.google.devtools.build.lib.skyframe.SkyframeExecutor.getConfigurations(SkyframeExecutor.java:1594)
	at com.google.devtools.build.lib.skyframe.SkyframeExecutor.createConfigurations(SkyframeExecutor.java:1203)
	at com.google.devtools.build.lib.buildtool.BuildTool.buildTargets(BuildTool.java:201)
	at com.google.devtools.build.lib.buildtool.BuildTool.processRequest(BuildTool.java:350)
	at com.google.devtools.build.lib.runtime.commands.BuildCommand.exec(BuildCommand.java:75)
	at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.execExclusively(BlazeCommandDispatcher.java:472)
	at com.google.devtools.build.lib.runtime.BlazeCommandDispatcher.exec(BlazeCommandDispatcher.java:199)
	at com.google.devtools.build.lib.server.GrpcServerImpl.executeCommand(GrpcServerImpl.java:855)
	at com.google.devtools.build.lib.server.GrpcServerImpl.access$2100(GrpcServerImpl.java:111)
	at com.google.devtools.build.lib.server.GrpcServerImpl$2.lambda$run$0(GrpcServerImpl.java:924)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.RuntimeException: Unrecoverable error while evaluating node 'ConfigurationFragmentKey(class=com.google.devtools.build.lib.rules.cpp.CppConfiguration, checksum=ee0f8e363e5740c160cf05602b0ed15f)' (requested by nodes 'f65da6650cd83ab54356239a660f2d61')
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:441)
	at com.google.devtools.build.lib.concurrent.AbstractQueueVisitor$WrappedRunnable.run(AbstractQueueVisitor.java:355)
	... 3 more
Caused by: java.lang.IllegalArgumentException: The include path '../external/root/bin/gcc' is not normalized.
	at com.google.devtools.build.lib.rules.cpp.CppToolchainInfo.computeToolPaths(CppToolchainInfo.java:805)
	at com.google.devtools.build.lib.rules.cpp.CppToolchainInfo.create(CppToolchainInfo.java:120)
	at com.google.devtools.build.lib.rules.cpp.CppConfiguration.create(CppConfiguration.java:214)
	at com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.create(CppConfigurationLoader.java:70)
	at com.google.devtools.build.lib.rules.cpp.CppConfigurationLoader.create(CppConfigurationLoader.java:42)
	at com.google.devtools.build.lib.skyframe.ConfigurationFragmentFunction.compute(ConfigurationFragmentFunction.java:64)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:363)

[irengrig]

Hi @gregestren, Bazel sheriff here. Please have a look, what is the status here? Show I reassign that?

[benjaminp]

Maybe @lberki has an opinion.

[gregestren]

Sorry for my delayed response - I was offline the last two weeks.

Since this is CROSSTOOL-related I'd run this by @mhlopko

[lberki]

Opinions are something I have in abundance :) I think Bazel crashing on some input is just unacceptable no matter what, so we should have proper error reporting at the very least.

I'm reluctant to just allow blanket uplevel references, though, because it makes it easy to depend on a binary that's in the output tree but is not a declared input (not a big deal) or, worse yet, have enough .. segments to escape the repository that contains the CROSSTOOL file or even the execroot, thus opening up the possibility of Bazel only working if your home directory / output base have the exact required number of path segments (consider tool_path = "../../../../../usr/bin/gcc")

Is there an actual use case for this or it's just for general code tidiness?

[benjaminp]

I'm reluctant to just allow blanket uplevel references, though, because it makes it easy to depend on a binary that's in the output tree but is not a declared input (not a big deal) or, worse yet, have enough .. segments to escape the repository that contains the CROSSTOOL file or even the execroot, thus opening up the possibility of Bazel only working if your home directory / output base have the exact required number of path segments (consider tool_path = "../../../../../usr/bin/gcc")

I don't see how that's fundamentally different than writing, e.g., a genrule with an up-level reference. Additionally, you can make a perfectly nonhermetic toolchain without any up-level references by improperly filling the compiler_files, linker_files, etc. cc_toolchain attributes correctly.

(Either case will hopefully be caught by sandboxing.)

Is there an actual use case for this or it's just for general code tidiness?

I want to share wrapper scripts between crosstools.

[hlopko]

I cannot comment on the genrule (since I had no idea we allowed non-normalized paths), but for the CROSSTOOL, we don't want to support this (mostly for the bright future, when CROSSTOOL uses artifacts). I'll upload a cl fixing the crash, and unless I'm convinced otherwise, I'll upload a change adding an incompatible flag to also stop allowing non-normalized paths in action_configs.

[lberki]

Uplevel references in genrules are also a bad idea. But we can't prohibit them, firstly because a significant use case for genrules is running commands "outside" of the Bazel world (e.g. /usr/bin/<whatever>), which is not realistic to prohibit, secondly, because even if I wanted to do that, I couldn't because we just hand the cmd attribute over to a shell and you can add as many .. path segments as you want with a little creativity.

But let's not make the hole bigger than it needs to be. One could argue that genrule is a special-case and it being an escape hatch is by design, but that's not the case with our C++ toolchain infrastructure.

[benjaminp]

I'm not trying to escape any sort of hermeticity. I just want to escape the artificial sandbox of the directory where the CROSSTOOL file is. I'm happy to declare the file I'm referencing in compiler_files. Maybe this will be addressed by the Starlark crosstool rules?

[ulfjack]

Tool paths can't be artifacts as long as artifacts can't be at absolute paths, and as long as we support crosstools at absolute paths. I don't immediately see why this patch would be problematic, although maybe we can check that the path is normalized after concatenating it with the crosstool root.

[katre]

CppToolchainInfo doesn't exit and the entire basis of cc_toolchain has shifted, to actually use artifacts, so I am closing this PR.

If you still see a problem, please feel free to open a new issue regarding this.