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

Move ImportDepsChecker to a checked in deploy jar. #7140

Closed
wants to merge 3 commits into from

Conversation

iirina
Copy link
Contributor

@iirina iirina commented Jan 16, 2019

instead of embedding the deploy jar that is built when the bazel binary is built.

This comes in preparation for moving the Java tools to remote repositories (see #6316). This checked in jar will be moved to a remote repository and referenced from there.

Moving the Java tools happens in two phases:

  1. Using checked-in deploy jars/binaries instead of those built when bazel was built
  2. Moving the checked-in jars/binaries to a remote repository and make bazel reference them from there.

This PR addresses the first point for ImportDepsChecker. Doing the move incrementally this way is safer and allows finding bugs earlier. It also allows moving the Java tools code base out of the bazel repository.

We are making this change gradually for each java tool defined in JAVA_TOOLS (found in src/BUILD).

@iirina iirina requested review from ahumesky and jin as code owners January 16, 2019 12:11
@iirina iirina requested a review from lberki January 16, 2019 12:14
@@ -17,3 +17,8 @@ third_party/java/java_tools/JacocoCoverage_jarjar_deploy.jar
third_party/java/java_tools/turbine_deploy.jar
third_party/java/java_tools/turbine_direct_binary_deploy.jar
third_party/java/java_tools/SingleJar_deploy.jar

The following tools were built with bazel 0.21.0 at commit 7f97ed4df69c2d9358a063cc7bb310c615ff6496 by running:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's nicer to build all the tools at the same commit, but it's OK to do that once all of them are moved to checked-in deploy jars.

jars = ["//tools/jdk:ImportDepsChecker_deploy.jar"]
)

java_binary(
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, this is used only in Android code, but it looks like a useful Java tool, so move on.

Copy link
Member

@jin jin left a comment

Choose a reason for hiding this comment

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

android/ changes lgtm

@jin jin added the team-Rules-Java Issues for Java rules label Jan 17, 2019
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

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

Still looks good

@iirina
Copy link
Contributor Author

iirina commented Jan 21, 2019

I still have second thoughts whether to push this or not, since it's only used in the Android rules... I don't think we should mix the tools (it could be problematic when we decouple Android from Bazel as well).

@lberki
Copy link
Contributor

lberki commented Jan 21, 2019

Well, what does it do? According to a Javadoc, "Checker that checks the classes in the input jars have complete dependencies. If not, output the missing dependencies to a file" and it processes jdeps files and it does not contain any logic that would be specific to Android, so it makes sense to put in in tools specific to Java...

@meisterT
Copy link
Member

Irina, did you discuss with Jin where it should belong?

@iirina
Copy link
Contributor Author

iirina commented Mar 18, 2019

It should belong to the android tools. I'll leave it up to Jin to come up with a plan to build and embed it in the remote android tools.

@jin
Copy link
Member

jin commented Mar 29, 2019

I'll look into this alongside #1055.

@iirina iirina closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Java Issues for Java rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants