Replace @scalafmt_default with toolchains target
#1725
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbland
force-pushed
the
replace-scalafmt-default-repo
branch
from
e495b34 to
ce3234a
Compare
simuons
reviewed
Apr 28, 2025
docs/phase_scalafmt.md
Outdated
|
|
||
| - Put `.scalafmt.conf` at the root of your workspace | ||
| - Pass `.scalafmt.conf` in via `scala_toolchains`: | ||
|
|
||
| ```py | ||
| # MODULE.bazel | ||
| scala_deps.toolchains( | ||
| scalafmt = True, |
There was a problem hiding this comment.
Could we get rid of this flag? I mean scala_deps.scalafmt(default_config = "//path/to/my/custom:scalafmt.conf")
implies scalafmt = True,
For workspace sure we would need to specify boolean flag and pass config as there are no other way to render this in single repository.
There was a problem hiding this comment.
Ah, that was just a mistake in the docs. Fixed the commit while rebasing on master, and the build is passing.
But speaking of the boolean flag in WORKSPACE, well... The idea from #1722 (comment) of using the module extension to instantiate user defined toolchains is haunting me. I started experimenting with the idea in mbland/bzlmod-toolchains-extension.
In the process, I found a way to make the scalafmt, scala_proto, and twitter_scrooge parameters accept either a boolean (where True implies "use all default settings") or a dict (which implies True). I also updated the semantics of scala/extensions/deps.bzl somewhat, hopefully making them clearer and more consistent. I also changed scala_proto(options) to scala_proto(default_gen_deps) to match setup_scala_proto_toolchains().
I don't know that I'm close yet to making it work for all user defined toolchains, but it should make it easier for users to configure the builtin toolchains more precisely. I'll send a pull request from that new branch soon after this pull request lands.
Removes the `scalafmt_config()` macro and replaces it with the new `@rules_scala_toolchains//scalafmt:config` target. The new `test/shell/test_dependency_versions.sh` test found a problem with the previous implementation. The `dev_deps` extension in `MODULE.bazel` generated `@scalafmt_default`, leaving it invisible to `rules_scala` when it's not the main module: ```txt ERROR: no such package '@@[unknown repo 'scalafmt_default' requested from @@rules_scala~]//': The repository '@@[unknown repo 'scalafmt_default' requested from @@rules_scala~]' could not be resolved: No repository visible as '@scalafmt_default' from repository '@@rules_scala~' ERROR: .../tmp/test_dependency_versions/BUILD:52:20: every rule of type scalafmt_scala_test implicitly depends upon the target '@@[unknown repo 'scalafmt_default' requested from @@rules_scala~]//:config', but this target could not be found because of: no such package '@@[unknown repo 'scalafmt_default' requested from @@rules_scala~]//': The repository '@@[unknown repo 'scalafmt_default' requested from @@rules_scala~]' could not be resolved: No repository visible as '@scalafmt_default' from repository '@@rules_scala~' Documentation for implicit attribute config of rules of type scalafmt_scala_test: The Scalafmt configuration file. ERROR: Analysis of target '//:ScalafmtTest' failed; build aborted: Analysis failed ``` The `scalafmt_default_config()` macro is already gone, and only `scala_toolchains()` invoked `scalafmt_config()`, making this a straightforward change.
mbland
force-pushed
the
replace-scalafmt-default-repo
branch
from
ce3234a to
819449a
Compare
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Apr 30, 2025
This avoids the need for the user to use `exports_files` so `@rules_scala_toolchains//scalafmt:config` can access the config file. Essentially restores the API from before bazel-contrib#1725, but still fixes the same bug as bazel-contrib#1725.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Apr 30, 2025
Updates the Scalafmt documentation to reflect the current API. Adds a check to `scala_toolchains_repo` to `fail` if the Scalafmt `default_config` file doesn't exist. The previous commit doesn't actually restore the exact pre-bazel-contrib#1725 API. It eliminates the `exports_files` requirement, but still requires a `Label`, not just a relative path string or a `.scalafmt.conf` in the root directory. After experimenting a bit and thinking this through, it seems the explicit `Label` requirement provides the most robust and reliable interface. Specifically, supporting the previously optional `.scalafmt.conf` in the root directory requires detecting whether it actually exists before falling back to the default. Having users explicitly specify their own config seems a small burden to impose for a more straightforward and correct implementation. At the same time, I saw the opportunity to provide the user with explicit feedback if the specified config file doesn't exist. Hence the new check and `fail()` message. Also renamed the generated `.scalafmt.conf` file in `@rules_scala_toolchains//scalafmt` to `scalafmt.conf`. No need for it to be a hidden file in that context.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
Apr 30, 2025
Updates the Scalafmt documentation to reflect the current API. Adds a check to `scala_toolchains_repo` to `fail` if the Scalafmt `default_config` file doesn't exist. The previous commit doesn't actually restore the exact pre-bazel-contrib#1725 API. It eliminates the `exports_files` requirement, but still requires a `Label` or a relative path string, not an optional `.scalafmt.conf` in the root directory. After experimenting a bit and thinking this through, dropping support for an optional `.scalafmt.conf` provides the most robust and reliable interface. Specifically, supporting it requires detecting whether it actually exists before falling back to the default. Having users explicitly specify their own config seems a small burden to impose for a more straightforward and correct implementation. At the same time, I saw the opportunity to provide the user with explicit feedback if the specified config file doesn't exist. Hence the new check and `fail()` message. Also renamed the generated `.scalafmt.conf` file in `@rules_scala_toolchains//scalafmt` to `scalafmt.conf`. No need for it to be a hidden file in that context.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
May 6, 2025
This avoids the need for the user to use `exports_files` so `@rules_scala_toolchains//scalafmt:config` can access the config file. Essentially restores the API from before bazel-contrib#1725, but still fixes the same bug as bazel-contrib#1725.
mbland
added a commit
to mbland/rules_scala
that referenced
this pull request
May 6, 2025
Updates the Scalafmt documentation to reflect the current API. Adds a check to `scala_toolchains_repo` to `fail` if the Scalafmt `default_config` file doesn't exist. The previous commit doesn't actually restore the exact pre-bazel-contrib#1725 API. It eliminates the `exports_files` requirement, but still requires a `Label` or a relative path string, not an optional `.scalafmt.conf` in the root directory. After experimenting a bit and thinking this through, dropping support for an optional `.scalafmt.conf` provides the most robust and reliable interface. Specifically, supporting it requires detecting whether it actually exists before falling back to the default. Having users explicitly specify their own config seems a small burden to impose for a more straightforward and correct implementation. At the same time, I saw the opportunity to provide the user with explicit feedback if the specified config file doesn't exist. Hence the new check and `fail()` message. Also renamed the generated `.scalafmt.conf` file in `@rules_scala_toolchains//scalafmt` to `scalafmt.conf`. No need for it to be a hidden file in that context.
simuons
pushed a commit
that referenced
this pull request
May 7, 2025
* Add toolchain options API to WORKSPACE and Bzlmod Updates `scala_toolchains()` to accept either boolean or dict arguments for specific toolchains, and updates `//scala/extensions:deps.bzl` to generate them from tag classes. Part of #1482. Notable qualities: - Adds toolchain options support to the `scala_toolchains()` parameters `scalafmt`, `scala_proto`, and `twitter_scrooge`, and to the `scalafmt` tag class. - Eliminates the `scalafmt_default_config`, `scala_proto_options`, and `twitter_scrooge_deps` option parameters from `scala_toolchains()`. - Provides uniform, strict evaluation and validation of toolchain options passed to `scala_toolchains()`. - Configures enabled toolchains using root module settings or the default toolchain settings only. - Introduces the shared `TOOLCHAIN_DEFAULTS` dict in `//scala/private:toolchains_defaults.bzl` to aggregate individual `TOOLCHAIN_DEFAULTS` macro parameter dicts. This change also: - Replaces the non-dev dependency `scala_deps.scala()` tag instantiation in `MODULE.bazel` with `dev_deps.scala()`. - Renames the `options` parameter of the `scala_deps.scala_proto` tag class to `default_gen_opts` to match `setup_scala_proto_toolchain()`. - Introduces `_stringify_args()` to easily pass all toolchain macro args compiled from `scala_toolchains_repo()` attributes through to the generated `BUILD` file. - Extracts `_DEFAULT_TOOLCHAINS_REPO_NAME` and removes the `scala_toolchains_repo()` macro. - Includes docstrings for the new private implementation functions, and updates all other docstrings, `README.md`, and other relevant documentation accordingly. --- Inspired by @simuons's suggestion to replace toolchain macros with a module extension in: - #1722 (comment) Though a full replacement is a ways off, this is a step in that direction that surfaced several immediate improvements. First, extensibility and maintainability: - The new implementation enables adding options support for other toolchains in the future while maintaining backward compatibility, for both the `WORKSPACE` and Bzlmod APIs. Adding this support will only require a minor release, not a major one. - The `scala_deps` module extension implementation is easier to read, since all toolchains now share the `_toolchain_settings` mechanism. Next, improved consistency of the API and implementation: - Toolchain options parameters should present all the same parameters as the macros to which they correspond, ensured by the `TOOLCHAIN_DEFAULTS` mechanism. This is to make it easier for users and maintainers to see the direct relationship between these separate sets of parameters. (They can also define additional parameters to those required by the macro, like `default_config` from the `scalafmt` options.) This principle drove the renaming of the `scala_deps.scala_proto` tag class parameter from `options` to `default_gen_opts`. It also inspired updating `scala_toolchains_repo()` to pass toolchain options through `_stringify_args()` to generate `BUILD` macro arguments. - The consolidated `TOOLCHAIN_DEFAULTS` dict reduces duplication between the `scala/extensions/deps.bzl` and `scala/toolchains.bzl` files. It ensures consistency between tag class `attr` default values for Bzlmod users and the `scala_toolchains()` default parameter values for `WORKSPACE` users. The `TOOLCHAINS_DEFAULTS` dicts corresponding to each toolchain macro do duplicate the information in the macro argument lists. However, the duplicated values in this case are physically adjacent to one another, minimizing the risk of drift. - Extracting `_DEFAULT_TOOLCHAINS_REPO_NAME` is a step towards enabling customized repositories based on the builtin toolchains, while specifying different options. This extraction revealed the fact that the `scala_toolchains_repo()` macro was no longer necessary, since only `scala_toolchains()` ever called it. Finally, fixes for the following design bugs: - Previously, `scala_deps.scala_proto(options = [...])` compiled the list of `default_gen_opts` from all tag instances in the module graph. This might've been convenient, but didn't generalize to other options for other toolchains. In particular, it differed from the previous `toolchains`, `scalafmt`, and `twitter_scrooge` tag class behavior. The new semantics are unambiguous, consistent, and apply to all toolchains equally; they do not show a preference for any one toolchain over the others. They also maintain the existing `scalafmt` and `twitter_scrooge` tag class semantics, but now using a generic mechanism, simplifying the implementation. - Instantating `scala_deps.scala()` was a bug left over from the decision in #1722 _not_ to enable the builtin Scala toolchain by default under Bzlmod. The previous `scala_deps.toolchains()` tag class had a default `scala = True` parameter. The user could set `scala = False` to disable the builtin Scala toolchain. After replacing `toolchains()` with individual tag classes, the documented behavior was that the user must enable the builtin Scala toolchain by instantiating `scala_deps.scala()`. By instantiating `scala_deps.scala()` in our own `MODULE.bazel` file, we ensured that `rules_scala` would always instantiate the builtin Scala toolchain. While relatively harmless, it violated the intention of allowing the user to avoid instantiating the toolchain altogether. * Update documentation for toolchain option dicts Touched up documentation for the `scalafmt` and `scala_proto` toolchains. * Replace Scalafmt default_config alias with symlink This avoids the need for the user to use `exports_files` so `@rules_scala_toolchains//scalafmt:config` can access the config file. Essentially restores the API from before #1725, but still fixes the same bug as #1725. * Update phase_scalafmt.md, check scalafmt conf path Updates the Scalafmt documentation to reflect the current API. Adds a check to `scala_toolchains_repo` to `fail` if the Scalafmt `default_config` file doesn't exist. The previous commit doesn't actually restore the exact pre-#1725 API. It eliminates the `exports_files` requirement, but still requires a `Label` or a relative path string, not an optional `.scalafmt.conf` in the root directory. After experimenting a bit and thinking this through, dropping support for an optional `.scalafmt.conf` provides the most robust and reliable interface. Specifically, supporting it requires detecting whether it actually exists before falling back to the default. Having users explicitly specify their own config seems a small burden to impose for a more straightforward and correct implementation. At the same time, I saw the opportunity to provide the user with explicit feedback if the specified config file doesn't exist. Hence the new check and `fail()` message. Also renamed the generated `.scalafmt.conf` file in `@rules_scala_toolchains//scalafmt` to `scalafmt.conf`. No need for it to be a hidden file in that context. * Update the `scala_deps` module extension docstring The `scala_deps` docstring now more accurately describes the behavior implemented by `_toolchain_settings()`. * Fix `scala_proto` param in test_version/WORKSPACE Caught this after doing an `--enable_workspace --noenable_bzlmod` build.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Removes the
scalafmt_config()macro and replaces it with the new@rules_scala_toolchains//scalafmt:configtarget. Part of #1482.Motivation
The new
test/shell/test_dependency_versions.shtest found a problem with the previous implementation. Thedev_depsextension inMODULE.bazelgenerated@scalafmt_default, leaving it invisible torules_scalawhen it's not the main module:The
scalafmt_default_config()macro is already gone, and onlyscala_toolchains()invokedscalafmt_config(), making this a straightforward change.