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

Update docs for version 7.0.0, remove old APIs #1703

Merged
merged 11 commits into from
Feb 28, 2025

Conversation

mbland
Copy link
Contributor

@mbland mbland commented Feb 18, 2025

Description

Describes the new scala_toolchains() API, breaking changes, Bazel compatibility matrix, and breaking of the io_bazel_rules_scala repo name dependency. Part of #1482, #1647, #1652, and #1699, it also removes obsolete WORKSPACE APIs in favor of scala_toolchains(), et. al.

Much of the README text regarding WORKSPACE configuration, Bazel compatibility, and breaking changes comes from:

The "Breaking changes in rules_scala 7.x" section of README.md describes the removed WORKSPACE macros, and provides guidance for adopting the scala_toolchains() API. Also includes information about the upcoming Bzlmod implementation (#1482) and the Bazel 8 compatibility changes for rules_scala 8.x (#1652).

Contains a light editing pass over almost every Markdown file to resolve markdownlint warnings when editing in VSCode. Also added .markdownlint.json to silence rule MD033/no-inline-html, since several docs contain <br/> and/or <table> elements.

Performed other opportunistic grammar edits and added new information, including:

  • docs/coverage.md: Describes how to determine the default JaCoCo version used by rules_java

  • scripts/README.md: Updates create_repository.py docs extensively, and adds a section for sync_bazelversion.sh

  • Replaces starlark code fences with py, since the syntax is identical and editors seem to support it better.

Motivation

Since the next major release is imminent, ensuring the documentation accurately reflects all the changes since v6.6.0 has become an urgent priority.

Rather than leave the old WORKSPACE APIs in place, which could lead to surprising failures, this change removes all of them. This also changes some code that still depended on some of these obsolete macros, including scala_toolchains(). Since all the toolchainization changes led to the entire project already using scala_toolchains() and scala_register_toolchains() exclusively, this proved a low risk refactoring.

With some Bzlmod and Bazel 8 information already in place, very little will need to change when these implementations land. The commits that contain those implementations will also include their relevant documentation updates.

Copy link
Contributor

@bartoszkosiorek bartoszkosiorek left a comment

Choose a reason for hiding this comment

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

Great job!
It is well written, and explain rules_scala 7.0 migration details.
Thanks.

README.md Outdated
[Install Bazel]: https://docs.bazel.build/versions/master/install.html
[Bazelisk]: https://docs.bazel.build/versions/master/install.html

Add the following stanza to your `WORKSPACE` file and update versions with their
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what is stanza?
Unfortunately it is not common word. Maybe you could replace it with code or code snippet?
I was a little bit confused about it at first :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry. Updated in b294878.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be used originally in the context of literature, but it is also commonly used in the context of IT for a group of lines (similarly as in literature).

mbland added a commit to mbland/rules_scala that referenced this pull request Feb 21, 2025
Thanks to @bartoszkosiorek for pointing out in bazelbuild#1703 that "stanza" isn't
a common word. That was my former English Lit major innie leaking out,
applying a poetry term (a group of lines, like a paragraph) out of
context.
@mbland
Copy link
Contributor Author

mbland commented Feb 21, 2025

Great job! It is well written, and explain rules_scala 7.0 migration details. Thanks.

@bartoszkosiorek Thanks!

Also, I've added a section about having to use use_repo() to bring repos into scope under Bzlmod, based on the discussion with @michalbogacz mentioned in be73da7.

That discussion also got me to realize that the native.toolchain() call in setup_scala_toolchain() could likely move into the scala_toolchain() wrapper macro, to avoid the explicit toolchain() call mentioned in this latest update. I'll open a new pull request for that (if it works, which I expect it will).

mbland added a commit to mbland/rules_scala that referenced this pull request Feb 22, 2025
@grepwood
Copy link
Contributor

Hi, I've proofread the updated README.md and I'd like to comment on a couple thing that could help improve this doc.

  1. Mixing plaintext and monospace text in the same hyperlink slices up the hyperlink underline into many pieces. This causes confusion.
    image
    On first glance this is 4 different links, because the vertical blue underline under blue text is 4 separate vertical lines. But when you inspect the code, it's 1 link. I think this could be better worded to avoid mixing plaintext and monospace in the same hyperlink text. There's more examples of this:
    image
    Again - 2 separate horizontal lines are probably 2 separate links. It's actually 1.
    image
    And there's one more.
    image
    That's it.

  2. Issue Protobuf compatibility findings and suggestions #1647 is not linked. Why?
    image

  3. When reading the Getting started part, paragraph 2 and 4 are slightly confusing. Paragraph 2 says Add the following stanza and this stanza never materializes immediately after paragraph 2. The only code snippet that follows first after this paragraph, is the one referenced by paragraph 4. As per p4's wording, the code snippet that follows, is in reference to see the New scala_toolchains() API for WORKSPACE section below. I'm now confused - is this code snippet supposed to be just for paragraph 4, just for paragraph 2, or for both? And if it's only for only one of them, where did the other paragraph's code disappear?

mbland added a commit to mbland/rules_scala that referenced this pull request Feb 25, 2025
- Updates links not to use code tags, since it gives the appearance of
  multiple adjacent links.

- Updates issue reference missing the `bazelbuild/rules_scala` prefix.

- Moves paragraphs above "Getting started" `WORKSPACE` snippet to a new
  "Important changes in `rules_scala` v7.0.0 configuration" section
  following the snippet.
@mbland
Copy link
Contributor Author

mbland commented Feb 25, 2025

@grepwood I've addressed all these points in 00dafd8.

Regarding the reference to #1647, that was just an oversight in that one instance. I think it's linked elsewhere, but I didn't realize until after pushing my first draft that GitHub wants the bazelbuild/rules_scala prefix before issue/PR references when it's in a README, as opposed to a commit or PR.

Regarding those two paragraphs, I've moved them to a new section below the WORKSPACE snippet. I did think they were awkward there, but couldn't think of a better place at the time in the midst of making all the other updates. They do seem to fit OK in this new section; let me know what you think.

load("....bzl", "srcs")
load("@io_bazel_rules_scala_config//:config.bzl", "SCALA_VERSIONS")
load("@rules_scala//:scala_cross_version.bzl", "version_suffix")

scala_library(
...
srcs = select({
"@io_bazel_rules_scala_config//:scala_version" + version_suffix(v): srcs(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are still a lot of "io_bazel_rules_scala" instances laying around in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, though I did double check and found two more that I've since updated in 5bb4529 (one in README.md, another in .github/workflows/workspace_snippet.sh).

As mentioned in that commit message, the remaining references are actually prefixed io_bazel_rules_scala_ (notice the trailing _). Most of them are private, with the exception of @io_bazel_rules_scala_config, which is a documented public interface.

I also mention that we could change this to @rules_scala_config as part of the breaking changes for v7.0.0. If @simuons and/or @liucijus say the word, I'm happy to do that in a separate pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mbland I'm ok with renaming @io_bazel_rules_scala_config since there will be breaking changes as you mentioned and there is no reason for keeping label misaligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simuons Thanks, I'll take care of that in a follow-up PR. That'll be another sweeping, albeit straightforward change similar to #1696, and this current PR has already gotten bit enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Preview: rules-scala-config-repo-name in mbland/rules_scala (specifically mbland@d95c0e7).

It wasn't as big a diff as #1696, but still probably worth its own PR.

README.md Outdated

rules_scala_dependencies()

load("@rules_java//java:rules_java_deps.bzl", "rules_java_dependencies")
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried doing this, and got:

ERROR: Error computing the main repository mapping: cannot load '@@rules_java//java:rules_java_deps.bzl': no such file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh! Thanks for that catch. Fixed in 85519d0.

As I mention in that commit message, this was the correct snippet for rules_java 8.x, used in my Bazel 8 compatibility branch. It's now updated for rules_java 7.x, matching what's in the current WORKSPACE files, with some comments explaining what to expect when rules_java 8.x lands in this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we'd need a documentation change when we'd land rules_java 8.x (maybe add that on a branch already?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course! I've already been updating the README.md in bzlmod-bazel-8. 😉 As I've described in several of my "weekly updates" from #1482, I've been keeping that branch and bzlmod up to date with the latest merges to master and other developments.

This PR contains the bulk of the necessary doc updates, so it's a much lighter lift to include doc updates in those other branches now.

mbland added a commit to mbland/rules_scala that referenced this pull request Feb 25, 2025
The `WORKSPACE` snippet in the `README.md` now matches what's currently
in all the other `WORKSPACE` files.

I'd previously included the `rules_java` 8.x statements from my Bazel 8
compatibility branch in the `README.md` changes for `rules_scala` 7.x.
@gergelyfabian tried the previous snippet from bazelbuild#1703, and it broke his
build:

- bazelbuild#1703 (comment)
mbland added a commit to mbland/rules_scala that referenced this pull request Feb 25, 2025
Includes a full editing pass over `docs/cross-compilation.md` and
removes the `incompatible_use_toolchain_transition` attribute, which
has been deprecated since before Bazel 6.5.0.

- https://bazel.build/versions/6.4.0/rules/lib/globals?hl=en#rule

The "Cross-build support tiers" could use extra review by someone more
knowledgable about that aspect of `rules_scala`.

Prompted by a suggestion by @gergelyfabian in bazelbuild#1703 to update a single
line, but turned into a complete overhaul.
mbland added a commit to mbland/rules_scala that referenced this pull request Feb 25, 2025
Removes a remaining, incorrect `@io_bazel_rules_scala` reference in
`README.md`, as well as in the `.github/workflows/workspace_snippet.sh` script.
Updates the `README.md` to use `<VERSION>` in place of `7.0.0` in the
`WORKSPACE` snippet under "Getting started".

Prompted by a couple comments by @gergelyfabian in bazelbuild#1703. I used the following
command to confirm no more unintentional `io_bazel_rules_scala` references
remain. (I'm leaving the `test_intellij_aspect.sh` script alone for now.)

```txt
$ git ls-files | xargs grep 'io_bazel_rules_scala[^_]'

.github/workflows/workspace_snippet.sh:    name = "rules_scala",  # Can be "io_bazel_rules_scala" if you still need it.
README.md:    name = "rules_scala",  # Can be "io_bazel_rules_scala" if you still need it.
README.md:- __`rules_scala` no longer requires the `io_bazel_rules_scala` repository
test_intellij_aspect.sh:  bazel test --test_output=errors --override_repository io_bazel_rules_scala="${rules_scala_dir}"  --extra_toolchains=@io_bazel_rules_scala//scala:default_toolchain //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...
```

There are still many `io_bazel_rules_scala_.*` references, most of them for
internal toolchain dependency repos. The `@io_bazel_rules_scala_config`
repository is the exception, which is a publicly documented interface. (We
_could_ change it, as another potential known breakage for `rules_scala`
v7.0.0.)
Copy link
Collaborator

@simuons simuons left a comment

Choose a reason for hiding this comment

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

Thanks, @mbland! Really appreciated. Well done indeed. Looks like there is a conflict, I'll merge once you will resolve it.

Special thanks @bartoszkosiorek and @gergelyfabian for looking into this.

Describes the new `scala_toolchains()` API, breaking changes, Bazel
compatibility matrix, and breaking of the `io_bazel_rules_scala` repo
name dependency. Part of bazelbuild#1482, bazelbuild#1647, bazelbuild#1652, and bazelbuild#1699, it also removes
obsolete `WORKSPACE` APIs in favor of `scala_toolchains()`, et. al.

Much of the README text regarding `WORKSPACE` configuration, Bazel
compatibility, and breaking changes comes from:

- bazelbuild#1482 (comment)

The "Breaking changes in `rules_scala` 7.x" section of `README.md`
describes the removed `WORKSPACE` macros, and provides guidance for
adopting the `scala_toolchains()` API. Also includes information about
the upcoming Bzlmod implementation (bazelbuild#1482) and the Bazel 8 compatibility
changes for `rules_scala` 8.x (bazelbuild#1652).

Contains a light editing pass over almost every Markdown file to resolve
`markdownlint` warnings when editing in VSCode. Also added
`.markdownlint.json` to silence rule `MD033/no-inline-html`, since
several docs contain `<br/>` and/or `<table>` elements.

- https://github.com/DavidAnson/vscode-markdownlint?tab=readme-ov-file#markdownlintconfig

Performed other opportunistic grammar edits and added new information,
including:

- `docs/coverage.md`: Describes how to determine the default JaCoCo
  version used by `rules_java`

- `scripts/README.md`: Updates `create_repository.py` docs extensively,
  and adds a section for `sync_bazelversion.sh`

- Replaces `starlark` code fences with `py`, since the syntax is
  identical and editors seem to support it better.

---

Since the next major release is imminent, ensuring the documentation
accurately reflects all the changes since v6.6.0 has become an urgent
priority.

Rather than leave the old `WORKSPACE` APIs in place, which could lead to
surprising failures, this change removes all of them. This also changes
some code that still depended on some of these obsolete macros,
including `scala_toolchains()`. Since all the toolchainization changes
led to the entire project already using `scala_toolchains()` and
`scala_register_toolchains()` exclusively, this proved a low risk
refactoring.

With some Bzlmod and Bazel 8 information already in place, very little
will need to change when these implementations land. The commits that
contain those implementations will also include their relevant
documentation updates.
It seems the README won't expand refs like bazelbuild#1482. This updates such refs
to use the repo prefix, e.g., bazelbuild#1482.
Thanks to @bartoszkosiorek for pointing out in bazelbuild#1703 that "stanza" isn't
a common word. That was my former English Lit major innie leaking out,
applying a poetry term (a group of lines, like a paragraph) out of
context.
Explicitly notes that some `rules_scala` APIs may break, specifically
`setup_scala_toolchain`. Part of bazelbuild#1482.

Brought to my attention by @michalbogacz in a `#scala` thread in the
Bazel Slack workspace, which was in the context of
michalbogacz/scala-bazel-monorepo#26.

- https://bazelbuild.slack.com/archives/CDCKJ2KFZ/p1740144886159909
- Updates links not to use code tags, since it gives the appearance of
  multiple adjacent links.

- Updates issue reference missing the `bazelbuild/rules_scala` prefix.

- Moves paragraphs above "Getting started" `WORKSPACE` snippet to a new
  "Important changes in `rules_scala` v7.0.0 configuration" section
  following the snippet.
Removes the reference to `@rules_scala_toolchains`, since that's not
really a public interface detail.

Prompted by a ping from @gergelyfabian in a Bazel Slack workspace direct
message, asking about the docstring currently reflected on `master`.
The `WORKSPACE` snippet in the `README.md` now matches what's currently
in all the other `WORKSPACE` files.

I'd previously included the `rules_java` 8.x statements from my Bazel 8
compatibility branch in the `README.md` changes for `rules_scala` 7.x.
@gergelyfabian tried the previous snippet from bazelbuild#1703, and it broke his
build:

- bazelbuild#1703 (comment)
Includes a full editing pass over `docs/cross-compilation.md` and
removes the `incompatible_use_toolchain_transition` attribute, which
has been deprecated since before Bazel 6.5.0.

- https://bazel.build/versions/6.4.0/rules/lib/globals?hl=en#rule

The "Cross-build support tiers" could use extra review by someone more
knowledgable about that aspect of `rules_scala`.

Prompted by a suggestion by @gergelyfabian in bazelbuild#1703 to update a single
line, but turned into a complete overhaul.
Removes a remaining, incorrect `@io_bazel_rules_scala` reference in
`README.md`, as well as in the `.github/workflows/workspace_snippet.sh` script.
Updates the `README.md` to use `<VERSION>` in place of `7.0.0` in the
`WORKSPACE` snippet under "Getting started".

Prompted by a couple comments by @gergelyfabian in bazelbuild#1703. I used the following
command to confirm no more unintentional `io_bazel_rules_scala` references
remain. (I'm leaving the `test_intellij_aspect.sh` script alone for now.)

```txt
$ git ls-files | xargs grep 'io_bazel_rules_scala[^_]'

.github/workflows/workspace_snippet.sh:    name = "rules_scala",  # Can be "io_bazel_rules_scala" if you still need it.
README.md:    name = "rules_scala",  # Can be "io_bazel_rules_scala" if you still need it.
README.md:- __`rules_scala` no longer requires the `io_bazel_rules_scala` repository
test_intellij_aspect.sh:  bazel test --test_output=errors --override_repository io_bazel_rules_scala="${rules_scala_dir}"  --extra_toolchains=@io_bazel_rules_scala//scala:default_toolchain //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...
```

There are still many `io_bazel_rules_scala_.*` references, most of them for
internal toolchain dependency repos. The `@io_bazel_rules_scala_config`
repository is the exception, which is a publicly documented interface. (We
_could_ change it, as another potential known breakage for `rules_scala`
v7.0.0.)
Removed in favor of using the existing `scalatest`, `junit`, and
`specs2` parameters at @simuons's suggestion in:

- bazelbuild#1482 (comment)

Also updated the `scala_toolchains()` docstring slightly and added `doc`
parameters to the `attrs` for `_scala_toolchains_repo()`.
@mbland
Copy link
Contributor Author

mbland commented Feb 26, 2025

Thanks, @mbland! Really appreciated. Well done indeed. Looks like there is a conflict, I'll merge once you will resolve it.

Special thanks @bartoszkosiorek and @gergelyfabian for looking into this.

My pleasure! I've rebased on master after #1700, and added c789cb0 as one final change to address the decision from #1482 (comment) to remove the testing parameter from scala_toolchains().

mbland added a commit to mbland/rules_scala that referenced this pull request Feb 26, 2025
Shortens `@io_bazel_rules_scala_config` to `@rules_scala_config` in
light of the `@io_bazel_rules_scala` removal in bazelbuild#1696. Part of bazelbuild#1482.

Adds a section to `README.md` indicating this as a breaking change, with
advice on working around any breakages that aren't immediately fixable.

Per @simuons's advice in
bazelbuild#1703 (comment).
@simuons simuons merged commit e75d7f1 into bazelbuild:master Feb 28, 2025
2 checks passed
@mbland mbland deleted the bzlmod-docs branch February 28, 2025 14:15
mbland added a commit to mbland/rules_scala that referenced this pull request Feb 28, 2025
Shortens `@io_bazel_rules_scala_config` to `@rules_scala_config` in
light of the `@io_bazel_rules_scala` removal in bazelbuild#1696. Part of bazelbuild#1482.

Adds a section to `README.md` indicating this as a breaking change, with
advice on working around any breakages that aren't immediately fixable.

Includes updates the "Builtin repositories no longer visible by default
under Bzlmod" section to improve clarity.

Per @simuons's advice in
bazelbuild#1703 (comment).
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.

5 participants