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

Embed a default java_toolchain macro into java_tools. #8613

Closed
wants to merge 6 commits into from

Conversation

iirina
Copy link
Contributor

@iirina iirina commented Jun 12, 2019

There are several benefits of using a macro:

  • re-using the same default values for the 2 toolchains embedded into a java_tools repository (e.g. @java_tools//:toolchain and @java_tools//:toolchain_jdkX)
  • allowing users to use the embedded macro and only override parts of it (most common case is overriding soruce_version and target_version).

@bazel_tools//tools/jdk:default_java_toolchain.bzl will be deprecated and removed in favor of @java_tools//:java_tools/java_tools_defs.

This change also adds test cases for building targets in the java_tools repository. Note that ijar and singlejar don't yet build on Windows (see #8614).

@iirina iirina requested a review from hlopko June 13, 2019 07:45
@iirina iirina added the team-Rules-Java Issues for Java rules label Jun 13, 2019
@iirina iirina force-pushed the embed-default-java-toolchain branch from 24fbe05 to 7a3448b Compare June 13, 2019 08:21
fi
cat >WORKSPACE <<EOF
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.

Will local_repository work instead of http_archive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

local_repository only works if we unzip the archive first, which means additional setup and complexity in the tests. My intention was also to test the archive in the same way it is used by Bazel and Java users.


# override the javac in the JDK.
"--patch-module=java.compiler=$(location :java_compiler_jar)",
"--patch-module=jdk.compiler=$(location :jdk_compiler_jar)",
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually confused here, so I'll ask :) Is it true that this relative label keeps referencing @bazel_tools//tools/jdk:java_compiler_jar even when we call the macro from a random package //foo/bar? Or it assumed that the //foo/bar package has those labels around?

"jacocorunner": ":jacoco_coverage_runner_filegroup"
}

def default_java_toolchain(name, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Alternative is to make all of these default values of attributes on java_toolchain, effectively making java_toolchain itself serve the purpose of the macro. Is that viable?

@irengrig irengrig removed WIP labels Jun 19, 2019
@jin
Copy link
Member

jin commented Apr 28, 2020

Closing this for now, please ping/reopen if you're intending to work on this.

@jin jin closed this Apr 28, 2020
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