coursier: ignore dependencies with classifier="sources" and no "file"#1479
Conversation
this was a request brought up in bazel-contrib#1207 that was not addressed before it was merged
when coursier is asked to resolve an artifact that has a transitive
dependency on `org.apache.logging.log4j:log4j:3.0.0-beta3` (note the
lack of packaging in the [real-world example here][0]) and
`fetch_sources=True` is set, coursier will return this in the list of
dependencies:
```
{
"coord": "org.apache.logging.log4j:log4j:jar:sources:3.0.0-beta3",
"file": null,
"directDependencies": [],
"dependencies": []
}
```
in rules_jvm_external 6.8 this will cause errors when building the
external repo generated by RJE since RJE will end up handling this
dependency by a) generating a `http_file` with an empty list of `urls`
and b) emitting a `copy_file` rule in the external repo's BUILD file
that refers to the non-existing `http_file` repo from A. See
[this comment][1] for a breakdown of why this happens.
PR bazel-contrib#1207 added `pom` to the list of `SUPPORTED_PACKAGING_TYPES` so that
the dependencies of the pom could be aggregated (Maven interprets a
dependency on an artifact with packaging=pom as depending on the
`<dependencies>` in that pom), but that PR didn't test what happens with
`fetch_sources=True` nor did it consider the case like with
`org.apache.logging.log4j:log4j:3.0.0-beta3` where the coordinates
output by coursier don't mention the packaging at all.
fixes bazel-contrib#1477
[0]: https://central.sonatype.com/artifact/org.opencadc/cadc-util/1.12.10
[1]: bazel-contrib#1477 (comment)
| "org.opencadc:cadc-util:1.12.10", | ||
| ], | ||
| fetch_sources = True, | ||
| lock_file = "//tests/custom_maven_install:transitive_dependency_with_type_of_pom.json", |
There was a problem hiding this comment.
this is following up on https://github.com/bazel-contrib/rules_jvm_external/pull/1207/files#r1733224922
|
|
||
| def rewrite_files_attribute_if_necessary(repository_ctx, dep_tree): | ||
| # There are cases where `coursier` will download both the pom and the | ||
| def filter_dependencies_if_necessary(repository_ctx, dep_tree): |
There was a problem hiding this comment.
repurposed this existing function to filter out more cases rather than adding yet another function that iterates over dep_tree["dependencies"]
shs96c
left a comment
shs96c
left a comment
There was a problem hiding this comment.
Thank you for handling this. One open question, and then I'll land with glee!
|
|
||
| # Pinned repo | ||
| "transitive_dependency_with_type_of_pom", | ||
| "unpinned_transitive_dependency_with_type_of_pom", |
There was a problem hiding this comment.
Nit: we no longer need the unpinned_ variant since //transitive_dependency_with_type_of_pom:pin will work
There was a problem hiding this comment.
would you want it to be removed from here? I figured it made sense to follow the pattern for other test cases
There was a problem hiding this comment.
I should go through and remove more, but I guess if we add this it'll be one more line :)
| for dep in dep_tree["dependencies"]: | ||
| if not dep.get("file", None): | ||
| amended_deps.append(dep) | ||
| if get_classifier(dep["coord"]) == "sources": |
There was a problem hiding this comment.
Do we also need to be concerned about the javadocs classifier too?
There was a problem hiding this comment.
Good question, and to be honest I am not sure - I suppose it would depend on if coursier could be made to return dependencies for packaging=jar classifier=javadoc with "file": null - I would think if an artifact with classifier=javadoc was requested and doesn't exist, it would return an error instead?
There was a problem hiding this comment.
I ask because the javadocs jars follow the same pattern as the source jars when we try and download them. I guess we can wait until someone files an issue....
shs96c
left a comment
shs96c
left a comment
There was a problem hiding this comment.
LGTM! Let's land this thing :)
| for dep in dep_tree["dependencies"]: | ||
| if not dep.get("file", None): | ||
| amended_deps.append(dep) | ||
| if get_classifier(dep["coord"]) == "sources": |
There was a problem hiding this comment.
I ask because the javadocs jars follow the same pattern as the source jars when we try and download them. I guess we can wait until someone files an issue....
|
|
||
| # Pinned repo | ||
| "transitive_dependency_with_type_of_pom", | ||
| "unpinned_transitive_dependency_with_type_of_pom", |
There was a problem hiding this comment.
I should go through and remove more, but I guess if we add this it'll be one more line :)
|
Hi @shs96c, do you know if this will be released soon? We're evaluating whether we should upgrade to 6.9 and add this patch, or wait for the 6.10 release. Thanks :) |
3 participants
when coursier is asked to resolve an artifact that has a transitive dependency on
org.apache.logging.log4j:log4j:3.0.0-beta3(note the lack of packaging in the real-world example here) andfetch_sources=Trueis set, coursier will return this in the list of dependencies:in rules_jvm_external 6.8 this will cause errors when building the external repo generated by RJE since RJE will end up handling this dependency by a) generating a
http_filewith an empty list ofurlsand b) emitting acopy_filerule in the external repo's BUILD file that refers to the non-existinghttp_filerepo from A. See this comment for a breakdown of why this happens.PR #1207 added
pomto the list ofSUPPORTED_PACKAGING_TYPESso that the dependencies of the pom could be aggregated (Maven interprets a dependency on an artifact with packaging=pom as depending on the<dependencies>in that pom), but that PR didn't test what happens withfetch_sources=Truenor did it consider the case like withorg.apache.logging.log4j:log4j:3.0.0-beta3where the coordinates output by coursier don't mention the packaging at all.fixes #1477