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

(WIP) Bzlmod: export DEFAULT_SYSTEM_JAVABASE to MODULE.bazel #14253

Closed
wants to merge 2 commits into from

Conversation

meteorcloudy
Copy link
Member

DEFAULT_SYSTEM_JAVABASE is an default variable in WORKSPACE file, its value is controlled by --default_system_javabase flag, we need this information to locate the local JDK path in rules_java.

Required by bazelbuild/rules_java#45 (comment)

@Wyverald
Copy link
Member

cc @comius

I think DEFAULT_SYSTEM_JAVABASE as a symbol in WORKSPACE was an ugly hack, and I would really like to avoid having it in MODULE.bazel too. [Off-topic: In fact it feels weird that we can specify --default_system_javabase on the command line, but cannot specify, for example, --default_system_pythonbase or something (i.e. why is Java special?).]

Anyhow, I think my preferred solution would be to make it available under ctx.os.default_system_javabase, or something to that effect. That way, module extensions and repo rules get access to it, but not MODULE.bazel files. WDYT?

@meteorcloudy meteorcloudy changed the title Bzlmod: export DEFAULT_SYSTEM_JAVABASE to MODULE.bazel (WIP) Bzlmod: export DEFAULT_SYSTEM_JAVABASE to MODULE.bazel Nov 10, 2021
@comius
Copy link
Contributor

comius commented Nov 10, 2021

Hey, I wish you asked about this earlier. I think there are no obstacles to completely remove DEFAULT_SYSTEM_JAVABASE logic and its passing down to the workspace.

Proper implementation should leverage repository api in the local_java_repository repo rule. I believe the logic that happens in C++ can be completely reproduced in local_java_repository's implementation. It would perhaps be even better, to simplify it and only resolve to JAVA_HOME environment variable (this is what's advertised in the documentation, but we actually also look in PATH).

This was one thing I wanted to do, but there was no proper motivation to do it, that is "tech debt", but not much impact when removing it. Now with bzlmod, it has much bigger impact to remove it, because you can keep the codebase simple.

@meteorcloudy
Copy link
Member Author

OK, looks like the java base is detected in the Bazel client, and each platform has its own implementation:
https://cs.opensource.google/search?q=GetSystemJavabase&sq=&ss=bazel

@meteorcloudy
Copy link
Member Author

meteorcloudy commented Nov 10, 2021

to simplify it and only resolve to JAVA_HOME environment variable

Can we require user to always set JAVA_HOME manually? That's a breaking change, right?

I think I can try to replicate the java base detection logic in module extension.

@comius
Copy link
Contributor

comius commented Nov 10, 2021

Can we require user to always set JAVA_HOME manually? That's a breaking change, right?

You're right, Java compilations on systems without JAVA_HOME set would start failing. JAVA_HOME or PATH it is.

@meteorcloudy
Copy link
Member Author

Closing this one as we prefer to get rid of DEFAULT_SYSTEM_JAVABASE

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.

3 participants