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

Support strong first-party declarations of provided types #13698

Merged

Conversation

chrisjrn
Copy link
Contributor

@chrisjrn chrisjrn commented Nov 23, 2021

Adds a new field to java_source(s) and scala_source(s) targets that allow them to strongly declare that they provide a specific JVM type. This excludes these types from being provided by third-party dep inference. This allows first-party sources to provide types that amend a third-party package.

TODO: remove exampleapp amendment

Closes #13661

Christopher Neugebauer added 4 commits November 23, 2021 11:41
WIP
# 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]
WIP
# 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]
# 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 tdyas November 23, 2021 21:11
@chrisjrn
Copy link
Contributor Author

This code specifically resolves the ambiguity by having the third-party mapping return no results for provide-d types, rather than amending what actually gets stored inside the trie: in practice the first party symbol mapping was already good enough, so this makes the 3rd-party side of things quite a bit less complicated. Would be keen to hear thoughts about this.

# 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]
Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Should the field be on java_source and when the field is present on java_sources then Pants figures out how to "split" it to only the java_source target that defines the type?

Christopher Neugebauer added 3 commits November 24, 2021 07:46
# 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]
# 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
Copy link
Contributor Author

Should the field be on java_source and when the field is present on java_sources then Pants figures out how to "split" it to only the java_source target that defines the type?

If we wanted to be technically correct, then yes, I guess?

I didn't go looking particularly hard for how the first party symbol map works in relation to target generation, but I suspect we'd need the symbol map to be run first. I'm not sure that's doable. On the other hand, all that experimental_provides_types does is rule out a type from third-party inference. It doesn't affect how the first-party types are handled, so there's not really a technical need to isolate the provides_types value to a specific source.

To prevent spurious use of the field, we may want to consider verifying that all of the provides types are found inside the first-party symbol map (and that those types are correctly annotated with provides). If that makes sense to you, then that's what I'll do.

(cc @tdyas)

# 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
Copy link
Contributor Author

^--- And the above is exactly what I've done now.

Christopher Neugebauer added 2 commits November 24, 2021 08:47
# 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 requested a review from tdyas November 24, 2021 17:16
def __init__(self):
self.children: dict[str, MutableTrieNode] = {}
self.recursive: bool = False
self.addresses: OrderedSet[Address] = OrderedSet()
self.first_party: bool = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we end up having the ! syntax for packages, we may want to redefine this field for more complicated exclusions, but for now, this will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

@tdyas
Copy link
Contributor

tdyas commented Nov 24, 2021

I didn't go looking particularly hard for how the first party symbol map works in relation to target generation,

The results from first party and third party symbol matching are merged together (and then checked for ambiguity). The order in which they are evaluated does not matter.

Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Should this PR just be implemented by implementing negation support in the trie? Is there a benefit to supporting first_party as a concept in the trie now versus changing it to be implemented via general negation support later?

def __init__(self):
self.children: dict[str, MutableTrieNode] = {}
self.recursive: bool = False
self.addresses: OrderedSet[Address] = OrderedSet()
self.first_party: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

@@ -64,6 +64,11 @@ class JvmArtifactPackagesField(StringSequenceField):
)


class JvmProvidesTypesField(StringSequenceField):
alias = "experimental_provides_types"
help = "TODO: Add help for this."
Copy link
Contributor

Choose a reason for hiding this comment

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

Still needs to be filled in.

@chrisjrn
Copy link
Contributor Author

@tdyas Re supporting negation in the trie: I think we may need to map out a proper design for how to implement negation properly. I think there are actually two concepts that matter to here:

  • "This artifact does not provide this type" (i.e. the ! syntax that @stuhood mooted)
  • vs "No artifact should provide this type" (i.e. the first_party concept that we've added in this PR)

With that in mind, I'd prefer to punt on redesigning that, until we implement negation as a more general concept, so we can flesh out the design better.

@tdyas
Copy link
Contributor

tdyas commented Nov 24, 2021

With that in mind, I'd prefer to punt on redesigning that, until we implement negation as a more general concept, so we can flesh out the design better.

Makes sense. Agreed.

Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

lgtm subject to any open comments

Christopher Neugebauer added 2 commits November 24, 2021 11:46
# 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]
This reverts commit a27e44e.

[ci skip-rust]

[ci skip-build-wheels]
@chrisjrn chrisjrn merged commit 14fd529 into pantsbuild:main Nov 24, 2021
@chrisjrn chrisjrn deleted the chrisjrn/13661-first-third-party-split branch November 24, 2021 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.

[jvm] Split thirdparty/firstparty packages need finer-grained thirdparty definitions
2 participants