bazel-contrib / bazel-contrib/rules_jvm_external
Ruleset problems in large organisations (flakey lockfile, merge races, inefficient fetching)
- Dominant language
- Java
- Stars
- 373
- Forks
- 301
- Avg merge
- 7d 17h
- Merged PRs (30d)
- 3
Description
# Summary
I have some feedback for **improving the lockfile format and how we declare Maven repositories in rules_jvm_external**, based on our experience using the ruleset at Canva. I hope this can start a discussion or be used to further iterate the lockfile format. There are three points of interest here, so **let me know if you'd like to break the discussion into separate issues to make it easier to follow**. That said, all the below suggestions are cohesive and jointly improve both the lockfile and the `maven_install` rule.
My recommendations are:
1. Make the lockfile resilient against network flakes, so that if the third party code contents (measured by a hash) doesn't change, the lockfile doesn't change. Currently, [the lockfile will change](https://github.com/bazelbuild/rules_jvm_external/pull/896/commits/4c2cc90a92e37ee2546c46dc10aefbba048ec414#r1188163702) if repository used to retrieve a particular dependency during `bazel run @unpinned_maven//:pin` changes since the last time it was run, [even if the same third party code contents](https://github.com/bazelbuild/rules_jvm_external/pull/896/commits/4c2cc90a92e37ee2546c46dc10aefbba048ec414#r1188163702) were downloaded both times (i.e. no `sha` fields changed in the lockfile).
2. Make lockfile merge conflicts able to be more easily auto-merged, so that it doesn't create merge races in large organisations with many concurrent package updates and consequent lockfile changes.
3. Avoid redundant network fetch attempts by specifying which repos should host which packages (via the `maven_install` rule). Additionally, allow users to specify which repos are mirrors and which provide unique packages.
# Problems
The [`maven_install.json` lockfile recommended by rules_jvm_external](https://github.com/bazelbuild/rules_jvm_external/tree/master#pinning-artifacts-and-integration-with-bazels-downloader) is great for reproducing builds assuming you don't change it, but it's difficult to evolve or verify as a lockfile (especially for large projects with many third-party Maven dependencies).
IMO a good lockfile should do all of:
0. Allow reproducing builds with the exact same third-party code between builds.
1. Be resilient against network flakiness.
2. Be resilient against large volumes of changes.
3. Permit efficient fetching.
`maven_install.json` is good at 0, but in my experience at a larger organisation like Canva, it isn't so good at 1, 2, and 3. I'll highlight these problem in more detail below.
## 1: Network flakiness
We enforce that engineers update the `maven_install.json` by regenerating it on our CI system before allowing feature branches to merge to our main branch. However, occasional network flakes can cause widespread changes in the lockfile. For example:
1. I update Maven package `foo` on my feature branch.
2. I run `bazel run @unpinned_maven//:pin`, update the lockfile. An existing package `bar` is fetched from a mirror, `repo2.com`, because the first repo `repo1.com` in the list happened to be down when I ran the command. The lockfile contains `repositories: { repo2.com: [ bar ] }`.
3. I push my changes and run a CI check before I can merge to master. CI runs `bazel run @unpinned_maven//:pin`, but the first repo is back online, so it generates a different lockfile with `repositories: { repo1.com: [ bar ] }`.
4. Diffing the lockfile in order to detect an outdated `maven_install.json` is now producing a false error, because the third party contents didn't change but the lockfile did.
We had to work around this when `repo.maven.apache.org/maven2/` went down a few weeks ago. We fixed it by ignoring the fields that change in the lockfile during network flakes, like `repositories`. However, it would be convenient if the lockfile was simply diffable to check whether a meaningful change occurred in the closure of third-party packages.
## 2: Racing against merge conflicts
We have many engineers committing to the main branch throughout a typical day. If two people's feature branches update `maven_install.json`, perhaps by bumping or adding dependencies, it's often a brutal race for them to get their changes in because the slower feature branch to merge will become blocked by merge conflicts induced by the faster one. [Not all lockfiles suffer from this problem.](https://github.com/TerrorJacktyl/yarn-lockfile-auto-resolution-example/pull/1#pullrequestreview-1417706307)
## 3: Inefficient fetching
When running `bazel run @unpinned_maven//:pin`, for every package it tries fetching the package from each repository in the `maven_install` target's `repositories` field.
1. This is ineffective, since AFAIK it doesn't fetch in parallel, and there are some repositories we don't expect to contain particular packages (e.g. we don't expect a Maven Central repo like `repo1.maven.org/maven2` to contain our custom, self-hosted packages).
2. This is also painful because it generates a bunch of warnings in our logs, even those it's (implicitly) expected behaviour to see `com.canva.custom-package` fail to fetch from a Maven Central repo. Because we only have a single `repositories` list field in `maven_install`, we are forced to conflate mirror repositories with unique repositories (i.e. repositories that host a perhaps entirely different set of packages).
# Recommendations
Addressing the problems above in order, here are some suggested solutions.
## 1: Make the lockfile format immune to network flakes
We need to avoid changing the lockfile based on the particular repository that was used to fetch a package. We might solve this by removing the lockfile's `repositories` object field altogether (since it currently just holds the single first repository that successfully returned a particular package).
If it's necessary to have the repository URLs listed in the lockfile, see point 3 below.
## 2: Make the lockfile automatically merge-able
I can't speak for all version control systems, but Git tries to resolve conflicts by checking whether the "same lines" were changed across two conflicting commits. One solution might be moving the lockfile to YAML or something resembling the [yarn lockfile format, which doesn't suffer from the same degree of merge conflicts.](https://github.com/TerrorJacktyl/yarn-lockfile-auto-resolution-example/pull/1#pullrequestreview-1417706307) In our experience at Canva, yarn lockfile merge conflicts are less frequent than `maven_install.json` merge conflicts.
Some care would be required to ensure that merging two conflicting lockfiles always results in the same lockfile you'd get from running `bazel run @unpinned_maven//:pin` on the version of the `maven_install` target with all changes resolved.
## 3: Allow specification of mirrors in `maven_install`
We could extend `maven_install` to take:
1. A dictionary from repository groups to repository URL lists. For example, one group might be `maven-central`, and another might be `jooq`.
```
repositories = {
"maven-central": ["https://repo1.maven.org/maven2/", "http://repo.maven.apache.org/maven2/"],
"jooq": ["repo.jooq.org"],
}
```
3. A dictionary of packages, which must depend on a group of repositories.
```
artifacts = {
"com.google.guava": "maven-central",
"org.apache.logging.log4j": "maven-central",
"org.jooq.jooq": "jooq",
}
```
Using this method, `bazel run @unpinned_maven//:pin` would not need to redundantly try to fetch `jooq` from the `central` repositories. Also, the only "failed to fetch" warnings we'd see in the logs would be due to actual network flakes (i.e. a true warning).
# Conclusion
Thank you for reading this far! I hope our experience with rules_jvm_external at a large organisation can help improve this ruleset. 😊
Contributor guide
Research direction
Start with the maven_install rule and maven_install.json, then run bazel run @unpinned_maven//:pin to understand the current lockfile and repository behavior. The work is done when lockfile updates are stable across network mirrors and concurrent dependency changes, while maven_install can avoid fetching packages from irrelevant repositories as described in the three recommendations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- build-system
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100