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.

Sign up for GitHub

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

Jump to bottom

Don't mask error in cquery transitions output formatter #23210

Closed
wants to merge 2 commits into from
Closed

Don't mask error in cquery transitions output formatter #23210

wants to merge 2 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 5, 2024 •
edited
Loading

All errors used to be wrapped in an InterruptedException instead of being reported with a message. Also show a warning when an incompatible target is skipped instead of failing the build.

Work towards #21010

@fmeum
Don't mask error in cquery transitions output formatter
465e5f2 
All errors used to be wrapped in an `InterruptedException` instead of being reported with a message.
Also show a warning when an incompatible target is skipped instead of failing the build.
@fmeum fmeum requested a review from gregestren August 5, 2024 12:14
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Aug 5, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 5, 2024

@keith @illicitonion Do you think this handling of incompatible targets is reasonable?

gregestren
gregestren reviewed Aug 5, 2024
View reviewed changes
src/test/java/com/google/devtools/build/lib/query2/cquery/ProtoOutputFormatterCallbackTest.java Outdated

options.transitions = Transitions.LITE;
AnalysisProtosV2.CqueryResult cqueryResult =
getProtoOutput("deps(//test:my_target)", AnalysisProtosV2.CqueryResult.parser());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume getProtoOutput before would trigger the InterruptedException that auto-failed this test? Could we, just for clarity, add a line verifying the result isn't null and/or checking the warning message?

Also, what are you distinctly testing here compared to the shell test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a leftover from an earlier attempt at a Java test that ultimately failed. I removed it.

@gregestren gregestren removed the awaiting-review PR is awaiting review from an assigned reviewer label Aug 5, 2024
@keith
Copy link
Member

keith commented Aug 5, 2024

tested and it seems to work for our case, thanks!

illicitonion
illicitonion reviewed Aug 5, 2024
View reviewed changes
Copy link
Contributor

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very reasonable - thanks!

@fmeum fmeum requested a review from gregestren August 6, 2024 06:39
@gregestren gregestren added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 6, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 6, 2024

@bazel-io fork 7.4.0

@bazel-io bazel-io mentioned this pull request Aug 6, 2024
Closed
@copybara-service copybara-service bot closed this in 300c586 Aug 6, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 6, 2024
@fmeum fmeum deleted the 21010-transition-error-handling branch August 7, 2024 08:53
@gregestren gregestren mentioned this pull request Aug 12, 2024
Closed
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 19, 2024
@fmeum @iancha1992
Don't mask error in cquery transitions output formatter
16f54ac 
All errors used to be wrapped in an `InterruptedException` instead of being reported with a message. Also show a warning when an incompatible target is skipped instead of failing the build.

Work towards bazelbuild#21010

Closes bazelbuild#23210.

PiperOrigin-RevId: 660116656
Change-Id: I22651110984e471a6b31dcc59a387f3f85ad49bc
@iancha1992 iancha1992 mentioned this pull request Aug 19, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2024
@iancha1992 @fmeum
[7.4.0] Don't mask error in cquery transitions output formatter (#2…
06f99bb 
…3351)

All errors used to be wrapped in an `InterruptedException` instead of
being reported with a message. Also show a warning when an incompatible
target is skipped instead of failing the build.

Work towards #21010

Closes #23210.

PiperOrigin-RevId: 660116656
Change-Id: I22651110984e471a6b31dcc59a387f3f85ad49bc


300c586

Co-authored-by: Fabian Meumertzheim <[email protected]>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 7.4.0 RC1. Please test out the release candidate and report any issues as soon as possible.
If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=7.4.0rc1. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@illicitonion illicitonion illicitonion left review comments

@gregestren gregestren gregestren approved these changes

Assignees
No one assigned
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fmeum @keith @iancha1992 @illicitonion @gregestren