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

Support path mapping #1664

Open
jadenPete opened this issue Dec 9, 2024 · 4 comments
Open

Support path mapping #1664

jadenPete opened this issue Dec 9, 2024 · 4 comments

Comments

@jadenPete
Copy link
Contributor

This issue is part of a broader proposal to merge some of the features in lucidsoftware/rules_scala into this ruleset.

The tracking issue for this effort

The relevant proposal section.

Path mapping should prevent issues with targets from being built multiple times in multi-toolchain repositories. These issues are discussed in more detail in the proposal, but the gist is that if a target is depended on under different configurations, it may be built multiple times, even if the difference in configuration doesn't affect how the target is built. This is because what toolchain is used to build a Scala target depends on Bazel builds targets under different output directories, depending on the configuration. Path mapping attempts to solve this by "[rewriting] paths in action command lines with the aim of making them more likely to be disk or remote cache hits", and it's solved this issue for us over at lucidsoftware/rules_scala.

@crt-31
Copy link
Contributor

crt-31 commented Jan 30, 2025

Path mapping is a "highly experimental" Bazel feature, and I don't believe rules_scala should depend on experimental features. If this path mapping can be optionally enabled, then that would be fine.

@jadenPete
Copy link
Contributor Author

That's valid. At the very least, we'd like to update this ruleset to support path mapping (i.e. satisfying the requirements listed here). Two important things to note are that:

  1. Satisfying these requirements wouldn't remove a repository's ability to disable path mapping; doing so would only make it more in-line with Bazel's best practices for writing rulesets
  2. Whether path mapping is enabled or disabled is dependent on the options provided to the Bazel server. Even if we wanted to, we couldn't force users of this ruleset to enable path mapping; we can only make the ruleset compatible with path mapping

We'd just like the option to enable path mapping, since without it, large, multi-Scala version repositories will suffer immensely from redundant builds. It's for that reason that I'd push for path mapping the most, of any experimental feature we're proposing.

From the discussion on path mapping I linked above:

Hi, I wanted to update that after using path mapping in our project we went from ~38m build time to ~6m!
We still have a lot of CppArchive duplicates which we think could make our build even faster. Do you think you would be able to get it into bazel 8.1.0?

At Lucid Software, I think we could see similarly drastic improvements in build time.

@crt-31
Copy link
Contributor

crt-31 commented Feb 8, 2025

doing so would only make it more in-line with Bazel's best practices for writing rulesets

Where are you getting the "Bazel's best practices for writing rulesets" from?

@jadenPete
Copy link
Contributor Author

Where are you getting the "Bazel's best practices for writing rulesets" from?

From what I understand from the documentation here, here and here, using ctx.actions.args()—one of the requirements for rulesets supporting path mapping—is already a recommended practice because it reduces the amount of memory allocated at analysis time. This is because things like Args#add_all, the map_each argument, etc. aren't evaluated until analysis is complete.

Path mapping takes this to another extreme by requiring that (1) ctx.actions.args() is used and (2) that File#path isn't accessed until after analysis is complete. This is because with path mapping enabled, Bazel doesn't know what the path will be to an input until the action is executed.

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

No branches or pull requests

2 participants