Allow setting :scope via maven coords too #646
Conversation
dmivankov
requested review from
c-parsons,
cheister,
jin and
shs96c
as code owners
private/rules/pom_file.bzl
Outdated
| " <groupId>%s</groupId>\n" % unpacked.groupId, | ||
| " <artifactId>%s</artifactId>\n" % unpacked.artifactId, | ||
| " <version>%s</version>\n" % unpacked.version, | ||
| (" <type>%s</type>\n" % unpacked.type) if unpacked.type and unpacked.type != "jar" else "", |
There was a problem hiding this comment.
Does this work? I would expect: "<type>%s</type>\n" % unpacked.type if unpacked.type and unpacked.type != "jar" else "" That is, the closing bracket seems like it's in the wrong place (and it's confusing even if it's the right place :) )
There was a problem hiding this comment.
Also, the struct has a type_ field, and you're using type here, so the code won't work. I'm not sure how this managed to sneak past the CI, but that's my problem to solve :)
private/rules/pom_file.bzl
Outdated
| </dependency>""" | ||
| def _format_dep(unpacked): | ||
| return "".join([ | ||
| " <dependency>\n", |
There was a problem hiding this comment.
Indentation goes wrong here. Have you run buildifier -r -lint fix . on this PR? If not, could you please do so?
There was a problem hiding this comment.
running on whole repo creates big diff, so applying it only to this file for now
private/rules/pom_file.bzl
Outdated
| </dependency>""" | ||
| def _unpack_coordinates(coords): | ||
| """Takes a maven coordinate and unpacks it into a struct with fields | ||
| groupId, artifactId, version, type_, scope |
There was a problem hiding this comment.
type_? A quick exploration suggests you'll be fine with type on it's own:
foo = struct(type = "bar")
print("Foo is: %s and dir is %s" % (foo, dir(foo)))prints Foo is: struct(type = "bar") and dir is ["to_json", "to_proto", "type"]
There was a problem hiding this comment.
You may want to mark those fields as code (eg. groupId rather than just groupId) so it renders more legibly in the generated docs.
There was a problem hiding this comment.
used type_ just because vim syntax highlighted it as a keyword, but maybe it works, let's try
|
trying to fix test failures with #657 |
Default scope is "compile" but it could be useful to depend on other scopes like "tests"
dmivankov
force-pushed
the
allow_scope_coords
branch
from
2b0f395 to
19bd64b
Compare
There was a problem hiding this comment.
Looks good, but this fails:
cd examples/java-export
bazel build //:example-export-pomThe output is:
ERROR: /src/rules_jvm_external/examples/java-export/BUILD:10:12: in pom_file rule //:example-export-pom:
Traceback (most recent call last):
File "/tmp/_bazel_shs/76cdb8bc63b14507f5041d8c30cccb52/external/rules_jvm_external/private/rules/pom_file.bzl", line 63, column 32, in _pom_file_impl
ctx.actions.expand_template(
Error in expand_template: got dict<string, NoneType> for 'substitutions', want dict<string, string>
ERROR: Analysis of target '//:example-export-pom' failed; build aborted: Analysis of target '//:example-export-pom' failedApparently, our test suite coverage isn't great for building poms, and I apologise for that. I think that if bazel build //... passes in examples/java-export we're good to go.
|
Thank you for testing, will try to make an update, sometime I rely on tests too much 😅 |
Default scope is "compile" but it could be useful to depend on other scopes like "tests"