-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Depend on @bazel_tools//third_party/ijar:zip from java_tools #8945
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Yun, this is great!
Could you please add the following two test cases to src/test/shell/bazel/bazel_java_tools_test.sh
function test_java_tools_singlejar_builds() {
local java_tools_rlocation=$(rlocation io_bazel/src/java_tools_${JAVA_TOOLS_JAVA_VERSION}.zip)
local java_tools_zip_file_url="file://${java_tools_rlocation}"
if "$is_windows"; then
java_tools_zip_file_url="file:///${java_tools_rlocation}"
fi
cat >WORKSPACE <<EOF
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
name = "local_java_tools",
urls = ["${java_tools_zip_file_url}"]
)
EOF
bazel build @local_java_tools//:singlejar_cc_bin || fail "singlejar failed to build"
}
function test_java_tools_ijar_builds() {
local java_tools_rlocation=$(rlocation io_bazel/src/java_tools_${JAVA_TOOLS_JAVA_VERSION}.zip)
local java_tools_zip_file_url="file://${java_tools_rlocation}"
if "$is_windows"; then
java_tools_zip_file_url="file:///${java_tools_rlocation}"
fi
cat >WORKSPACE <<EOF
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
name = "local_java_tools",
urls = ["${java_tools_zip_file_url}"]
)
EOF
bazel build @local_java_tools//:ijar_cc_binary || fail "ijar failed to build"
}
@@ -30,6 +30,7 @@ cc_library( | |||
"//src:__subpackages__", | |||
"//third_party/ijar:__subpackages__", | |||
"//tools/test:__pkg__", | |||
"@local_java_tools//:__pkg__", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is enough. It's enough for the tests to pass. The remote repo names in the workspace suffix are "remote_java_tools_linux", "remote_java_tools_windows" and "remote_java_tools_darwin". Can you please add them as well? Also add a TODO to migrate this target out of bazel_tools.
@iirina Thanks! I added tests and extra targets. |
I'm not quite sure the test will pass because of bazel/tools/jdk/BUILD.java_tools Lines 5 to 6 in 8ef1799
We didn't declare rules_cc and rules_java anywhere in the WORKSPACE file. |
Thanks @meteorcloudy! bazel/src/main/java/com/google/devtools/build/lib/bazel/rules/java/jdk.WORKSPACE Lines 242 to 263 in 8ef1799
|
Building |
Thanks @meteorcloudy. This is something new, the tools were building before... Let me see what changed :( |
OK, thanks for checking! |
@meteorcloudy f5a6ee1 fixed the tests on the other platforms. Can you rebase and remove the lines that skip the tests on Windows? bazel/src/test/shell/bazel/bazel_java_tools_test.sh Lines 193 to 195 in f5a6ee1
and bazel/src/test/shell/bazel/bazel_java_tools_test.sh Lines 212 to 214 in f5a6ee1
|
Found a better fix at #8954, please take a look ;) |
Closing due to #8954 |
Fixes #8614