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

Obtain bootstrap class path from dedicated runtime toolchain type #114

Closed
wants to merge 4 commits into from

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Jun 9, 2023

This decouples the concepts of a Java runtime used to run on the target platform from the Java runtime that provides the bootstrap class path used during compilation.

The former needs constraints on the target platform, whereas the latter should not have any constraints on the target platform to allow for cross-compilation to target platforms for which a JDK runtime is not available (e.g. Android).

Work towards bazelbuild/bazel#17085
Work towards #64
Split off from bazelbuild/bazel#18262

@fmeum
Copy link
Contributor Author

fmeum commented Jul 5, 2023

@hvadehra Could you review this? It is based on bazelbuild/bazel#18841.

@hvadehra hvadehra requested a review from cushon July 5, 2023 13:32
hvadehra
hvadehra previously approved these changes Jul 5, 2023
Copy link
Member

@hvadehra hvadehra left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like for @cushon to also have a look

cushon
cushon previously approved these changes Jul 5, 2023
@fmeum fmeum dismissed stale reviews from cushon and hvadehra via 7f94f77 July 5, 2023 16:28
@fmeum
Copy link
Contributor Author

fmeum commented Jul 5, 2023

@cushon @hvadehra I noticed that I forgot to register the toolchains in MODULE.bazel and added a new commit to do this.

hvadehra
hvadehra previously approved these changes Jul 5, 2023
@fmeum
Copy link
Contributor Author

fmeum commented Jul 31, 2023

@hvadehra @cushon Friendly ping.

I pushed a commit to fix CI after the merge of bazelbuild/bazel@8715e9a. Let me know whether you would want me to handle this differently.

cushon
cushon previously approved these changes Jul 31, 2023
fmeum added a commit to fmeum/rules_java that referenced this pull request Jul 31, 2023
hvadehra
hvadehra previously approved these changes Aug 1, 2023
@fmeum fmeum dismissed stale reviews from hvadehra and cushon via f26cfa6 August 1, 2023 08:48
@fmeum fmeum force-pushed the 17085-bootstrap-toolchain branch from f26cfa6 to 6b04a31 Compare August 1, 2023 08:51
@fmeum
Copy link
Contributor Author

fmeum commented Aug 1, 2023

Had to push another commit to fix a breakage that CI didn't catch: I forgot to add the register_toolchains calls for the bootstrap runtime toolchains to the WORKSPACE macro.

Do you see a way to have CI catch such issues, for example by including a pipeline that runs Bazel@HEAD with an --override_repository for @rules_java_builtin?

CC @meteorcloudy in case you have an idea

@meteorcloudy
Copy link
Member

I do feel the test coverage for rules_java is quite limited, I think we can:

I would suggest to look into rules_bazel_integration_test first to see if fits the need. ;)

This decouples the concepts of a Java runtime used to run on the target
platform from the Java runtime that provides the bootstrap class path
used during compilation.

The former needs constraints on the target platform, whereas the latter
should not have any constraints on the target platform to allow for
cross-compilation to target platforms for which a JDK runtime is not
available (e.g. Android).

Work towards #17085
Work towards bazelbuild#64
Split off from #18262
copybara-service bot pushed a commit that referenced this pull request Aug 7, 2023
BEGIN_PUBLIC
Copybara import of the project:

--
cb82ccb by Fabian Meumertzheim <fabian@meumertzhe.im>:

Obtain bootstrap class path from dedicated runtime toolchain type

This decouples the concepts of a Java runtime used to run on the target
platform from the Java runtime that provides the bootstrap class path
used during compilation.

The former needs constraints on the target platform, whereas the latter
should not have any constraints on the target platform to allow for
cross-compilation to target platforms for which a JDK runtime is not
available (e.g. Android).

Work towards #17085
Work towards #64
Split off from #18262

--
ab3c792 by Fabian Meumertzheim <fabian@meumertzhe.im>:

Register toolchains in MODULE.bazel

--
f2218a4 by Fabian Meumertzheim <fabian@meumertzhe.im>:

Update Bazel to 7.0.0-pre.20230710.5

--
0294790 by Fabian Meumertzheim <fabian@meumertzhe.im>:

Add missing WORKSPACE `register_toolchains` calls

END_PUBLIC

COPYBARA_INTEGRATE_REVIEW=#114 from fmeum:17085-bootstrap-toolchain 0294790
PiperOrigin-RevId: 554446834
Change-Id: I961944be5bfe461528e94c85247f6c36236a561d
copybara-service bot pushed a commit that referenced this pull request Aug 7, 2023
*** Reason for rollback ***

`bazel_compatibility` does not support pre-release versions

We can re-introduce this once bazelbuild/bazel#19189 is fixed

*** Original change description ***

Copybara Merge: #123

BEGIN_PUBLIC
Copybara import of the project:

--
3de49c8 by Fabian Meumertzheim <fabian@meumertzhe.im>:

Mark rules_java as compatible with Bazel >= 7.0.0-pre.20230710.5

Since #114, rules_java requires a version of Bazel that contains
bazelbuild/bazel@8715e9a

END_PUBLIC

***

PiperOrigin-RevId: 554463780
Change-Id: Ifdbaeb60bce59fdad3bfde8b343ed870d792eba7
@hvadehra
Copy link
Member

hvadehra commented Aug 8, 2023

@fmeum With ea0bd91 I guess this is done?

@fmeum
Copy link
Contributor Author

fmeum commented Aug 8, 2023

@hvadehra I think so, thanks!

@fmeum fmeum closed this Aug 8, 2023
@fmeum fmeum deleted the 17085-bootstrap-toolchain branch August 8, 2023 07:26
@fmeum
Copy link
Contributor Author

fmeum commented Aug 8, 2023

@hvadehra Could you create a release that I can then update Bazel to?

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

Successfully merging this pull request may close these issues.

4 participants