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

Provide select_for_scala_version utility macro #1563

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

aszady
Copy link
Contributor

@aszady aszady commented Mar 28, 2024

Description

For later use in BUILD files of rules_scala where customization depending on Scala version is needed.
End users are also encouraged to use this one.

The approach in this PR differs from the one in #1552, as with selects we will avoid the need of multiplying targets within rules_scala for each Scala version configured.
This comes at a cost of not so easy access to the internal binaries for a specific, non-default version. #1552 proposed suffixing target names with the Scala version, whereas here the target built depends on a configuration – harder to specify via command-line. Possible workaround for this, if ever needed, would be to use a flag (see: #1558 (comment)).

The choice of matchers is quite arbitrary, but covers the usage required at the moment.

See usage in #1564.

Motivation

Originally #1290.

@aszady aszady requested review from liucijus and simuons as code owners March 28, 2024 11:05
@aszady aszady force-pushed the select branch 2 times, most recently from 4dd3950 to 7cfb44f Compare March 28, 2024 13:53
@simuons
Copy link
Collaborator

simuons commented Apr 15, 2024

Hi, @aszady, would it be possible to write some tests for this macro?

I'm not sure about exposing this macro for end users right now. I mean such sophisticated matchers might be needed internally to support wide range of scala versions. But end users (presumably) would have only handful set of scala versions and could write regular selects.

Could you make this bzl private for now?

I think maybe what's missing is more coarse grained config_settings for scala version at @io_bazel_rules_scala_config ie:

  • @io_bazel_rules_scala_config//:scala_version_2
  • @io_bazel_rules_scala_config//:scala_version_2_12
  • @io_bazel_rules_scala_config//:scala_version_2_13
  • @io_bazel_rules_scala_config//:scala_version_3

That would allow to have more stable selects. wdyt?

@aszady
Copy link
Contributor Author

aszady commented Apr 17, 2024

would it be possible to write some tests for this macro?

@simuons, do you have any specific idea in mind for such tests? I think we cannot test the macro itself, as we haven't enabled multiple versions yet, so such test would be quite trivial.
Other option would be to writ unit tests for some implementation details, like _matches_for_version – not sure if satisfactory?
Imo the best would be to set up a separate test repository, e.g. test_cross_build and do the full analysis-phase test there. But that's also something we can't do yet, without multiple versions enabled.

I'm not sure about exposing this macro for end users right now.

Certainly it may seem that some matchers are here just for internal support, with most striking example of between…and… to use for after_2_12_13_and_before_2_13_12-specific code.
My reasoning for exposing this macro was that it expresses closer the intention and could make the maintenance easier.

With the macro we say: this will/won't work for such Scala versions and we usually can state which Scala version caused the change. This information is irrelevant to currently enabled scala_versions and therefore ideally needs no updates when adding extra versions (easier Scala version change).
With the pure select only: the original intention may get intermixed with the current workspace setup.

The coarser-grained Scala version settings is something in between, as it emulates the any matcher – potentially the most needed one. Though in this case again it may require extra work (per each select()) when Scala versions get added or removed. On the other hand I can imagine that most of the customization needs would revolve around Scala 2 vs. 3 and more precise selections would be quite rare and we can neglect them when estimating impact of this change.

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.

I think one way of testing this could be to extract a function that builds a map for select and then test that with https://bazel.build/rules/testing#testing-starlark-utilities

Probably writing analysis test is not feasible right now and those tend to be very tedious.

To be clear: this macro looks nice and is well documented. My only concern was that it might be to powerful for end users and if we could stick just to plain select that would be more familiar.

I have no objections for merging this. Any way it already takes longer than it should and blocks other PRs.

Approving. Thanks.

@liucijus liucijus merged commit 8158717 into bazelbuild:master Apr 22, 2024
2 checks passed
@aszady aszady deleted the select branch April 22, 2024 21:38
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.

3 participants