Implement the new artifacts hashing algorithm #1280
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,26 @@ public class Coordinates implements Comparable<Coordinates> { | |
| private final String classifier; | ||
| private final String extension; | ||
|
|
||
| /** | ||
| * Converts a `String` in one of two formats and extracts the information from it. | ||
| * | ||
| * <p>The two supported formats are: | ||
| * | ||
| * <ol> | ||
| * <li>group:artifact:version:classifier@extension | ||
| * <li>group:artifact:extension:classifier:version. | ||
| * </ol> | ||
| * | ||
| * The first of these matches Gradle's <a | ||
| * href="https://docs.gradle.org/8.11.1/dsl/org.gradle.api.artifacts.dsl.DependencyHandler.html#N1739F"> | ||
| * external dependencies</a> form, and is the preferred format to use since it matches | ||
| * expectations of users of other tools. The second format is the one used within | ||
| * `rules_jvm_external` since its inception. | ||
| * | ||
| * <p>Note that there is potential confusion when only three segments are given (that is, the | ||
| * value could be one of `group:artifact:version` or `group:artifact:extension`) In this case, we | ||
| * assume the value is `group:artifact:version` as this is far more widely used. | ||
| */ | ||
| public Coordinates(String coordinates) { | ||
| Objects.requireNonNull(coordinates, "Coordinates"); | ||
|
|
||
|
|
@@ -36,29 +56,64 @@ public Coordinates(String coordinates) { | |
| "Bad artifact coordinates " | ||
| + coordinates | ||
| + ", expected format is" | ||
| + " <groupId>:<artifactId>[:<version>][:<classifier>][@<extension>"); | ||
| } | ||
|
|
||
| groupId = Objects.requireNonNull(parts[0]); | ||
| artifactId = Objects.requireNonNull(parts[1]); | ||
|
|
||
| boolean isGradle = | ||
|
Collaborator
There was a problem hiding this comment. Please document this constructor formally on what formats are accepted. It's getting a bit harder to understand this code, and AFAIUC, we are trying to use this to canonicalize the format, so some documentation will be useful.
Collaborator
Author
There was a problem hiding this comment. Done. I have added a little white-lie about how we handle |
||
| coordinates.contains("@") | ||
|
Collaborator
There was a problem hiding this comment. can non-gradle coordinates contain
Collaborator
Author
There was a problem hiding this comment. Theoretically, a non-gradle coordinate can contain an I've never seen one in the wild, and I think it's reasonable to tell people to use the Gradle format if it's something they really need. |
||
| || (parts.length > 2 && !parts[2].isEmpty() && Character.isDigit(parts[2].charAt(0))); | ||
|
|
||
| String version = null; | ||
| String extension = "jar"; | ||
| String classifier = "jar"; | ||
|
|
||
| if (parts.length == 2) { | ||
| extension = "jar"; | ||
| classifier = ""; | ||
| version = ""; | ||
| } else if (parts.length == 5) { // Unambiguously the original format | ||
| extension = "".equals(parts[2]) ? "jar" : parts[2]; | ||
| classifier = "jar".equals(parts[3]) ? "" : parts[3]; | ||
| version = parts[4]; | ||
| } else if (parts.length == 3) { | ||
| // Could either be g:a:e or g:a:v or g:a:v@e | ||
| if (isGradle) { | ||
| classifier = ""; | ||
|
|
||
| if (parts[2].contains("@")) { | ||
| String[] subparts = parts[2].split("@", 2); | ||
| version = subparts[0]; | ||
| extension = subparts[1]; | ||
| } else { | ||
| extension = "jar"; | ||
| version = parts[2]; | ||
| } | ||
| } | ||
| } else { | ||
| // Could be either g:a:e:c or g:a:v:c or g:a:v:c@e | ||
| if (isGradle) { | ||
| version = parts[2]; | ||
| if (parts[3].contains("@")) { | ||
| String[] subparts = parts[3].split("@", 2); | ||
| classifier = subparts[0]; | ||
| extension = subparts[1]; | ||
| } else { | ||
| classifier = parts[3]; | ||
| extension = "jar"; | ||
| } | ||
| } else { | ||
| extension = parts[2]; | ||
| classifier = ""; | ||
| version = parts[3]; | ||
| } | ||
| } | ||
|
|
||
| this.version = version; | ||
| this.classifier = classifier; | ||
| this.extension = extension; | ||
| } | ||
|
|
||
| public Coordinates( | ||
|
|
@@ -103,6 +158,7 @@ public String getExtension() { | |
| return extension; | ||
| } | ||
|
|
||
| // This method matches `coordinates.bzl#to_key`. Any changes here must be matched there. | ||
| public String asKey() { | ||
| StringBuilder coords = new StringBuilder(); | ||
| coords.append(groupId).append(":").append(artifactId); | ||
|
|
@@ -155,13 +211,23 @@ public int compareTo(Coordinates o) { | |
| } | ||
|
|
||
| public String toString() { | ||
| StringBuilder builder = new StringBuilder(); | ||
|
|
||
| builder.append(getGroupId()).append(":").append(getArtifactId()); | ||
|
|
||
| if (getVersion() != null && !getVersion().isEmpty()) { | ||
| builder.append(":").append(getVersion()); | ||
| } | ||
|
|
||
| if (getClassifier() != null && !getClassifier().isEmpty() && !"jar".equals(getClassifier())) { | ||
| builder.append(":").append(getClassifier()); | ||
| } | ||
|
|
||
| if (getExtension() != null && !getExtension().isEmpty() && !"jar".equals(getExtension())) { | ||
| builder.append("@").append(getExtension()); | ||
| } | ||
|
|
||
| return builder.toString(); | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to mention that the format should also be updated in tandem with ArtifactsHash.java.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also added a very similar comment to
ArtifactsHash.java