Implement the new artifacts hashing algorithm #1280
Conversation
shs96c
force-pushed
the
new-artifacts-hash
branch
from
4bf9b05 to
1526bbe
Compare
shs96c
force-pushed
the
new-artifacts-hash
branch
2 times, most recently
from
375945f to
c0b40e3
Compare
shs96c
force-pushed
the
new-artifacts-hash
branch
from
c0b40e3 to
26571a7
Compare
private/lib/coordinates.bzl
Outdated
| @@ -57,17 +57,30 @@ def unpack_coordinates(coords): | |||
| def _is_version_number(part): | |||
| return part[0].isdigit() | |||
|
|
|||
| def _unpack_if_necssary(coords): | |||
| @@ -45,7 +45,7 @@ public void shouldRenderAggregatingJarsAsJarWithNullShasum() { | |||
| Set.of(), | |||
| new TreeMap<>()); | |||
|
|
|||
| Map<String, Object> rendered = new V2LockFile(repos, Set.of(aggregator), Set.of()).render(); | |||
| import java.util.TreeSet; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class ArtifactsHash { |
There was a problem hiding this comment.
Done. Also added copyright notice.
| for (DependencyInfo info : infos) { | ||
| StringBuilder line = new StringBuilder(); | ||
| line.append(info.getCoordinates().toString()) | ||
| .append(" | ") |
| // utility class | ||
| } | ||
|
|
||
| public static int calculateArtifactsHash(Collection<DependencyInfo> infos) { |
There was a problem hiding this comment.
Does this format/spec need to be versioned?
There was a problem hiding this comment.
I don't think so: we're working on the DependencyInfo at this point, and we can reconstruct that from either lock file format.
| } | ||
|
|
||
| groupId = Objects.requireNonNull(parts[0]); | ||
| artifactId = Objects.requireNonNull(parts[1]); | ||
|
|
||
| boolean isGradle = | ||
| coordinates.contains("@") |
There was a problem hiding this comment.
can non-gradle coordinates contain @?
There was a problem hiding this comment.
Theoretically, a non-gradle coordinate can contain an @ character, but the Maven folks recommend against it: https://maven.apache.org/guides/mini/guide-naming-conventions.html
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.
| } | ||
|
|
||
| groupId = Objects.requireNonNull(parts[0]); | ||
| artifactId = Objects.requireNonNull(parts[1]); | ||
|
|
||
| boolean isGradle = |
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.
There was a problem hiding this comment.
Done. I have added a little white-lie about how we handle a:b:c, but I'll create a follow up PR to make that true.
| artifacts = _get_artifacts(lock_file_contents) | ||
|
|
||
| for artifact in artifacts: | ||
| line = "%s | %s | " % (to_external_form(artifact["coordinates"]), artifact["sha256"] if artifact["sha256"] else "") |
There was a problem hiding this comment.
Probably want to mention that the format should also be updated in tandem with ArtifactsHash.java.
There was a problem hiding this comment.
Done. Also added a very similar comment to ArtifactsHash.java
|
Also please update the PR description with more details on why this is happening. |
shs96c
force-pushed
the
new-artifacts-hash
branch
from
47b1557 to
2df9618
Compare
No description provided.