Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow excluding transitive jars from jvm_artifact targets #13683

Closed
tdyas opened this issue Nov 19, 2021 · 11 comments · Fixed by #14715
Closed

allow excluding transitive jars from jvm_artifact targets #13683

tdyas opened this issue Nov 19, 2021 · 11 comments · Fixed by #14715
Assignees
Labels
backend: JVM JVM backend-related issues

Comments

@tdyas
Copy link
Contributor

tdyas commented Nov 19, 2021

Pants v1 supported excluding certain jars from the transitive dependency graph of a jar_libary target. The jvm_artifact target type should also support exclusions. If done in a similar manner to v1, it would be via an exclusions field on jvm_artifact.

It also supported an intransitive=True flag, which excluded all transitive dependencies.

@tdyas tdyas added the backend: JVM JVM backend-related issues label Nov 19, 2021
@Eric-Arellano
Copy link
Contributor

Can you use !! ignores in the dependencies field instead?

The dependencies toolbox at https://www.pantsbuild.org/docs/targets

@tdyas
Copy link
Contributor Author

tdyas commented Nov 19, 2021

Can you use !! ignores in the dependencies field instead?

The dependencies toolbox at https://www.pantsbuild.org/docs/targets

I'm not sure that would cover this case. Given jvm_artifact does not support dependencies, the user would have to write a jvm_artifact target and then reference it with !! on every target that pulled in the original dependency where we need that exclusion. The issue here is to filter that jar(s) from the classpath entirely with respect to the transitive dependencies of a specific dependency. The excluded jar at a different version might appear (and be wanted) to come in from another part of the transitive dependency graph. Easier to specify the exclusion on the jvm_artifact that represents the part of the dependency graph where we want the filtering.

@stuhood stuhood changed the title allow excluding transtive jars from jvm_artifact targets allow excluding transitive jars from jvm_artifact targets Dec 17, 2021
@chrisjrn chrisjrn self-assigned this Mar 3, 2022
@chrisjrn
Copy link
Contributor

chrisjrn commented Mar 3, 2022

Taking this on -- it looks like cs fetch has an excludes option which lets you specify partially qualified coordinates (i.e. group:artifact without version). Let's see what it would take to make that work.

@tdyas
Copy link
Contributor Author

tdyas commented Mar 3, 2022

I see at least two similar but slightly different use cases for exclusions:

  1. Exclude a jar from a resolve entirely.
  2. Exclude a jar that is a transitive dependency of a jvm_artifact but allow it to come in via another jvm_artifact (presumably at a different version). That is, influence the selection of jars by selectively pruning parts of the jar dependency graph. This use case may be moot with lockfiles though since the user could maybe pin the transitive jar via an explicit jvm_artifact?

(1) points at exclusions being a per-resolve item (like Scala version), while (2) points at it being a per-jvm_artifact field. But depends on how the use case in (2) should be accomplished.

@tdyas
Copy link
Contributor Author

tdyas commented Mar 3, 2022

I had the use case in (2) in the past in Pants v1 to resolve conflicting versions of netty by pruning netty from one of the jar_library v1 targets. v1 didn't have lockfiles though, so for v2, how we would handle pinning netty in such a case probably will point at how to model this.

@chrisjrn
Copy link
Contributor

chrisjrn commented Mar 3, 2022

@tdyas I'm going to aim for solving (2) initially, since it's the easier option, which should be easy enough, given that we fetch artifacts on an individual basis. We may need to think a bit harder about how to model (1).

I haven't tested coursier's behaviour if you specify a specific version to fetch, but it'd be interesting to see what happens if a higher version shows up as a transitive dependency. Let me go try that one out.

@tdyas
Copy link
Contributor Author

tdyas commented Mar 3, 2022

We fetch individually but we resolve lockfiles collectively by passing in all of the coordinates:

*coursier_resolve_info.coord_arg_strings,

