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

Expose current repository name to Java with @AutoBazelRepository #16534

Closed
wants to merge 5 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Oct 23, 2022

Java targets depending on @bazel_tools//tools/java/runfiles can add the new @AutoBazelRepository to a class to have an annotation processor generate a companion class with a BAZEL_REPOSITORY constant containing the repository name of the target that compiled the class.

This requires a small addition to JavaBuilder to parse the repository name out of the target label and pass it to javac as a processor option.

Work towards #16124

@fmeum fmeum force-pushed the 16124-auto-bazel-repository branch 4 times, most recently from 2ad9baa to e743290 Compare October 24, 2022 10:13
@fmeum fmeum changed the title WIP: Implement AutoBazelRepository Expose current repository name to Java with @AutoBazelRepository Oct 24, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 24, 2022

@cushon I followed your recommendation in #16281 (comment) and experimented with annotation processing rather than JavaBuilder magic. The ergonomics seem decent enough and the only change to JavaBuilder is passing the repository name via -A.

The integration test is most likely not in the correctly place, I only put it there so that the test only runs against java_tools built from HEAD. How would I do this properly - split up the PR into two parts with a java_tools update in between?

@Wyverald Do you think this is usable?

@fmeum fmeum marked this pull request as ready for review October 24, 2022 10:19
@fmeum fmeum requested a review from a team as a code owner October 24, 2022 10:19
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 24, 2022

I don't understand the test failure - locally, bazel test //src/test/shell/bazel:bazel_java17_test passes. Any idea what could be up with that?

@ShreeM01 ShreeM01 added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-user-response Awaiting a response from the author labels Oct 24, 2022
@fmeum fmeum force-pushed the 16124-auto-bazel-repository branch from 6c63677 to bb84246 Compare October 25, 2022 20:35
@fmeum fmeum requested a review from lberki as a code owner October 25, 2022 20:35
@fmeum
Copy link
Collaborator Author

fmeum commented Oct 25, 2022

I changed this commit so that the flag is injected in JavaCommon rather than JavaBuilder. That does require duplicating it in two places though.

@sgowroji sgowroji added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Oct 26, 2022
@fmeum fmeum force-pushed the 16124-auto-bazel-repository branch from bb84246 to 30c63c4 Compare October 28, 2022 21:16
out.printf(" * The canonical name of the repository containing the Bazel target that\n");
out.printf(" * compiled {@link %s}.\n", annotatedClass.getQualifiedName().toString());
out.printf(" */\n");
out.printf(" static final String BAZEL_REPOSITORY = \"%s\";\n", repositoryName);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cushon Given that the class is called AutoBazelRepository_X, should I maybe replace this with NAME? More concise and less repetitive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did this in a separate commit because it just seemed better, let me know if you disagree and I can revert.

Copy link
Member

@Wyverald Wyverald left a comment

Choose a reason for hiding this comment

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

looks good!

@comius comius self-requested a review November 3, 2022 14:35
private void emitClass(TypeElement annotatedClass) {
// This option is always provided by JavaBuilder.
String repositoryName = processingEnv.getOptions()
.getOrDefault("bazel.repository", "for testing only");
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better, that this results in an error? User wants to get repository, but they get a string "for testing only". This can only result in a hard to debug problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made this a hard error.

tools/java/runfiles/testing/AutoBazelRepositoryTest.java Outdated Show resolved Hide resolved
@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Nov 4, 2022
@fmeum
Copy link
Collaborator Author

fmeum commented Nov 7, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 7, 2022
@Wyverald
Copy link
Member

Wyverald commented Nov 7, 2022

@bazel-io fork 6.0.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Nov 7, 2022
Java targets depending on `@bazel_tools//tools/java/runfiles` can add
the new `@AutoBazelRepository` to a class to have an annotation
processor generate a companion class with a `BAZEL_REPOSITORY` constant
containing the repository name of the target that compiled the class.
@fmeum fmeum force-pushed the 16124-auto-bazel-repository branch from fd68e9c to c4ddafa Compare November 9, 2022 21:13
fmeum added a commit to fmeum/bazel that referenced this pull request Nov 10, 2022
Java targets depending on `@bazel_tools//tools/java/runfiles` can add the new `@AutoBazelRepository` to a class to have an annotation processor generate a companion class with a `BAZEL_REPOSITORY` constant containing the repository name of the target that compiled the class.

This requires a small addition to JavaBuilder to parse the repository name out of the target label and pass it to javac as a processor option.

Work towards bazelbuild#16124

Closes bazelbuild#16534.

PiperOrigin-RevId: 487573496
Change-Id: Id9b6526ce32268089c91c6d17363d1e7682f64a4
Wyverald pushed a commit that referenced this pull request Nov 10, 2022
)

Java targets depending on `@bazel_tools//tools/java/runfiles` can add the new `@AutoBazelRepository` to a class to have an annotation processor generate a companion class with a `BAZEL_REPOSITORY` constant containing the repository name of the target that compiled the class.

This requires a small addition to JavaBuilder to parse the repository name out of the target label and pass it to javac as a processor option.

Work towards #16124

Closes #16534.

PiperOrigin-RevId: 487573496
Change-Id: Id9b6526ce32268089c91c6d17363d1e7682f64a4
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants