Add support for pinning snapshots in gradle resolver#1412
Conversation
d2b0d98 to
7a13a1d
Compare
|
Thank you for the PR. I think there's a couple of things that I think about as I read this:
You are correct that some maven repos do write snapshots versions like this, and that's what makes things tricky. I suspect that the correct fix will be to:
What do you think? |
|
Thanks for the feedback. Let me ask for some more context to understand a bit better your thoughts 😄
By "version", do you mean "timestamp"? If yes, is there maybe a public snapshot you could share that does not have timestamp, that I could use during development and to add tests?
What do you mean here, that snapshots should be handled by all resolvers? If yes, I agree. I was planning to look into the other resolvers after merging this PR. Is that okey, or would you prefer to do it differently?
Let me expand a bit on my comment here. How would this work in bazel?
Integrating snapshots without timestamps goes against bazel fundamentals + it is deprecated from maven 3. This is what makes me question wether this ruleset wants/needs to support it. Wdyt? |
1f1457a to
e16bd7b
Compare
|
@shs96c let me know 🙏 |
e16bd7b to
72ff1e8
Compare
|
I know this is a long reply, but please know that I'm generally supportive of the idea of allowing snapshots to be used, but I'm very wary about how that support should be added. I'm really happy to work with you to find a solution that works for everyone. @acecilia, an example of where the My other concern is that snapshot servers seldom keep an extensive history of snapshots; they get removed fairly swiftly. In one company I worked for, the lifetime of a snapshot was at most 30 days. Using snapshots necessarily means that we've given up on historical builds of our project. That's not an objection to providing support for them, but it's something we should call out in the docs.
It would be nice if the resolvers all supported snapshots, but the main thing is that we should be able to handle both kinds of snapshots (the ephemeral dated ones and the ones that are being replaced) with similar logic. The only safe way to do that is to set the checksum to
Yes. This is true. Having said that, there are notable rulesets in the bazel world that already omit the shasum check by necessity (for example, when
I believe that Bazel will refetch the resource every time the daemon is started. It should be sufficient to do a
Sadly this isn't the case in the wild. Many OSS projects have ephemeral snapshots that are replaced with each new version, and from experience I know that there are corporate servers that work the same way.
Given that we recently had a request to support java 8, I don't think everyone will migrate to maven 3 in a hurry. If we're going to support snapshots, we need to support all of them. So far, I've never implemented snapshot support for these reasons:
The inability to rely on a build is a real problem for me, but I acknowledge that people who deliberately use snapshots accept this risk, so that's not a blocking issue for me. Of these reasons, I think we have a path forward for the first ("just" don't set the checksum), the second is a quality of life thing users of snapshots will have to accept, and you're making a start on the third. I think we can figure this out :) |
72ff1e8 to
d049174
Compare
|
Got it thanks for that detailed answer, I was not aware non-version snapshots were still so widely used 🙏 Updated the PR and added support for non-versioned snapshots in gradle resolver |
e4b6af6 to
d95d82b
Compare
ffdb54e to
213df86
Compare
|
For some reason the PR is failing when validating with bazel 6.4.0. I spent some time but I dont manage to understand why, any help is much appreciated |
|
@smocherla-brex would appreciate your feedback here 🙏 Thanks! |
ee6f9c0 to
7b9e605
Compare
|
I am keen to complete this PR with your guidance, have a look when you have a chance please 🙏 |
9e1b780 to
9490ca5
Compare
|
Apologies for the long delay getting back to you. I've been traveling and sick. I want to review the |
4a04e54 to
ed26238
Compare
1fd3886 to
9fef226
Compare
| // POM-only result rather than null so the caller can still use the POM. | ||
| if (hadPomOnlyKnownPath) { | ||
| return new DownloadResult(coordsToUse, Set.of(), null, null); | ||
| } |
There was a problem hiding this comment.
Check the commit message in c1d50b0 for an explanation of this change (is fixing an existing bug introduced in the last master commit)
|
Ready for another review round |
| "artifacts": { | ||
| "com.example:snapshot": { | ||
| "shasums": { | ||
| "jar": null |
There was a problem hiding this comment.
Here is how a non-timestamped snapshot dependency shows: with the shasum set to null
308026e to
50422f3
Compare
| "shasums": { | ||
| "jar": "9f71d9a756937d784cd175af7e3a1992f088321435b880af02488003da4c43b2" | ||
| }, | ||
| "version": "999.0.0-HEAD-jre-20260701.164618-384" |
There was a problem hiding this comment.
Here is how a timestamped snapshot dependency shows: with the timestamp as part of the version
50422f3 to
370e322
Compare
370e322 to
d556007
Compare
|
@shs96c Would appreciate a review when you have a chance |
d556007 to
62149ec
Compare
When Gradle resolves a Kotlin Multiplatform (KMP) module like com.squareup.okio:okio, the JVM variant has no jar (the actual jar lives in a separate okio-jvm module). The only file Gradle provides for the root okio coordinate is a .pom file. Commit 723d203 added an isPomPathForNonPomCoordinates check in Downloader.performDownload that short-circuits when the known path is a POM for a non-POM coordinate, returning DownloadResult with null sha256 without trying to download the actual jar from the repos. This caused okio to get null sha256 in the regenerated lockfile, making it a java_library instead of jvm_import. 723d203 passed CI because the test at that time only checked that the okio target exists (via plain 'bazel query'), not its rule kind. With null sha256, okio is java_library but still exists, so the test passed. This branch tightened the test to use 'bazel query --output=label_kind' and expect 'jvm_import rule', which exposed the null sha256 bug. The fix: instead of returning immediately, skip the POM known-path and fall through to the download loop so the real jar can be fetched from the remote repos. If the jar download succeeds, its sha256 is computed normally. If it fails (e.g. jar genuinely missing), return the POM-only result (null sha256) as before. Also adds a new test verifying that when the jar IS available on the repo, the downloader fetches it and returns a real sha256.
62149ec to
02ef750
Compare
Hi 👋
This PR adds support for snapshot dependencies in the gradle resolver. It uses the repository timestamp of the snapshot as a version. I added this information in a new field with a generic name
VersionRevision.Related: #974