Regression in 3.5.0RC1: external repo with malformed WORKSPACE causes spurious errors

[konste]

Description of the problem / feature request:

In the WORKSPACE file we have git_repository rule which pulls in another git repo. That git repo happens to have its own WORKSPACE file which is very old and is not correct for the recent versions of Bazel. Up until Bazel 3.4.1 that secondary WORKSPACE was rightfully completely ignored. Starting with the Bazel 3.5.0RC1 external WORKSPACE file seems to be analyzed for some reason and causes build errors.

Feature requests: what underlying problem are you trying to solve with this feature?

Accordingly to the docs only the primary WORKSPACE matters, all others should be ignored.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Use git_repository to pull several years old com_google_protobuf. Its workspace file would not be correct from the recent Bazel point of view and cause errors, while it should be ignored.

What operating system are you running Bazel on?

Linux, Mac, Windows

What's the output of bazel info release?

3.5.0RC1

[aiuto]

Do you have a specific old protobuf version to point to? That would save some time.

[konste]

I got a repro with v3.6.0 Commit SHA ab8edf1dbe2237b4717869eaab11a2998541ad8d.

[aiuto]

@alandonovan @brandjon You two are the most likely to have touched WORKSPACE loading recently. Any ideas?

[laurentlb]

Judging by the title of the issue, it might be the same problem as #11837 (comment)

cc @michajlo

[konste]

@laurentlb it does not look the same to me. #11837 is about WORKSPACE name. This issue is about WORKSPACE file in the repo pulled in by git_repository. That external WORKSPACE file should not be analysed at all and may contain garbage. This was the case till 3.4.1, but with 3.5.0RC1 it is clearly analysed by Bazel and syntax errors in that file are now reported as the errors in the build.

[laurentlb]

Do you have a repro?
When I use references to the old protobuf repo (load or dependency), I get legitimate errors. And without reference, Bazel won't download it.

[konste]

Give me couple hours.

[aiuto]

