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

[bazel][java] Use a transition to target Java 11 for our maven artifacts #14830

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented Nov 29, 2024

User description

This allows us to use Java 21 elsewhere in the repo where it might be useful (Tests! Tests! Tests!) without needing to monkey around with special flags.


PR Type

enhancement, configuration changes


Description

  • Implemented a transition to target Java 11 for Maven artifacts using with_cfg in export.bzl.
  • Updated the Java toolchain in .bazelrc to use version 21, allowing flexibility to use Java 21 elsewhere in the repository.
  • Added a new dependency on with_cfg.bzl in MODULE.bazel to support the configuration transition.

Changes walkthrough 📝

Relevant files
Enhancement
export.bzl
Implement transition to Java 11 for Maven artifacts           

java/private/export.bzl

  • Added a transition to target Java 11 using with_cfg.
  • Modified java_module to use the new configuration.
  • +6/-1     
    Configuration changes
    .bazelrc
    Update Java toolchain to version 21                                           

    .bazelrc

  • Updated Java toolchain versions to 21.
  • Removed default targeting of Java 11.
  • Added comments explaining the transition to Java 11 for binaries.
  • +7/-8     
    Dependencies
    MODULE.bazel
    Add dependency for configuration transition                           

    MODULE.bazel

    • Added a dependency on with_cfg.bzl version 0.6.0.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    …acts
    
    This allows us to use Java 21 elsewhere in the repo where it might be
    useful (Tests! Tests! Tests!) without needing to monkey around with
    special flags.
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Validation
    Verify that the Java 11 transition configuration is properly applied to all maven artifacts and doesn't have unintended side effects on other build targets

    Version Compatibility
    Ensure that using Java 21 for build tooling while targeting Java 11 for artifacts doesn't create any compatibility issues or unexpected behavior

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation checks for configuration operations to prevent silent failures in Java version targeting

    Add error handling to validate that the Java version configuration was successful,
    as failed configuration could lead to incorrect Java version targeting.

    java/private/export.bzl [12-14]

     _builder = with_cfg(_java_module)
    -_builder.set("java_language_version", "11").set("java_runtime_version", "remotejdk_11")
    -java_module, _ = _builder.build()
    +result = _builder.set("java_language_version", "11").set("java_runtime_version", "remotejdk_11")
    +if not result:
    +    fail("Failed to configure Java 11 version settings")
    +java_module, status = _builder.build()
    +if not status:
    +    fail("Failed to build Java module with version configuration")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important error handling to validate Java version configuration, which is critical since incorrect Java version targeting could lead to build and runtime issues. Silent failures in this configuration could be hard to debug.

    8

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 changed the title [bazel + java] Use a transition to target Java 11 for our maven artifacts [bazel][java] Use a transition to target Java 11 for our maven artifacts Dec 5, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants