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

Remove ijar from the bazel binary. #7665

Closed
wants to merge 1 commit into from

Conversation

iirina
Copy link
Contributor

@iirina iirina commented Mar 7, 2019

Bazel now doesn't embed ijar anymore and uses ijar embeded in the Java tools remote repository.

Bazel embedded both ijar's source code (required by remote execution) and platform specific pre-built binaries. This change adds platform specific remote Java tools repository that include the corresponding pre-built ijar binary (for Windows, Darwin and Linux), alongside ijar's source code.

This change introduces a collection of macros that wrap the native rules filegroup and java_import and select a given target from the external java tools repository based on the current platform.

Progress on #6316.

@iirina
Copy link
Contributor Author

iirina commented Mar 7, 2019

@lberki ijar is also used by the android rules. I wouldn't go forward with removing ijar from the bazel binary until there is a clear story for the Android tools.

The difference in binary size (with and without ijar) is ~ 0.1M so there is not much gained.

@iirina
Copy link
Contributor Author

iirina commented Mar 7, 2019

cc @jin @ahumesky

src/BUILD Outdated Show resolved Hide resolved
src/BUILD Outdated Show resolved Hide resolved
tools/jdk/BUILD Outdated Show resolved Hide resolved
@jin
Copy link
Member

jin commented Mar 7, 2019

Like the other Java tools, this looks like a transparent change to the Android rules, since ijar will now be downloaded at build time instead. Is that accurate?

@dslomov dslomov added the team-Rules-Java Issues for Java rules label Mar 8, 2019
@iirina iirina requested review from ahumesky and jin as code owners March 8, 2019 14:46
tools/android/BUILD.tools Outdated Show resolved Hide resolved
@iirina iirina changed the title Remove ijar from the bazel binary. WIP Remove ijar from the bazel binary. Mar 11, 2019
@iirina iirina removed the request for review from ahumesky March 11, 2019 08:50
@iirina iirina force-pushed the remove-ijar-from-srcs branch 3 times, most recently from c1ba3bc to da33f09 Compare March 14, 2019 08:23
@iirina iirina changed the title WIP Remove ijar from the bazel binary. Remove ijar from the bazel binary. Mar 14, 2019
@iirina
Copy link
Contributor Author

iirina commented Mar 14, 2019

@lberki The PR is now ready for review 🎆 I added more details in the first PR comment.

@iirina
Copy link
Contributor Author

iirina commented Mar 14, 2019

@jin yes, that's accurate. I was worried because there are some genrules in the android tools that use ijar directly (ijar_desugared_java8_legacy_libs for example) and I'm not at all familiar with that part of the code. I thought it was better to ask first if this change is a problem for the android team.

@iirina iirina requested a review from lberki March 14, 2019 08:58
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.

Your review, ma'am!

third_party/java/java_tools/BUILD-windows.pkg Outdated Show resolved Hide resolved
third_party/java/java_tools/BUILD-windows.pkg Outdated Show resolved Hide resolved
def _get_args(target, attr, **kwargs):
workspace_target_dict = {
"//src/conditions:linux_x86_64": ["@remote_java_tools_linux//" + target],
"//src/conditions:linux_aarch64": ["@remote_java_tools_linux//" + target],
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a separate ijar build for Aarch64, do we? But if not, how does this work? An ijar binary built for x86_64 surely won't run there...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have a separate ijar build for aarch64. I assumed it might work with the same binary, but if it doesn't I am open to suggestions...

Previously when ijar was embedded into bazel, it used either the cc_binary target (in remote execution) or the prebuilt embedded binary for all other cases... If someone was using the linux bazel release on linux aarch64 then ijar probably didn't work for them... In this case I assume they compile bazel themselves? If so, they have to do the same with the Java tools... which makes things more complicated at this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed: build a source archive and direct //conditions:default to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I realized we can re-use any of the already existing archives (I choose the linux one). The BUILD file in the remote repository returns the prebuilt ijar binary for the 3 known platforms and the cc_binary target for all the others (+ remote execution). All the other targets are deploy jars and the platform doesn't matter in their case.

tools/jdk/remote_java_tools_aliases.bzl Outdated Show resolved Hide resolved
src/BUILD Outdated Show resolved Hide resolved
src/create_embedded_tools.py Outdated Show resolved Hide resolved
@iirina iirina requested a review from lberki March 14, 2019 13:31
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.

Impressive! 👍

@iirina
Copy link
Contributor Author

iirina commented Mar 15, 2019

@lberki Thanks! I moved the BUILD files embedded into the bazel binary from third_party to tools/jdk to avoid doing bazel changes and third_party changes. Otherwise this PR would have been difficult to import. 😞

@lberki
Copy link
Contributor

lberki commented Mar 15, 2019

...and that also :)

@bazel-io bazel-io closed this in d4651e8 Mar 15, 2019
@iirina iirina reopened this Mar 15, 2019
@iirina iirina force-pushed the remove-ijar-from-srcs branch from 3cff3fa to 75f1744 Compare March 15, 2019 13:19
@iirina
Copy link
Contributor Author

iirina commented Mar 15, 2019

third_party changes not merged yet.

@iirina iirina force-pushed the remove-ijar-from-srcs branch from 75f1744 to 8d02e74 Compare March 15, 2019 16:28
bazel-io pushed a commit that referenced this pull request Mar 15, 2019
Partial commit for third_party/*, see #7665.

Signed-off-by: iirina <elenairina@google.com>
@iirina
Copy link
Contributor Author

iirina commented Mar 15, 2019

third_party changes pushed in 9554a03.

@iirina iirina closed this Mar 15, 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