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

Incompatibilities with openjdk 17 #136

Closed
1 task done
jaimergp opened this issue Aug 9, 2022 · 10 comments
Closed
1 task done

Incompatibilities with openjdk 17 #136

jaimergp opened this issue Aug 9, 2022 · 10 comments
Labels

Comments

@jaimergp
Copy link
Member

jaimergp commented Aug 9, 2022

Solution to issue cannot be found in the documentation.

  • I checked the documentation.

Issue

Some projects rely on old versions of Bazel on purpose (e.g. tensorstore), as specified in their .bazelversion file.

conda-forge has access to openjdk 17 since last week or so, but this openjdk is not compatible with Bazel 5.0 and 5.1 (ref). The solution for those is to pin openjdk to the previous release (11) (source comment).

v5.2 is not affected (ref).

@xhochy - do you think we should issue a repodata patch or is this report enough? Only jaxlib relies on 5.1.1, but they have an upper bound, so they are getting 5.2, I guess.

Installed packages

N/A

Environment info

N/A
@jaimergp jaimergp added the bug label Aug 9, 2022
@h-vetinari
Copy link
Member

v5.2 is not affected

Ran into build errors in #138 (targetting main), which had very java-flavoured stacktrace and went away when I restricted to <17, so I'm pretty sure 5.2 is affected.

@ngam
Copy link

ngam commented Aug 17, 2022

In my experience, this is an error in how the toolchain gets the extra commands, but we are fine.

Instead of bazel build --logging=6 --subcommands --verbose_failures //main:hello-world, do:

cat >> .bazelrc <<EOF
build --crosstool_top=//bazel_toolchain:toolchain
build --logging=6
build --subcommands
build --verbose_failures
EOF

bazel build //main:hello-world

@ngam
Copy link

ngam commented Aug 17, 2022

@ngam
Copy link

ngam commented Aug 17, 2022

Ooops, sorry I misread the upper bound. Well, here are my 2c: I kept facing this error in jaxlib (even with 5.2.0) until we moved to .bazelrc

@ngam
Copy link

ngam commented Aug 17, 2022

I followed the lead of a jax maintainer here conda-forge/jaxlib-feedstock#126 so I copied that to tensorflow-feedstock too!

Change CUSTOM_BAZEL_OPTIONS to append to .bazelrc rather than pass
--bazel_options. This is both more readable and allows us to limit the
applicability of these flags only to the "build" phase. In my local
builds, I saw build errors from "bazel shutdown" because it was being
passed flags only applicable to "build".

@ngam
Copy link

ngam commented Aug 20, 2022

v5.2 is not affected

Ran into build errors in #138 (targetting main), which had very java-flavoured stacktrace and went away when I restricted to <17, so I'm pretty sure 5.2 is affected.

This will need restructuring build.sh so that the bootstrapping is done with a bazelrc file instead of passing flags

@xhochy
Copy link
Member

xhochy commented Aug 23, 2022

We don't know about all consumer of the bazel package as people could also use it locally (outside of a feedstock). Thus I would propose to patch all bazel versions prior a date and <5.2 to openjdk<17.

@ngam
Copy link

ngam commented Aug 23, 2022

We don't know about all consumer of the bazel package as people could also use it locally (outside of a feedstock). Thus I would propose to patch all bazel versions prior a date and <5.2 to openjdk<17.

I suspect the issue isn't with version incompatibility per se, and patching is likely only going to work temporarily. The larger issue seems to be in passing the flags. That's what I faced when I was (re)building jaxlib and all these issues disappeared when the flags were passed via bazelrc rather than directly. These flags are being passed indiscriminately to all stages. I got inspired by this comment conda-forge/jaxlib-feedstock#126 (comment) to form this conclusion, but I am quite uncertain if it makes full sense...

@h-vetinari
Copy link
Member

I opened a PR to lift this cap again (now that openjdk 17 is getting way more widespread): #194

@h-vetinari
Copy link
Member

This has been "fixed" by #236 😄

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

No branches or pull requests

4 participants