Thinking "aloud", this brings up some points about when exclusion can be applied (with some regard to whether the user writes it in a jvm_artifact field or in a per-resolve exclusions option):

  1. If the exclusion is applied during lockfile resolution, it would be removed from resolve entirely unless Coursier had a way to prune for a specific coordinate. And given how we pass coordinate strings during lockfile resolution, not sure if that is the case.
  2. If the exclusion is applied when constructing a classpath, then it could influence what subset of jars to take from the resolve. Not sure if this corresponds to a real use case though. Thinking to the netty use case, if I used the jvm_artifact with the exclusion as a dependency and not the other jvm_artifact with a netty dep, then netty would be missing when instead I wanted to influence what netty version to include.

@chrisjrn
Copy link
Contributor

chrisjrn commented Mar 3, 2022

Per below, it looks like Coursier's default way to handle a complex resolve is to use the newest version of the artifact. This means we'll need to force our version of the artifact when we do the resolve (re-specifying the versions from our jvm_artifact targets as a conflict resolution preference should be sufficient here, tbqh).

We will still need to specify excludes at fetch time to ensure that the transitive dependency isn't fetched (this is actually even more important than before, since we won't have a SHA digest for the excluded dependency.

chrisjrn@chrisjrns-MacBook-Pro example-jvm % less coursier_report.json | jq .
{
  "conflict_resolution": {
    "com.fasterxml.jackson.core:jackson-core:2.11.4": "com.fasterxml.jackson.core:jackson-core:2.12.2"
  },
  "dependencies": [
    {
      "coord": "com.fasterxml.jackson.core:jackson-annotations:2.12.2",
      "file": "/Users/chrisjrn/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-annotations/2.12.2/jackson-annotations-2.12.2.jar",
      "directDependencies": [],
      "dependencies": []
    },
    {
      "coord": "com.fasterxml.jackson.core:jackson-core:2.12.2",
      "file": "/Users/chrisjrn/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-core/2.12.2/jackson-core-2.12.2.jar",
      "directDependencies": [],
      "dependencies": []
    },
    {
      "coord": "com.fasterxml.jackson.core:jackson-databind:2.12.2",
      "file": "/Users/chrisjrn/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/fasterxml/jackson/core/jackson-databind/2.12.2/jackson-databind-2.12.2.jar",
      "directDependencies": [
        "com.fasterxml.jackson.core:jackson-annotations:2.12.2",
        "com.fasterxml.jackson.core:jackson-core:2.12.2"
      ],
      "dependencies": [
        "com.fasterxml.jackson.core:jackson-annotations:2.12.2",
        "com.fasterxml.jackson.core:jackson-core:2.12.2"
      ]
    }
  ],
  "version": "0.1.0"
}

@tdyas
Copy link
Contributor Author

tdyas commented Mar 3, 2022

Might be relevant: I had to disable "strict" resolution of versions due to a bug described in #13496. Not directly on-point with exclusions but we may want to revisit strict dependency resolution again.

@chrisjrn
Copy link
Contributor

chrisjrn commented Mar 3, 2022

So for starters, which version of jackson-core should we be returning by default here?

rule_runner.request(
        CoursierResolvedLockfile,
        [
            ArtifactRequirements.from_coordinates(
                [
                    Coordinate(group="com.fasterxml.jackson.core", artifact="jackson-databind", version="2.12.1"),
                    Coordinate(group="com.fasterxml.jackson.core", artifact="jackson-core", version="2.11.1"),
                ]
            ),
        ],
    )

@stuhood
Copy link
Member

stuhood commented Mar 3, 2022

The flag used for a local exclude in v1 was:

# Local exclusions
local_exclude_args = []
for jar in jars:
for ex in jar.excludes:
# `--` means exclude. See --local-exclude-file in `coursier fetch --help`
# If ex.name does not exist, that means the whole org needs to be excluded.
ex_arg = f"{jar.org}:{jar.name}--{ex.org}:{ex.name or '*'}"
local_exclude_args.append(ex_arg)

chrisjrn pushed a commit that referenced this issue Mar 7, 2022
Adds `excludes`, which accepts unversioned coordinates to exclude as transitive dependencies of that artifact. This is passed to Coursier's `--local-excludes-file` at resolve time, so it's excluded in the lockfile. Consequently, we also add excludes to the metadata string for lockfiles.

Closes #13683.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: JVM JVM backend-related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants