Invalid variables in output names give confusing errors

[katre]

Please see the example reproduction at https://github.com/katre/invalid-output-name-test-case

If I define a rule with an output, which contains an invalid variable to expand, such as:

outputs = {
        'bad_output': 'example-%{name}-%{nonexistent}.txt',
    }

When I build that, I would expect an error telling me that the 'nonexistent' variable can't be expanded and this caused an error.

Instead, I get an error that the output 'bad_output' does not exist, which is very confusing.

$ bazel build :working
INFO: Reading 'startup' options from /usr/local/google/home/jcater/.bazelrc: --watchfs
WARNING: /usr/local/google/home/jcater/repos/invalid-output-name-test-case/example.bzl:2:3: Outputs: ["out", "to_json", "to_proto"].
INFO: Found 1 target...
Target //:working up-to-date:
  bazel-bin/example-working.txt
INFO: Elapsed time: 0.240s, Critical Path: 0.01s

$ bazel build :not_working
INFO: Reading 'startup' options from /usr/local/google/home/jcater/.bazelrc: --watchfs
WARNING: /usr/local/google/home/jcater/repos/invalid-output-name-test-case/example.bzl:2:3: Outputs: ["out", "to_json", "to_proto"].
ERROR: /usr/local/google/home/jcater/repos/invalid-output-name-test-case/BUILD:7:1: in example rule //:not_working: 
Traceback (most recent call last):
	File "/usr/local/google/home/jcater/repos/invalid-output-name-test-case/BUILD", line 7
		example(name = 'not_working')
	File "/usr/local/google/home/jcater/repos/invalid-output-name-test-case/example.bzl", line 9, in _example_impl
		ctx.action(command = 'echo Goodbye > %s' % ..., ...])
	File "/usr/local/google/home/jcater/repos/invalid-output-name-test-case/example.bzl", line 10, in ctx.action
		ctx.outputs.bad_output
No attribute 'bad_output' in outputs. Make sure you declared a rule output with this name.
Available attributes: out.
WARNING: errors encountered while analyzing target '//:not_working': it will not be built.
INFO: Found 0 targets...
ERROR: command succeeded, but not all targets were analyzed.
INFO: Elapsed time: 0.151s, Critical Path: 0.00s

[hvardhanx]

I would like to work on this issue.

[hvardhanx]

Can you provide some details on this? I was looking through ImplicitOutputsFunction.java. Is this the file which is related to this issue? @laurentlb @katre

[katre]

The problem is with invalid explicit (not implicit) outputs. I haven't looked into the code carefully enough to determine where, however.

Thanks for investigating this!

[gcc42]

@katre While looking at another bug, I stumbled onto this. On investigating, it turns out that the problem is in the ImplicitOutputsFunction. The SkylarkRuleContext constructor while adding the implicit outputs calls calculateOutputs here, which in turn uses fromTemplates (which internally calls substitutePlaceholderIntoTemplate which returns an empty list of subsitutions when any of the variable names is invalid).

To show a sensible error here, we would have to throw an Exception from substitutePlaceholderIntoTemplate here if any of the placeholders turn out to be invalid. Would this be okay to do?

[katre]

You're right, that looks like the cause.

Throwing an exception there will be fine, but it needs to be a checked exception so we can fix all of the callers to handle it properly.

[gcc42]

Understood. Should I fix this and submit to Gerrit?

[katre]

I'll be happy to review a patch.

[gcc42]

Great, I'll get on this, then. One question, in the handling of this exception, the callers must further throw an exception, right? I mean, this is an error, right?

[katre]

Generally, yes, the exception should either be wrapped in whatever exceptions the caller is already throwing, or handled in another fashion.

[gcc42]

@katre I have fixed this and pushed to Gerrit for review(c9490), please review when you get the time.

A couple of things to note:

1. The invalid variables now give an error like so

$ bbzl build examples/inv_output_name:not_working
Warning: ignoring http_proxy in environment.
.
ERROR: /home/pranx/storage/bazel/examples/inv_output_name/BUILD:3:1: In rule //examples/inv_output_name:working: While evaluating implicit outputs: For attribute 'bad_output' in outputs: Invalid placeholder(s) in template.
ERROR: /home/pranx/storage/bazel/examples/inv_output_name/BUILD:7:1: In rule //examples/inv_output_name:not_working: While evaluating implicit outputs: For attribute 'bad_output' in outputs: Invalid placeholder(s) in template.
ERROR: error loading package 'examples/inv_output_name': Package 'examples/inv_output_name' contains errors.
INFO: Elapsed time: 0.551s

2. I couldn't add the exact placeholder name in the error message because the function substitutePlaceholderIntoTemplate is called from SafeImplicitOutputsFunction.getImplicitOutputs which by definition, can't throw an EvalException(see here), thus I had to be a little vague. (Don't know why this is explicitly made this way)

3. The calculateOutputs function which throws an error is initially called while constructing the rule in Rule.populateImplicitOutputFiles here, which evaluates the output labels but only to check that they are not invalid. So if there is an EvalException, the execution never reached SkylarkRuleContext constuctor.

Please tell me if any changes are required. Sorry for the long posts, this is my first code contribution to Bazel :).