I will want to add a regression test for this. What would be interesting to see in the repo is if this is about git_repository only or any repository. A local repository (or http_repository with a file:// scheme) with a corrupt WORKSPACE would be a nicely self-contained test.

[michajlo]

Could it be that workspaces were always being analyzed but errors were ignored and/or lost until now?

[michajlo]

As in, this could be another misbug fixed by 2436a35.

[konste]

@aiuto Here is what you asked for: https://github.com/Bazel-snippets/repro_11936
Secondary WORKSPACE file only contains the word garbage.

d:\dev\bazel\repro\repro_11936>bazel-3.4.1-windows-x86_64.exe  build hello-world
Starting local Bazel server and connecting to it...
INFO: Analyzed target //:hello-world (15 packages loaded, 62 targets configured).
INFO: Found 1 target...
Target //:hello-world up-to-date:
  bazel-bin/hello-world.exe
INFO: Elapsed time: 9.539s, Critical Path: 1.39s
INFO: 4 processes: 4 local.
INFO: Build completed successfully, 9 total actions

d:\dev\bazel\repro\repro_11936>bazel-3.5.0rc2-windows-x86_64.exe   build hello-world
Starting local Bazel server and connecting to it...
ERROR: C:/users/kerman/_bazel_kerman/beoihmbf/external/secondary_workspace/WORKSPACE:1:1: name 'garbage' is not defined
INFO: Analyzed target //:hello-world (16 packages loaded, 63 targets configured).
INFO: Found 1 target...
Target //:hello-world up-to-date:
  bazel-bin/hello-world.exe
INFO: Elapsed time: 9.006s, Critical Path: 0.06s
INFO: 0 processes.
INFO: Build completed successfully, 1 total action

Despite the error the second build claims it is successful, so @michajlo is probably right that workspaces were always being analyzed but errors were ignored and/or lost until now

[laurentlb]

Thanks a lot for the repro!

I confirm this is caused 2436a35.

[michajlo]

Talked to @aiuto about this over chat. AFAICT this is just extra console output about real errors that were hidden before. Assigning to @aiuto to confirm that this is fine/if not find someone who knows WORKSPACE things better.

[konste]

The doc says: As external repositories are repositories themselves, they often contain a WORKSPACE file as well. However, these additional WORKSPACE files are ignored by Bazel. Reporting errors in the files which supposed to be ignored is very confusing, especially considering it is a regression. The fact that those red-colored errors in the console output do not fail the build only makes it more confusing.
Is there a way to actually skip non-primary WORKSPACE files from the analysis?

[aiuto]

Thanks. That answers so many questions.

• this is not git_repository specific. Any repo does it
• this is very likely a result in better error processing.
• it looks like this is just console spam and not an actual build error.

So

• we won't roll back the fix to error processing.
• we should fix the problem of non-primary WORKSPACE files being analyzed.
  • I am not sure if this is a 3.5 release blocker or not, since there is a reasonable workaround (patch_cmds).
  • This might be a 4.0 blocker. That is also not clear because it raises the question of if invalid WORKSPACE files
    (even if unused) should be treated fatally or not.

[laurentlb]

Could we revert to the previous behavior (for now) and ask @philwo's team to investigate why WORKSPACE files are loaded?

[aiuto]

There are too many changes to the error reporting stack to do a rollback of that. It would be dependent on a fix forward to suppress the reporting if we want a short term fix.

[michajlo]

I'm not sure the However, these additional WORKSPACE files are ignored by Bazel documentation is correct. https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java#L65 loads the WORKSPACE file from a local_repository, which I confirmed by stepping through a random test that uses local_repository in a debugger. The only thing 2436a35 changed is events from that loading are no longer suppressed. Reverting that change (and at least c17b80e which depends on it) will suppress the warnings, but it won't stop the files from being loaded.

[laurentlb]

I'd rather suppress the messages and keep the existing behavior, that seems safer to me.

Changes in the behavior can be decided later (when we know what we're doing).

[aiuto]

I think we have a handle on the problem so we are going to try to fix forward and remove the unneeded parsing of the workspace. I have some tests I can try on Monday.

[aiuto]

I think I have a solution. I'm just trying to figure out where to hang a real regression test.
I turned the repro into a first pass test case in this pr https://github.com/Bazel-snippets/repro_11936/pulls

The fix is basically deleting ~35 lines of code.

==== third_party/bazel/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java#x - third_party/bazel/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryLoaderFunction.java ====
17d16
< import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
19d17
< import com.google.devtools.build.lib.packages.WorkspaceFileValue;
21,22d18
< import com.google.devtools.build.lib.vfs.Root;
< import com.google.devtools.build.lib.vfs.RootedPath;
50,80d45
< RootedPath workspaceFilePath;
< try {
< workspaceFilePath =
< WorkspaceFileHelper.getWorkspaceRootedFile(Root.fromPath(repository.getPath()), env);
< if (workspaceFilePath == null) {
< return null;
< }
< } catch (IOException e) {
< throw new RepositoryLoaderFunctionException(
< new IOException(
< "Could not determine workspace file ("WORKSPACE.bazel" or "WORKSPACE"): "
< + e.getMessage()),
< Transience.PERSISTENT);
< }
< SkyKey workspaceKey = WorkspaceFileValue.key(workspaceFilePath);
< WorkspaceFileValue workspacePackage = (WorkspaceFileValue) env.getValue(workspaceKey);
< if (workspacePackage == null) {
< return null;
< }
<
< try {
< String workspaceNameStr = workspacePackage.getPackage().getWorkspaceName();
< if (workspaceNameStr.isEmpty()) {
< RepositoryName.create("");
< } else {
< RepositoryName.create("@" + workspaceNameStr);
< }
< } catch (LabelSyntaxException e) {
< throw new IllegalStateException(e);
< }
<