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

Use Turbine native image as header_compiler_direct #151

Closed
wants to merge 2 commits into from

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Nov 2, 2023

No description provided.

@fmeum fmeum marked this pull request as ready for review November 16, 2023 09:39
@fmeum fmeum requested review from comius and a team as code owners November 16, 2023 09:39
@fmeum
Copy link
Contributor Author

fmeum commented Nov 16, 2023

@hvadehra @cushon This is ready for review.

@comius comius requested review from hvadehra and removed request for comius November 16, 2023 12:00
@fmeum fmeum deleted the native-turbine branch November 16, 2023 16:13
@fmeum
Copy link
Contributor Author

fmeum commented Nov 16, 2023

@keertk Sorry for all the hoops this requires, but could you create another rules_java release with this change?

copybara-service bot pushed a commit that referenced this pull request Nov 16, 2023
Copybara Import from #154

BEGIN_PUBLIC
Update rules_java to 7.3.0 (#154)

#151 (comment)

Closes #154
END_PUBLIC

COPYBARA_INTEGRATE_REVIEW=#154 from bazelbuild:rules_java-7.3 ddcebff
PiperOrigin-RevId: 583054998
Change-Id: I7078c14eada52276fa6e1fc7c54963a7c1c88c56
@keertk
Copy link
Member

keertk commented Nov 16, 2023

@BoleynSu
Copy link

BoleynSu commented Dec 2, 2023

@fmeum this breaks rules_jvm_external with crashing here https://github.com/google/turbine/blob/33ed406f653ecfda611933103261a6ad3146dbd5/java/com/google/turbine/binder/CtSymClassBinder.java#L52. Could you take a look?

# a minimal repro
java_library(
    name = "l",
    srcs = ["t.java"],
    javacopts = [
        "--release",
        "8",
    ],
)

java_binary(
    name = "b",
    srcs = ["t.java"],
    main_class = "t.t",
    deps = [":l"],
)

@cushon
Copy link
Collaborator

cushon commented Dec 2, 2023

@BoleynSu see bazelbuild/stardoc#195

@fmeum
Copy link
Contributor Author

fmeum commented Dec 2, 2023

@BoleynSu rules_java 7.3.1 is out with the fix. Please give it a try.

@BoleynSu
Copy link

BoleynSu commented Dec 2, 2023 via email

@fmeum
Copy link
Contributor Author

fmeum commented Dec 2, 2023

@BoleynSu That does sound like a problem with updating rules_java. How are you doing that?

With the following setup:

# BUILD
java_library(
    name = "l",
    srcs = ["t.java"],
    javacopts = [
        "--release",
        "8",
    ],
)

java_binary(
    name = "b",
    srcs = ["t.java"],
    main_class = "t.t",
    deps = [":l"],
)

# t.java
package t;

public class t {}

# MODULE.bazel
bazel_dep(name = "rules_java", version = "7.3.0")

I get:

$ USE_BAZEL_VERSION=7.0.0rc5 bazel build //:b
...
Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
java.lang.NullPointerException: attempted to use --release, but JAVA_HOME is not set
	at java.base@20.0.2/java.util.Objects.requireNonNull(Objects.java:259)
	at com.google.turbine.binder.CtSymClassBinder.bind(CtSymClassBinder.java:52)
	at com.google.turbine.main.Main.bootclasspath(Main.java:309)
	at com.google.turbine.main.Main.compile(Main.java:142)
	at com.google.turbine.main.Main.compile(Main.java:133)
	at com.google.turbine.main.Main.main(Main.java:89)
Target //:b failed to build

When I update the rules_java version to 7.3.1, the build passes.

@BoleynSu
Copy link

BoleynSu commented Dec 2, 2023 via email

@fmeum
Copy link
Contributor Author

fmeum commented Dec 2, 2023

Yes, Bazel 6.4.0 doesn't contain bazelbuild/bazel@4a29f08, which is needed to make the Java rules pass ct.sym to Turbine. I would recommend staying on rules_java 7.2.0 with Bazel 6.4.0.

@BoleynSu
Copy link

BoleynSu commented Dec 2, 2023

I see. I am skipping 7.3.x for now. BoleynSu-Org/monorepo@770b1fc
Hopefully, Bazel 7 can be used soon.

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.

None yet

5 participants