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

java_tools' ijar and singlejar don't build on windows #8614

Closed
iirina opened this issue Jun 12, 2019 · 16 comments

Comments

@iirina
Copy link
Contributor

commented Jun 12, 2019

The ijar embedded in java_tools doesn't build from source on Windows.

Repro:

Build one of the java_tools zips in the bazel repo:

bazel build //src:java_tools_java10_zip
export JAVA_TOOLS_ZIP=~/java_tools.zip
cp bazel-bin/src/java_tools_java10.zip $JAVA_TOOLS_ZIP
mkdir java_tools_repro && cd java_tools_repro
cat >WORKSPACE <<EOF
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
http_archive(
    name = "local_java_tools",
    urls = ["$JAVA_TOOLS_ZIP"]
)
EOF
bazel build @local_java_tools//:lib-util

The log includes:

ERROR: D:/b/g4je5q24/execroot/io_bazel/_tmp/33823c8031b1529f3520a7c37487665f/root/ayzg7dvk/external/local_java_tools/BUILD:376:1: C++ compilation of rule '@local_java_tools//:lib-util' failed (Exit 2)
external/local_java_tools/java_tools/src/main/native/windows/util.cc(30): fatal error C1083: Cannot open include file: 'src/main/native/windows/file.h': No such file or directory
Target @local_java_tools//:ijar_cc_binary failed to build

The failing target is a cc_library which uses strip_include_prefix (from a naive point of view this looks like the culprit).

The BUILD file where the targets are defined is tools/jdk/BUILD.java_tools.

Example of log on Buildkite: https://storage.googleapis.com/bazel-untrusted-buildkite-artifacts/fdd75d7e-09da-44dc-8e2b-24c907722980/src%5Ctest%5Cshell%5Cbazel%5Cbazel_java_tools_javac10_test%5Cattempt_1.log

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

Not P1 because on Windows the java_tools use the prebuilt ijar binary.

@iirina iirina changed the title java_tools' ijar doesn't build on windows java_tools' ijar and singlejar don't build on windows Jun 13, 2019

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

cc: @laszlocsomor for windows expertise, @hlopko for C++

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

I also checked that the file java_tools/src/main/native/windows/file.h exists in the zip archive in #8621.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

How can I help?

@laszlocsomor laszlocsomor self-assigned this Jun 28, 2019

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Found the culprit.

"lib-file" and "lib-util" were merged a few weeks ago. Let me update tools/jdk/BUILD.java_tools.

laszlocsomor added a commit to laszlocsomor/bazel that referenced this issue Jun 28, 2019
Windows: fix ijar compilation from ext.repo
//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
@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Thank you @laszlocsomor for looking into this! Can you explain why merging the two targets fixes this issue? In theory they're the same files and it shouldn't matter if they're one or two targets. I'm probably missing something.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Because src/main/native/windows/util.cc in "lib-util" includes src/main/native/windows/file.h, but the rule doesn't depend on that file.

I merged the two rules (in their "main" location) to avoid a circular dependency: util.cc includes file.h and file.cc includes util.h

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

I see, it makes sense. Thanks for the explanation!

@laszlocsomor laszlocsomor removed their assignment Jun 28, 2019

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

I'm giving up, couldn't fix the errors in my PR. See #8742 (comment)

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2019

Thanks @laszlocsomor for looking into it! Now I understand what's causing the error. IIUC one fix would be to remove the ijar sources from @bazel_tools completely. I'll take a look to see if it's feasible.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

@iirina : Sounds right. Did you try that?

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

@laszlocsomor Yes. Some ijar sources are still under @bazel_tools because they overalp with zipper's sources, which is a tool used by the Python rules for example. Because of this we don't have a good solution right now. Moving this tool in a remote java tools repository adds a Python -> java tools dependency which is unfortunate.

@laszlocsomor

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

So python rules use zipper?

Do you know what dependencies of zipper we must cut?

For example, nothing of //src/main/native/windows except for the built windows-jni.dll should be in bazel_tools, I think.

@iirina

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

All native Python rules have an implicit $zipper dependency on @bazel_tools//tools/zip:zipper.

LE: //tools/zip:zipper is an alias for //third_party/ijar:zipper

Do you know what dependencies of zipper we must cut?

Do you mean which sources overlap with ijar's sources? I don't know, but I can take a look and put together a list. From a quick glance they both depend on targets under //src/main/cpp/util (:errors, :logging, strings, etc)

@meteorcloudy

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

I found out that if we depend on @bazel_tools//third_party/ijar:zip directly, ijar_cc_binary will build on Windows.
#8945

@meteorcloudy

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Fixed by #8954

