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

Fix NPE when setting --java_classpath=bazel #16636

Closed
wants to merge 1 commit into from
Closed

Fix NPE when setting --java_classpath=bazel #16636

wants to merge 1 commit into from

Conversation

arunkumar9t2
Copy link
Contributor

Fixes #16635

@sgowroji sgowroji added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Nov 3, 2022
@lberki lberki requested review from comius and removed request for lberki November 3, 2022 07:38
comius
comius reviewed Nov 17, 2022
View reviewed changes
src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
Comment on lines +190 to +193
// Register JavaCompileActionContext for java_classpath=bazel to work
actionContextRegistryBuilder.register(
JavaCompileActionContext.class, new JavaCompileActionContext());

Copy link
Contributor

Choose a reason for hiding this comment

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

A better location to do this registration is BazelStrategyModule overriding registeringActionContext. The module allows for better extensibility, that is potential use of a different compilation context.

Please move it there.

src/main/java/com/google/devtools/build/lib/rules/java/BUILD
@@ -79,6 +79,19 @@ java_library(
],
)

java_library(
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bad practice to have a source file part of two rules. Please remove JavaCompileActgionContext.java from the other rule where it's still used and fix dependencies if needed.

comius
comius requested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@comius comius left a comment

Choose a reason for hiding this comment

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

Does the bazel classpath work after this fix?

Could you also add a regression test for this NPE?

@comius comius added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 18, 2022
benjaminp added a commit to benjaminp/bazel that referenced this pull request Dec 5, 2022
@benjaminp
Register JavaCompileActionContext in Bazel.
f1d3fa1 
This makes --experimental_java_classpath=bazel work.

This CL is similar to bazelbuild#16636 but applies the code review feedback. (Though, I put the context registration in BazelRulesModule rather than BazelStrategyModule as JavaCompileActionContext isn't really an strategy.)

Fixes bazelbuild#16635.
@benjaminp benjaminp mentioned this pull request Dec 5, 2022
Closed
benjaminp added a commit to benjaminp/bazel that referenced this pull request Dec 6, 2022
@benjaminp
Register JavaCompileActionContext in Bazel.
27752b5 
This makes --experimental_java_classpath=bazel work.

This CL is similar to bazelbuild#16636 but applies the code review feedback. (Though, I put the context registration in BazelRulesModule rather than BazelStrategyModule as JavaCompileActionContext isn't really a strategy.)

Fixes bazelbuild#16635.
benjaminp added a commit to benjaminp/bazel that referenced this pull request Dec 6, 2022
@benjaminp
Register JavaCompileActionContext in Bazel.
8278bdd 
This makes --experimental_java_classpath=bazel work.

This CL is similar to bazelbuild#16636 but applies the code review feedback. (Though, I put the context registration in BazelRulesModule rather than BazelStrategyModule as JavaCompileActionContext isn't really a strategy.)

Fixes bazelbuild#16635.
@comius
Copy link
Contributor

comius commented Dec 13, 2022

The same thing done #16921
addressing review comments.

@comius comius closed this Dec 13, 2022
copybara-service bot pushed a commit that referenced this pull request Dec 13, 2022
@benjaminp @copybara-github
Register JavaCompileActionContext in Bazel.
ac504cb 
This makes --experimental_java_classpath=bazel work.

This CL is similar to #16636 but applies the code review feedback. (Though, I put the context registration in BazelRulesModule rather than BazelStrategyModule as JavaCompileActionContext isn't really a strategy.)

Fixes #16635.

Closes #16921.

PiperOrigin-RevId: 494950719
Change-Id: Ia32e2d0e67ddacaf18ba52591f7b67b3c6c7b792
@arunkumar9t2
Copy link
Contributor Author

Sorry could not follow up on review comments, thanks @comius @benjaminp!

@ShreeM01 ShreeM01 mentioned this pull request Jan 25, 2023
Merged
ShreeM01 added a commit that referenced this pull request Jan 25, 2023
@ShreeM01 @benjaminp
Register JavaCompileActionContext in Bazel. (#17325)
2829dac 
This makes --experimental_java_classpath=bazel work.

This CL is similar to #16636 but applies the code review feedback. (Though, I put the context registration in BazelRulesModule rather than BazelStrategyModule as JavaCompileActionContext isn't really a strategy.)

Fixes #16635.

Closes #16921.

PiperOrigin-RevId: 494950719
Change-Id: Ia32e2d0e67ddacaf18ba52591f7b67b3c6c7b792

Co-authored-by: Benjamin Peterson <benjamin@engflow.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comius comius comius requested changes

Assignees
No one assigned
Labels
awaiting-user-response Awaiting a response from the author team-Rules-Java Issues for Java rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bazel NPE when setting --java_classpath=bazel
4 participants
@arunkumar9t2 @comius @sgowroji @arunsampathkumar-grabtaxi