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

Adds excludes field to jvm_artifact targets #14715

Merged

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Mar 7, 2022

Adds exclude_dependencies, 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.

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]
@chrisjrn chrisjrn marked this pull request as draft March 7, 2022 17:52
@chrisjrn
Copy link
Contributor Author

chrisjrn commented Mar 7, 2022

It doesn't look like we need to amend coursier_fetch_single_coord, since that only fetches items that come from a resolved lockfile, and the excluded artifacts are not going to be in that resolved lockfile.

Please correct me if this is not actually correct.

"This does not prevent this artifact from being included in the resolve as a dependency "
"of other artifacts that depend on it, and is currently intended as a way to resolve "
"version conflicts in complex resolves.\n\n"
"These values are passed directly to Coursier, and if specified incorrectly will show a "
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be possible to do some rudimentary sanity checking of this value, if defined, but given we don't actually implement the parse algorithm ourselves, there are limits here.

The Coursier error message is actually quite bad when parses fail (you get java.lang.String@0xetc representations instead of the actual string values in the exception logs). I'll report (and patch?!) that upstream, where we can probably get a more reasonable error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -320,9 +353,12 @@ async def prepare_coursier_resolve_info(

to_resolve = chain(no_jars, resolvable_jar_requirements)

digest = await Get(Digest, MergeDigests([jar_files.snapshot.digest, excludes_digest]))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excludes_digest is usually empty, I'm not sure if it's worth making this merge conditional for performance, but the code is cleaner if we always run, even in no-op cases.

Christopher Neugebauer added 2 commits March 7, 2022 10:00
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
…to None.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn requested a review from stuhood March 7, 2022 18:10
@chrisjrn chrisjrn marked this pull request as ready for review March 7, 2022 18:11
@chrisjrn chrisjrn marked this pull request as draft March 7, 2022 18:11
@chrisjrn chrisjrn marked this pull request as ready for review March 7, 2022 18:11
@chrisjrn chrisjrn requested a review from tdyas March 7, 2022 18:11
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@@ -168,6 +168,19 @@ class JvmProvidesTypesField(StringSequenceField):
)


class JvmArtifactExcludeDependenciesField(StringSequenceField):
alias = "exclude_dependencies"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This equivalent field in Pants v1 was called excludes. exclude_dependencies seems wordy. Any reason to not use the same name as from v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it wasn't immediately clear to me what we would be excluding, but if we're happy with excludes we can go with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I've made that change)

Christopher Neugebauer added 2 commits March 7, 2022 10:27
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn changed the title Adds exclude_dependencies field to jvm_artifact targets Adds excludes field to jvm_artifact targets Mar 7, 2022
@chrisjrn chrisjrn requested a review from tdyas March 7, 2022 18:41
Christopher Neugebauer added 2 commits March 7, 2022 10:51
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@chrisjrn chrisjrn merged commit 16306ed into pantsbuild:main Mar 7, 2022
@chrisjrn chrisjrn deleted the chrisjrn/13686-jvm-artifact-excludes branch March 7, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow excluding transitive jars from jvm_artifact targets
3 participants