Expose list of all artifacts, including transitive #648

dmivankov · 2022-01-04T12:58:08Z

And add support for :type and :classifier in defs.bzl:artifact() by internally
using strip_packaging_and_classifier_and_version. To keep support for groupId:artifactId
update strip_packaging_and_classifier_and_version implementation for that case.

To be used like

    load("@maven//:defs.bzl", "maven_artifacts")

and for example

    load("@rules_jvm_external//:defs.bzl", "artifact")
    load("@rules_jvm_external//:specs.bzl", "parse")

    all_maven_targets = [artifact(a) for a in maven_artifacts if not parse.parse_maven_coordinate(a).get("classifier", "compile") in ["sources", "native"]]

    java_export(
        name = "maven-transitive-deps-lib",
        maven_coordinates = "com.example.deps:%s:%s" % (name, "0.1"),
        visibility = ["//visibility:public"],
        runtime_deps = all_maven_targets,
    )

which makes "maven-transitive-deps-lib-pom" target available that
can be useful for other tools like security scanners and/or dependabot

…ependencies to some scopes Note that using package default_visibility doesn't always work so explicit visiblity attributes are needed bazelbuild/bazel#13681 Somewhat related to bazelbuild#648 In combination this for example allows to create a class_name -> maven artifact resolver for transitive dependency classes while still keeping strict visibility (making transitive dependencies visible only to the resolve cli tool)

shs96c

I'm not sure what the utility of exposing every single maven dependency is. For code scanning, it seems like it would be more targetted to override the macros generating deployable artifacts to walk the dependencies (possibly using the has_maven_deps aspect?) Out of process tools, such as dependabot, can already do something like bazel query @maven//:all to discover all the maven artifacts.

I'm reviewing even though I don't understand the why of this PR.

coursier.bzl

dmivankov · 2022-01-05T10:40:10Z

For github dependabot script execution isn't allowed but it has support for checked in pom.xml files (and it doesn't see transitive dependencies there as different projects may potentially resolve them differently), this file can be maintained by having java_export for transitive dependencies and diff_test between checked in file and java_export pom

Second case where it can be used is writing tools that depend on both dependencies metadata and jars, with dependencies info exported into starlark one can write such tools in bazel. One example is being able to list all (classname, maven dependency) pairs.

…ependencies to some scopes Note that using package default_visibility doesn't always work so explicit visiblity attributes are needed bazelbuild/bazel#13681 Somewhat related to bazelbuild#648 In combination this for example allows to create a class_name -> maven artifact resolver for transitive dependency classes while still keeping strict visibility (making transitive dependencies visible only to the resolve cli tool)

shs96c

One thing which I think our users will bump into quickly, then this is good to go. Thank you for the PR!

coursier.bzl

dmivankov · 2022-01-17T18:06:52Z

completely forgot to update this, let me maybe try exposing richer artifact information to avoid hardcoding the handling of sources and javadocs

…ependencies to some scopes Note that using package default_visibility doesn't always work so explicit visiblity attributes are needed bazelbuild/bazel#13681 Somewhat related to bazelbuild#648 In combination this for example allows to create a class_name -> maven artifact resolver for transitive dependency classes while still keeping strict visibility (making transitive dependencies visible only to the resolve cli tool)

shs96c

I can see why this would be a useful feature. Thank you for explaining.

shs96c · 2022-01-18T11:53:28Z

coursier.bzl

@@ -497,6 +499,17 @@ def _pinned_coursier_fetch_impl(repository_ctx):

    http_files.extend(_get_jq_http_files())

