Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bazel reports build success when it was not successful #7115

Closed
meteorcloudy opened this issue Jan 14, 2019 · 10 comments
Closed

Bazel reports build success when it was not successful #7115

meteorcloudy opened this issue Jan 14, 2019 · 10 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug

Comments

@meteorcloudy
Copy link
Member

This bug was filed at bazelbuild/continuous-integration#449, but it should be a Bazel bug

/cc @c-parsons @laurentlb

@meteorcloudy meteorcloudy added type: bug P1 I'll work on this now. (Assignee required) team-Starlark labels Jan 14, 2019
@laurentlb laurentlb added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc and removed team-Starlark labels Jan 14, 2019
@laurentlb
Copy link
Contributor

In the interpreter, we throw an exception for this. It is properly reported one level up: package contains errors: e2e/output_runner. But later, something swallows the error. I don't know what happens, I suspect it's related to --keep_going, so assigning to core team to investigate.

@laurentlb
Copy link
Contributor

Repro:

touch WORKSPACE
mkdir sub
touch test.sh && chmod +x test.sh
echo 'sh_test(name = "test", srcs = ["test.sh"])' > BUILD
echo 'parse error' > sub/BUILD
bazel test //... -k

If one package cannot be loaded, but all other tests pass, Bazel returns 0.

@laurentlb
Copy link
Contributor

In the repro above: if sub/BUILD declares at least one target, the exit code is properly set to 1.

cc @ericfelly who should own this?

@ericfelly ericfelly assigned ericfelly and haxorz and unassigned ericfelly Jan 14, 2019
@ericfelly
Copy link
Contributor

Nathan, can you take a look?

@haxorz
Copy link
Contributor

haxorz commented Jan 14, 2019

is this really P1?

@jin
Copy link
Member

jin commented Jan 14, 2019

@haxorz if it's not a P1 according to the definitions, please reassign the correct priority. thanks!

@meteorcloudy
Copy link
Member Author

If we don't fix this issue, Bazel users (including us) could miss potential failures in the build, I think it's a pretty serious issue (P1).

@laurentlb
Copy link
Contributor

I had a quick look yesterday. This issue also affects Google (Blaze) and has been around for at least a year. I'm surprised it didn't come up before.

@haxorz
Copy link
Contributor

haxorz commented Jan 15, 2019

ok, i will look into this today

@meteorcloudy
Copy link
Member Author

Thank you!

weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
…n encountering a package loading error during target pattern parsing of patterns like "//foo:all" and "//foo/...".

The bug is that we were completely ignoring package loading errors in the SkyFunction work that went into TargetPatternPhaseValue computation, and so therefore the control flow all the way from SkyframeExecutor#loadTargetPatterns to BuildView#createErrorMessage could never hope to take such a package loading error into account.

Fixes bazelbuild#7115

RELNOTES: In --keep_going mode, Bazel now correctly returns a non-zero exit code when encountering a package loading error during target pattern parsing of patterns like "//foo:all" and "//foo/...".
PiperOrigin-RevId: 229839139
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc type: bug
Projects
None yet
Development

No branches or pull requests

5 participants