Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add support for add_exports/add_opens #1551

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

Open
timothyg-stripe wants to merge 1 commit into bazel-contrib:master
base: master
Choose a base branch
Loading
from timothyg-stripe:upstream-module-flags

Conversation

@timothyg-stripe
Copy link
Contributor

@timothyg-stripe timothyg-stripe commented Mar 7, 2024

I'm happy to write some tests for this, but existing tests appear to be broken in CI, and much of the functionality requires Bazel 7.0.0 to work anyway (while CI is still on Bazel 6.3).


Description

Motivation

JDK 17 strongly encapsulates JDK internals, so many libraries need --add-exports= and --add-opens= JVM flags to run. Bazel's Java rules already support adding those flags through java_library(add_opens = [...], add_exports = [...]), and these fields are read in Bazel's java_binary rule. This PR adds the same functionality to Scala rules.

Fixes #1523

simuons and agluszak reacted with eyes emoji
Copy link
Contributor Author

I managed to get a passing build with this PR, but it's a bit unfortunate since it would force users to add some additional lines to their WORKSPACE file.

  1. I'm currently using bazel_features to detect Bazel version for support for add_opens/add_exports.
  2. The project currently recommends that folks run load("@io_bazel_rules_scala//scala:scala.bzl", "rules_scala_setup", "rules_scala_toolchain_deps_repositories") in WORKSPACE. But loading @io_bazel_rules_scala//scala:scala.bzl also loads Bazel rules like scala_library, which requires @bazel_features.
  3. For load("@bazel_features//...") to work, the WORKSPACE file needs to run bazel_features_deps() before the attempted load.

The sum effect of this is that users must have

http_archive(
 name = "bazel_features",
 sha256 = "d7787da289a7fb497352211ad200ec9f698822a9e0757a4976fd9f713ff372b3",
 strip_prefix = "bazel_features-1.9.1",
 url = "https://github.com/bazel-contrib/bazel_features/releases/download/v1.9.1/bazel_features-v1.9.1.tar.gz",
)
load("@bazel_features//:deps.bzl", "bazel_features_deps")
bazel_features_deps()

in the WORKSPACE file before loading anything from rules_scala – and specifically, we cannot call bazel_features_deps() for them in rules_scala_toolchain_deps_repositories().


I think there are three options:

  1. Bite the bullet and force every user to install bazel_features. (That's what this PR would do, in its current shape.)

  2. Abandon support for Bazel 6.x. (Bazel 7.0.0+ has proper support for this feature.)

  3. Change the .bzl file structure so that files that should be loaded from BUILD files are separate from those that should be loaded from WORKSPACE files. In other words, lift rules_scala_setup and rules_scala_toolchain_deps_repositories out of @io_bazel_rules_scala//scala:scala.bzl, and into something like @io_bazel_rules_scala//scala:workspace.bzl.

    This way, users still have to load bazel_features – but at least we can call bazel_features_deps() for them.

I think option 2 seems most straightforward, though I'll leave it up to the maintainers to decide.

@liucijus @simuons

Copy link
Collaborator

simuons commented Apr 22, 2024

Hi, @timothyg-stripe, so this feature can be enabled only for bazel 7 and up. Could this be done with skylib versions?

Copy link
Contributor Author

@simuons unfortunately, skylib versions isn't sufficient since versions.get does not work in a BUILD file. See bazelbuild/bazel#4566.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liucijus liucijus Awaiting requested review from liucijus

@simuons simuons Awaiting requested review from simuons simuons is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add add_opens and add_exports support

AltStyle によって変換されたページ (->オリジナル) /