+    http_files.extend([
+        "maven_artifacts = [%s]" % (",".join([


This structure doesn't match the expected usage. If you tried to add these maven_artifacts to a target's runtime_deps, the build will fail, as the deps are expected to be labels and not structs.

I think there's a couple of ways of handling this:

Document this clearly in the README.md and explain how people could use it.

Use the maven coordinates directly, once they've been converted back from the dict they are to a string, and use the artifact macro to ensure they're loaded properly.

The first option is a lot less work, but I think the second one makes the output more usable.

one slight complication with coords is that artifact macro only works with "groupId:artifactId" and that there's strip classifier logic, need some work to extend artifact macro

for example for com.github.jnr:jffi:jar:native:1.3.1, com.github.jnr:jffi:jar:sources:1.3.1 and com.github.jnr:jffi:1.3.1 there are following targets

@maven//:com_github_jnr_jffi_native_1_3_1 // alias @maven//:com_github_jnr_jffi_native // actual import @maven//:com_github_jnr_jffi_jar_sources_1_3_1_extension @maven//:com_github_jnr_jffi_jar_native_1_3_1_extension @maven//:com_github_jnr_jffi_1_3_1_extension @maven//:com_github_jnr_jffi_1_3_1 // alias @maven//:com_github_jnr_jffi // import

so artifact("com.github.jnr:jffi:jar:native:1.3.1") needs to also strip jar classifier it seems

The group and artifact IDs are the first two fields in the maven coordinates, which makes life a bit easier. I think you'd need to gather together all the imports, transform, and then sort, removing duplicates, and you'd be okay.

Currenty I can put com.github.jnr:jffi:jar:native:1.3.1 and com.github.jnr:jffi:1.3.1 as artifacts in maven_install and then refer to them as @maven//:com_github_jnr_jffi_native and @maven//:com_github_jnr_jffi.

But there seems to be no ready-made macro yet that'd transform string "com.github.jnr:jffi:jar:native:1.3.1" to label @maven//:com_github_jnr_jffi_native

load("@rules_jvm_external//:defs.bzl", "artifact") load("@rules_jvm_external//:specs.bzl", "parse") s = "com.github.jnr:jffi:jar:native:1.3.1" artifact(s) ==> @maven//:com_github_jnr_jffi # without _native part parse.parse_maven_coordinate(s) ==> { group: "com.github.jnr", artifact: "jffi", classifier: "native", packaging: "jar", version: "1.3.1" } artifact(parse.parse_maven_coordinate(s)) ==> still @maven//:com_github_jnr_jffi

I could decypher and replicate dict -> label conversion (aka escape(strip_packaging_and_classifier_and_version(s))), but maybe instead artifact macro should be extended to handle all potential keys (it already can accept dict, but decides to only use group and artifact)

Sounds quite doable, but maybe there's an even better way?

Update PR with support for full string coords in artifact macro

…ependencies to some scopes (#649) Note that using package default_visibility doesn't always work so explicit visiblity attributes are needed bazelbuild/bazel#13681 Somewhat related to #648 In combination this for example allows to create a class_name -> maven artifact resolver for transitive dependency classes while still keeping strict visibility (making transitive dependencies visible only to the resolve cli tool)

private/dependency_tree_parser.bzl

dmivankov · 2022-01-21T16:14:13Z

private/rules/artifact.bzl

-        return parse.parse_maven_coordinate(artifact_str)
+def _ensure_version(a):
+    # fake :version part is due to get_simple_coord not supporting unversioned coords
+    return a if len(a.split(":")) > 2 else (a + ":fake_version")


ugly hack, want to reuse get_simple_coord logic with support for type and classifier, but it doesn't support unversioned artifacts, also only works on strings

I don't understand why you need all of this? Is it so you can keep the one call to get_simple_coord ? Why can't this logic be put in get_simple_coord instead?

it can, but it may cause breaking changes to other use sites of that function

did a quick look, looks like this should be safe so moved there now

private/rules/artifact.bzl

cheister · 2022-01-23T18:56:00Z

private/coursier_utilities.bzl

@@ -27,7 +27,7 @@ SUPPORTED_PACKAGING_TYPES = [
    "scala-jar",
 ]

-def strip_packaging_and_classifier(coord):
+def get_versioned_coord(coord):


Why these name changes? Feels like the new names are not as clear.

can revert renaming. New names describe the purpose and applicability (maybe even better would be get_versioned_label_name and get_simple_label_name? Didn't do that initially because the jump to labels requires an extra escape on top and there are places where escape isn't applied) and it makes it more clear why they can be reused to parse coord strings into labels, while old ones describe implementation (and applicability but for cases where we have purpose in terms of implementation / action on coord parts rather than in terms of ways to address one and the same artifact)

reverted the rename. The only place without escape is around checking against jetify_include_list which could be shifted to operate on labels rather than coords, and it may add support for classifier to jetify_include_list, but that can be a separate PR if needed at all

thanks, the diff is easier to follow without the renames.

And add support for :type and :classifier in defs.bzl:artifact() by internally using strip_packaging_and_classifier_and_version. To keep support for groupId:artifactId update strip_packaging_and_classifier_and_version implementation for that case. To be used like load("@maven//:defs.bzl", "maven_artifacts") and for example load("@rules_jvm_external//:defs.bzl", "artifact") load("@rules_jvm_external//:specs.bzl", "parse") all_maven_targets = [artifact(a) for a in maven_artifacts if not parse.parse_maven_coordinate(a).get("classifier", "compile") in ["sources", "native"]] java_export( name = "maven-transitive-deps-lib", maven_coordinates = "com.example.deps:%s:%s" % (name, "0.1"), visibility = ["//visibility:public"], runtime_deps = all_maven_targets, ) which makes "maven-transitive-deps-lib-pom" target available that can be useful for other tools like security scanners and/or dependabot

dmivankov · 2022-01-24T09:26:31Z

squashed and updated to remove renames and expose just the coords, making sure artifact macro now can handle coords with classifiers. Diff got smaller now

cheister · 2022-01-24T21:43:43Z

private/coursier_utilities.bzl

@@ -46,6 +46,10 @@ def strip_packaging_and_classifier(coord):
    return ":".join(coordinates)

 def strip_packaging_and_classifier_and_version(coord):
+    coordinates = coord.split(":")


Do you mind adding a test to tests/unit/coursier_utilities_test.bzl for this?

added a test case now

jin

Given that this is a user-accessible feature, please document it in the README (or nobody will know it exists)

Also document strict_visibility_value from bazelbuild#649

dmivankov · 2022-01-26T11:10:10Z

added few new lines to readme

shs96c

Thank you for being so patient with this PR, and thank you for documenting the new feature, @dmivankov. LGTM! :)

The README has been updated as requested

dmivankov requested review from c-parsons, cheister, jin and shs96c as code owners January 4, 2022 12:58

dmivankov mentioned this pull request Jan 4, 2022

Add new strict_visibility_value= attribute to allow exposing strict dependencies to some scopes #649

Merged

shs96c requested changes Jan 5, 2022

View reviewed changes

coursier.bzl Outdated Show resolved Hide resolved

shs96c requested changes Jan 17, 2022

View reviewed changes

coursier.bzl Outdated Show resolved Hide resolved

dmivankov requested a review from shs96c January 17, 2022 18:49

dmivankov force-pushed the expose_all_artifacts_list branch from 5cb1632 to 960a32f Compare January 18, 2022 08:09

shs96c requested changes Jan 18, 2022

View reviewed changes

shs96c mentioned this pull request Jan 19, 2022

Run buildifier. No logical changes #658

Merged

dmivankov commented Jan 21, 2022

View reviewed changes

private/dependency_tree_parser.bzl Outdated Show resolved Hide resolved

dmivankov commented Jan 21, 2022

View reviewed changes

private/rules/artifact.bzl Outdated Show resolved Hide resolved

dmivankov requested a review from shs96c January 21, 2022 16:18

cheister reviewed Jan 23, 2022

View reviewed changes

dmivankov force-pushed the expose_all_artifacts_list branch from 05adb93 to 0ce871c Compare January 24, 2022 09:05

dmivankov added 2 commits January 24, 2022 10:20

handle full coord dict case in artifact() macro

f3eb06a

dmivankov force-pushed the expose_all_artifacts_list branch from 7640996 to f3eb06a Compare January 24, 2022 09:20

dmivankov requested a review from cheister January 24, 2022 09:25

cheister reviewed Jan 24, 2022

View reviewed changes

Add versionless test case for strip_packaging_and_classifier_and_version

bcdef21

dmivankov requested a review from cheister January 25, 2022 10:14

cheister approved these changes Jan 25, 2022

View reviewed changes

jin previously requested changes Jan 26, 2022

View reviewed changes

dmivankov force-pushed the expose_all_artifacts_list branch from 2e9e817 to 9c577c2 Compare January 26, 2022 10:55

Document artifact macro and dependencies list in README

8b82fe1

Also document strict_visibility_value from bazelbuild#649

dmivankov force-pushed the expose_all_artifacts_list branch from 9c577c2 to 8b82fe1 Compare January 26, 2022 10:56

dmivankov requested a review from jin January 26, 2022 10:57

shs96c approved these changes Jan 27, 2022

View reviewed changes

shs96c merged commit 2754341 into bazelbuild:master Jan 27, 2022

tsawada mentioned this pull request Mar 24, 2022

java_export to inherit testonly on its sub-targets #674

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose list of all artifacts, including transitive #648

Expose list of all artifacts, including transitive #648

dmivankov commented Jan 4, 2022 •

edited

Loading

shs96c left a comment

dmivankov commented Jan 5, 2022

shs96c left a comment

dmivankov commented Jan 17, 2022

shs96c left a comment

shs96c Jan 18, 2022

dmivankov Jan 19, 2022

shs96c Jan 19, 2022

dmivankov Jan 19, 2022

dmivankov Jan 19, 2022

dmivankov Jan 21, 2022

dmivankov Jan 21, 2022

cheister Jan 23, 2022

dmivankov Jan 24, 2022

dmivankov Jan 24, 2022

cheister Jan 23, 2022

dmivankov Jan 24, 2022

dmivankov Jan 24, 2022

cheister Jan 24, 2022

dmivankov commented Jan 24, 2022

cheister Jan 24, 2022

dmivankov Jan 25, 2022

jin left a comment

dmivankov commented Jan 26, 2022

shs96c left a comment

Expose list of all artifacts, including transitive #648

Expose list of all artifacts, including transitive #648

Conversation

dmivankov commented Jan 4, 2022 • edited Loading

shs96c left a comment

Choose a reason for hiding this comment

dmivankov commented Jan 5, 2022

shs96c left a comment

Choose a reason for hiding this comment

dmivankov commented Jan 17, 2022

shs96c left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmivankov commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jin left a comment

Choose a reason for hiding this comment

dmivankov commented Jan 26, 2022

shs96c left a comment

Choose a reason for hiding this comment

dmivankov commented Jan 4, 2022 •

edited

Loading