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

Windows: fix ijar compilation from ext.repo #8742

Closed
wants to merge 3 commits into from

Conversation

laszlocsomor
Copy link
Contributor

//src/main/native/windows:lib-file and
//src/main/native/windows:lib-util were merged
into one rule some time ago.

This commit merges the copies of those libraries
in tools/jdk/BUILD accordingly.

Fixes #8614

//src/main/native/windows:lib-file and
//src/main/native/windows:lib-util were merged
into one rule some time ago.

This commit merges the copies of those libraries
in tools/jdk/BUILD accordingly.

Fixes bazelbuild#8614
Copy link
Contributor

@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.

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"
}

They should test it fixes #8614.

@laszlocsomor
Copy link
Contributor Author

Done.

Copy link
Contributor

@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.

Thank you so much for fixing this!!

@laszlocsomor
Copy link
Contributor Author

The test fails because token_stream in tools/jdk/BUILD.java_tools does not depend on .../util/path_platform.h. If I add java_tools/src/main/cpp/util/path_platform.h to the rule's hdrs, I get a header check error because the same file is present under external/bazel_tools but that's not a declared rule input:

ERROR: C:/_bazel/okv4kk6b/external/local_java_tools/BUILD:552:1: undeclared inclusion(s) in rule '@local_java_tools//:singlejar_cc_bin':
this rule is missing dependency declarations for the following files included by 'external/local_java_tools/java_tools/src/tools/singlejar/singlejar_main.cc':
  'external/bazel_tools/src/main/cpp/util/path_platform.h'
Target @local_java_tools//:singlejar_cc_bin failed to build

Unfortunately on Windows we must expect this error because every action runs in the one and only execroot and they all share it, so all inputs are visible to every action. (There's no sandboxing yet.)

Change-Id: Idd10b6c86b70c40b623a9373ba29b1a428e0aa12
@laszlocsomor
Copy link
Contributor Author

My latest change didn't fix the test on Windows.

The problem is that @local_java_tools//:ijar_cc_binary depends on @local_java_tools//ijar/zip.h (with "include_prefix" and "strip_include_prefix" the include path being "third_party/ijar/zip.h"), but the rule also depends on some other rules in @bazel_tools. Because of the dependencies, both external/bazel_tools and external/local_java_tools are include directories. Unfortunately external/bazel_tools/third_party/ijar/zip.h also exists, and the compiler picks that file before encountering external/local_java_tools/java_tools/ijar/zip.h, so header checking fails.

At this point I'm giving up and abandoning this PR, because I don't know how to fix this without sandboxing.

@meteorcloudy
Copy link
Member

@iirina After #8950, building ijar and singlejar fail on Windows due to header check mentioned here by @laszlocsomor .

but the rule also depends on some other rules in @bazel_tools

This is actually not true, the rule is not depending on anything under @bazel_tools, but /Iexternal/bazel_tools still got added. @oquenchil Do you know why we add /Iexternal/bazel_tools for every compile action unconditionally?

bazel-io pushed a commit that referenced this pull request Jul 23, 2019
…emoving dependency on @bazel_tools//tools/cpp:malloc

As @laszlocsomor [explained](#8742 (comment)
), building `ijar_cc_binary` and `singlejar_cc_bin` on Windows (where sandbox is not available yet) will have header conflict due to the same headers are shipped in both @bazel_tools and @local_java_tools.

`ijar_cc_binary` and `singlejar_cc_bin` only declare headers in @local_java_tools as dependency, but it will search `external/bazel_tools` first because every cc targets depend on `@bazel_tools//tools/cpp:malloc` [implicitly](https://docs.bazel.build/versions/master/be/c-cpp.html#cc_binary).

This change remove the dependency on `@bazel_tools//tools/cpp:malloc` to avoid this confusion.

Closes #8954.

PiperOrigin-RevId: 259519714
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java_tools' ijar and singlejar don't build on windows
4 participants