bazel-io pushed a commit that referenced this issue Jul 24, 2019
Update to java_tools javac11 v4.0
RELNOTES: The new java_tools release:
* fixes #8614
* exposes a new toolchain `@java_tools//:prebuilt_toolchain` which is using all the pre-built tools, including singlejar and ijar, even on remote execution. This toolchain should be used only when host and execution platform are the same, otherwise the binaries will not work on the execution platform.

Closes #8960.

PiperOrigin-RevId: 259694959
bazel-io pushed a commit that referenced this issue Aug 28, 2019
Release 0.29.0 (2019-08-28)
Baseline: 6c5ef53

Cherry picks:

   + 338829f:
     Fix retrying of SocketTimeoutExceptions in HttpConnector
   + 14651cd:
     Fallback to next urls if download fails in HttpDownloader
   + b7d300c:
     Fix incorrect stdout/stderr in remote action cache. Fixes #9072
   + 9602176:
     Automated rollback of commit
     0f0a0d5.
   + da557f9:
     Windows: fix "bazel run" argument quoting
   + ef8b6f6:
     Return JavaInfo from java proto aspects.
   + 209175f:
     Revert back to the old behavior of not creating a proto source
     root for generated .proto files.
   + 644060b:
     Fix PatchUtil for parsing special patch format
   + 067040d:
     Put the removal of the legacy repository-relative proto path
     behind the --incompatible_generated_protos_in_virtual_imports
     flag.
   + 76ed014:
     repository mapping lookup: convert to canonical name first

Important changes:

  - rule_test: fix Bazel 0.27 regression ("tags" attribute was
    ingored, #8723
  - Adds --incompatible_enable_execution_transition, which enables
    incremental migration of host attributes to exec attributes.
  - objc_proto_library rule has been deleted from Bazel.
  - repository_ctx.read is no longer restricted to files
        in the repository contructed.
  - tags 'no-remote', 'no-cache', 'no-remote-cache',
    'no-remote-exec', 'no-sandbox' are propagated now to the actions
    from targets when '--ncompatible_allow_tags_propagation' flag set
    to true. See #8830.
  - Adds flag
    --//tools/build_defs/pkg:incompatible_no_build_defs_pkg. This
    flag turns off the rules //tools/build_defs/pkg:{pkg_deb,
    pkg_rpm, pkg_tar}.
  - The Android NDK is now integrated with toolchains. To use them,
    pass the `--extra_toolchains=@androidndk//:all` flag or register
    them in your WORKSPACE with
    `register_toolchains("@androidndk//:all")`.
  - Stdout and stderr are checked to determine if output is going to a
    terminal. `--is_stderr_atty` is deprecated and `--isatty` is
    undeprecated.
  - --incompatible_load_proto_rules_from_bzl was added to forbid
    loading the native proto rules directly. See more on tracking
    issue #8922
  - Docker Sandbox now respects remote_default_platform_properties
  - pkg_deb, pkg_rpm & pkg_tar deprecation plan announced in the
    documentation.
  - The new java_tools release:
    * fixes #8614
    * exposes a new toolchain `@java_tools//:prebuilt_toolchain`
    which is using all the pre-built tools, including singlejar and
    ijar, even on remote execution. This toolchain should be used
    only when host and execution platform are the same, otherwise the
    binaries will not work on the execution platform.
  - java_common.compile supports specifying
    annotation_processor_additional_inputs and
    annotation_processor_additional_outputs for the Java compilation
    action for supporting annotation processors that consume or
    produce artifacts. Fixes #6415
  - There is now documentation on optimizing Android app build
    performance. Read it at
    https://docs.bazel.build/versions/0.29.0/android-build-performance
    .html
  - Execution log now respects --remote_default_platform_properties
  - Include a link to the relevant documenation on transitive Python
    version errors.
  - New incompatible flag
    --incompatible_disable_target_provider_fields removes the ability
    (in Starlark) to access a target's providers via the field syntax
    (for example, `ctx.attr.dep.my_provider`). The provider-key
    syntax should be used instead (for example,
    `ctx.attr.dep[MyProvider]`). See
    #9014 for details.
  - A new platform exec_properties is added to replace
    remote_execution_properties.
  - Added --incompatible_load_python_rules_from_bzl, which will be
    flipped in Bazel 1.0. See
    #9006.
  - add --break_build_on_parallel_dex2oat_failure to shortcut tests
    on dex2oat errors

