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

Use java macros in java_tools. #8758

Closed
wants to merge 8 commits into from
Closed

Use java macros in java_tools. #8758

wants to merge 8 commits into from

Conversation

iirina
Copy link
Contributor

@iirina iirina commented Jul 1, 2019 •
edited
Loading

@rules_java repository is now available to all Bazel java users. There is the possibility to override the repository in the users' WORKSPACE file in order to safely migrate #8746.

Progress on #8746.

@iirina iirina requested a review from hlopko July 1, 2019 10:51
hlopko
hlopko suggested changes Jul 1, 2019
View reviewed changes
Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

Does this mean that java_tools will fetch rules_java? Will rules_java be bundled in Bazel (even as a http_archive)?

I'm wondering if the direction shouldn't be reversed, rules_java should fetch java_tools, wdyt?

@iirina
Copy link
Contributor Author

iirina commented Jul 1, 2019

Does this mean that java_tools will fetch rules_java?

Yes, after a java_tools release containing this PR the java_tools will fetch rules_java.

Will rules_java be bundled in Bazel (even as a http_archive)?

Not necessarily. My plan was to migrate users to adding rules_java to their own WORKSPACE file. I'm trying to figure out the consequences of embedding rules_java into Bazel. 1) It's not clear to me if/how users can override rules_java repository. 2) It's not clear to me if there's a way to embed @rules_java without the archive being always fetched. I would like to avoid the "why-is-bazel-downloading-java-stuff-for-my-non-java-build" story.

I'm wondering if the direction shouldn't be reversed, rules_java should fetch java_tools, wdyt?

In theory they both need each other. I don't see how we can avoid adding a repository dependency on ruels_java from java_tools. Until we migrate the rules to Starlark it doesn't make much sense for rules_java to embed/use java_tools.

@iirina
Copy link
Contributor Author

iirina commented Jul 3, 2019

@hlopko as discussed offline I've added @rules_java to the workspace suffix. PTAL.

hlopko
hlopko reviewed Jul 3, 2019
View reviewed changes
Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

Pls mention in the cl description that:

This PR makes @rules_java available to every Bazel user, making #8746 a little bit less effective (people can forget to add rules_java into their WORKSPACE and use the @rules_java from workspace suffix. This is fine, we'll follow up with another incompatible change that removes both @rules_java and @java_tools from Bazel.

This PR does not prevent people from adding their own rules_java repository into the WORKSPACE file, so well-behaving users can safely migrate for #8746.

hlopko
hlopko reviewed Jul 3, 2019
View reviewed changes
tools/jdk/WORKSPACE.java_tools Outdated
@@ -0,0 +1,8 @@
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
Copy link
Member

Choose a reason for hiding this comment

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

What's jdk.WORKSPACE for and what's WORKSPACE.java_tools for? Do we need both of them to define rules_java?

iirina
iirina commented Jul 3, 2019
View reviewed changes
Copy link
Contributor Author

@iirina iirina left a comment

Choose a reason for hiding this comment

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

@hlopko I removed WORKSPACE.java_tools because it's not needed if @rules_java is in the workspace suffix.

jdk.WORKSPACE is the java specific part of the implicit workspace suffix.
WORKSPACE.java_tools was the WORKSPACE file of the java_tools repository (not required anymore)

src/test/shell/bazel/bazel_java_tools_test.sh Show resolved Hide resolved
@iirina
Copy link
Contributor Author

iirina commented Jul 3, 2019

@hlopko you can take another look.

@iirina iirina requested a review from hlopko July 3, 2019 08:55
hlopko
hlopko approved these changes Jul 3, 2019
View reviewed changes
Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Do we need to tell users about @rules_java? :) They'll be better off if they put their own into their workspace.

@bazel-io bazel-io closed this in e826049 Jul 3, 2019
siberex pushed a commit to siberex/bazel that referenced this pull request Jul 4, 2019
@iirina @siberex
Use java macros in java_tools.
a24c31c 
`@rules_java` repository is now available to all Bazel java users. There is the possibility to override the repository in the users' WORKSPACE file in order to safely migrate bazelbuild#8746.

Progress on bazelbuild#8746.

Closes bazelbuild#8758.

PiperOrigin-RevId: 256347290
irengrig pushed a commit to irengrig/bazel that referenced this pull request Jul 15, 2019
@iirina @irengrig
Use java macros in java_tools.
ff06eed 
`@rules_java` repository is now available to all Bazel java users. There is the possibility to override the repository in the users' WORKSPACE file in order to safely migrate bazelbuild#8746.

Progress on bazelbuild#8746.

Closes bazelbuild#8758.

PiperOrigin-RevId: 256347290
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlopko hlopko hlopko approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@iirina @hlopko @googlebot