handle transitive dependencies of packaging=pom#1207
Merged
Conversation
mattnworb
requested review from
Wyverald,
cheister,
jin and
shs96c
as code owners
This comment was marked as outdated.
This comment was marked as outdated.
mattnworb
force-pushed
the
mattbrown/issue-1206
branch
2 times, most recently
from
53805be to
6d33611
Compare
closes bazel-contrib#1206 Maven interprets a `<dependency>` (not in the `<dependencyManagement>` section) with `type=pom` as adding all of those artifact's dependencies (direct and transitive) to the current module's list of dependencies. This commit modifies RJE to replicate that behavior, so that any artifact which is directly referenced in `maven_install()` also has all of its true transitive dependencies imported/installed into the Bazel workspace. The fix here is to append `pom` to the list of types passed to coursier's `--artifact-types` flag so that it properly fetches/resolves the type=pom artifact, and to append a condition to the if-else branch meant to support direct dependencies with type=pom in `maven_install()` (see bazel-contrib#99) to handle a transitive dependency on type=pom as well.
mattnworb
force-pushed
the
mattbrown/issue-1206
branch
from
6d33611 to
e9d41fe
Compare
Contributor
Author
|
This should be ready for review now. I'm not well-versed in testing strategies for rulesets so I tried to mimic what was going on with other tests, and would appreciate any feedback on how to better follow the practices for other tests (for example I couldn't decide if the |
shs96c
approved these changes
Aug 27, 2024
Collaborator
shs96c
left a comment
shs96c
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the patch, and for your patience with the review!
| artifacts = [ | ||
| # https://github.com/quarkiverse/quarkus-moneta/blob/2.0.0/runtime/pom.xml#L16-L21 | ||
| "io.quarkiverse.moneta:quarkus-moneta:2.0.0", | ||
| ], |
Collaborator
There was a problem hiding this comment.
Can you please add a lock file for this?
airlock-confluentinc Bot
pushed a commit
to confluentinc/rules_jvm_external
that referenced
this pull request
Sep 12, 2024
nlou9
added a commit
to confluentinc/rules_jvm_external
that referenced
this pull request
Sep 12, 2024
Revert "handle transitive dependencies of packaging=pom (bazel-contrib#1207)" (#4)
airlock-confluentinc Bot
pushed a commit
to confluentinc/rules_jvm_external
that referenced
this pull request
Sep 19, 2024
…l-contrib#1207)" (#4)" This reverts commit 2431a06.
Contributor
|
This PR seemingly introduced a regression that did not surface until the prebuit jars were rebuilt as part of the 6.8 release: #1477 |
mattnworb
added a commit
to mattnworb/rules_jvm_external
that referenced
this pull request
Nov 17, 2025
this was a request brought up in bazel-contrib#1207 that was not addressed before it was merged
mattnworb
added a commit
to mattnworb/rules_jvm_external
that referenced
this pull request
Nov 17, 2025
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)
shs96c
pushed a commit
that referenced
this pull request
Nov 19, 2025
…#1479) 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 #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 #1477 [0]: https://central.sonatype.com/artifact/org.opencadc/cadc-util/1.12.10 [1]: #1477 (comment)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
closes #1206
this seems like a relatively easy fix in terms of number of lines of code changed, but I can't tell if it has implications on the contents of the lockfile - when I first ran
bazel test//..I got errors for:but then running the repin command resolved the errors and the test pass, but I don't see any modifications in my checkout related to
regression_testing_maven_install.json.