This release contains contributions from many people at Google, as well as Alexander Ilyin, Arek Sredzki, Artem Zinnatullin, Benjamin Peterson, Fan Wu, John Millikin, Loo Rong Jie, Marwan Tammam, Oscar Bonilla, Peter Mounce, Sergio Rodriguez Orellana, Takeo Sawada, and Yannic Bonenberger.
buchgr pushed a commit to buchgr/bazel that referenced this issue Aug 30, 2019
Release 0.29.0 (2019-08-28)
Baseline: 6c5ef53

Cherry picks:

   + 338829f:
     Fix retrying of SocketTimeoutExceptions in HttpConnector
   + 14651cd:
     Fallback to next urls if download fails in HttpDownloader
   + b7d300c:
     Fix incorrect stdout/stderr in remote action cache. Fixes bazelbuild#9072
   + 9602176:
     Automated rollback of commit
     0f0a0d5.
   + da557f9:
     Windows: fix "bazel run" argument quoting
   + ef8b6f6:
     Return JavaInfo from java proto aspects.
   + 209175f:
     Revert back to the old behavior of not creating a proto source
     root for generated .proto files.
   + 644060b:
     Fix PatchUtil for parsing special patch format
   + 067040d:
     Put the removal of the legacy repository-relative proto path
     behind the --incompatible_generated_protos_in_virtual_imports
     flag.
   + 76ed014:
     repository mapping lookup: convert to canonical name first

Important changes:

  - rule_test: fix Bazel 0.27 regression ("tags" attribute was
    ingored, bazelbuild#8723
  - Adds --incompatible_enable_execution_transition, which enables
    incremental migration of host attributes to exec attributes.
  - objc_proto_library rule has been deleted from Bazel.
  - repository_ctx.read is no longer restricted to files
        in the repository contructed.
  - tags 'no-remote', 'no-cache', 'no-remote-cache',
    'no-remote-exec', 'no-sandbox' are propagated now to the actions
    from targets when '--ncompatible_allow_tags_propagation' flag set
    to true. See bazelbuild#8830.
  - Adds flag
    --//tools/build_defs/pkg:incompatible_no_build_defs_pkg. This
    flag turns off the rules //tools/build_defs/pkg:{pkg_deb,
    pkg_rpm, pkg_tar}.
  - The Android NDK is now integrated with toolchains. To use them,
    pass the `--extra_toolchains=@androidndk//:all` flag or register
    them in your WORKSPACE with
    `register_toolchains("@androidndk//:all")`.
  - Stdout and stderr are checked to determine if output is going to a
    terminal. `--is_stderr_atty` is deprecated and `--isatty` is
    undeprecated.
  - --incompatible_load_proto_rules_from_bzl was added to forbid
    loading the native proto rules directly. See more on tracking
    issue bazelbuild#8922
  - Docker Sandbox now respects remote_default_platform_properties
  - pkg_deb, pkg_rpm & pkg_tar deprecation plan announced in the
    documentation.
  - The new java_tools release:
    * fixes bazelbuild#8614
    * exposes a new toolchain `@java_tools//:prebuilt_toolchain`
    which is using all the pre-built tools, including singlejar and
    ijar, even on remote execution. This toolchain should be used
    only when host and execution platform are the same, otherwise the
    binaries will not work on the execution platform.
  - java_common.compile supports specifying
    annotation_processor_additional_inputs and
    annotation_processor_additional_outputs for the Java compilation
    action for supporting annotation processors that consume or
    produce artifacts. Fixes bazelbuild#6415
  - There is now documentation on optimizing Android app build
    performance. Read it at
    https://docs.bazel.build/versions/0.29.0/android-build-performance
    .html
  - Execution log now respects --remote_default_platform_properties
  - Include a link to the relevant documenation on transitive Python
    version errors.
  - New incompatible flag
    --incompatible_disable_target_provider_fields removes the ability
    (in Starlark) to access a target's providers via the field syntax
    (for example, `ctx.attr.dep.my_provider`). The provider-key
    syntax should be used instead (for example,
    `ctx.attr.dep[MyProvider]`). See
    bazelbuild#9014 for details.
  - A new platform exec_properties is added to replace
    remote_execution_properties.
  - Added --incompatible_load_python_rules_from_bzl, which will be
    flipped in Bazel 1.0. See
    bazelbuild#9006.
  - add --break_build_on_parallel_dex2oat_failure to shortcut tests
    on dex2oat errors

This release contains contributions from many people at Google, as well as Alexander Ilyin, Arek Sredzki, Artem Zinnatullin, Benjamin Peterson, Fan Wu, John Millikin, Loo Rong Jie, Marwan Tammam, Oscar Bonilla, Peter Mounce, Sergio Rodriguez Orellana, Takeo Sawada, and Yannic Bonenberger.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.