-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
[internal] Add compatible_resolves
to jvm_artifact
target
#13893
[internal] Add compatible_resolves
to jvm_artifact
target
#13893
Conversation
# 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]
if not tgt.has_field(JvmArtifactCompatibleResolvesField): | ||
continue | ||
# TODO: add a `default_compatible_resolves` field. | ||
# TODO: error if no resolves for the target? Which would mean it's practically invisible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a bug waiting to happen. I think we should error.
FYI the fix will be to either make sure [jvm].default_compatible_resolves
is set or to manually set it for the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pants/src/python/pants/jvm/subsystems.py
Lines 23 to 29 in 18350c5
register( | |
"--default-resolve", | |
type=str, | |
# TODO: Default this to something like `jvm-default`. | |
default=None, | |
help="The name of the resolve to use by default.", | |
) |
And as previously mentioned, I think that we should set an actual default value there.
But yes: no resolves should be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already using --default-resolve
if you look at the next line.
I think that we should set an actual default value there.
Agreed, waiting to do in a followup.
CoursierResolvedLockfile, ArtifactRequirements, artifact_requirements | ||
CoursierResolvedLockfile, | ||
ArtifactRequirements, | ||
# TODO: error if no artifacts associated with a resolve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably should allow it? You'd have an empty lockfile generated, which is a little weird but also legal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least, it is common in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should allow it. We have in pantsbuild/pants a jvm_testprojects
resolve. Currently, it's unused, but it's convenient we have it defined already in pants.toml
so that we can add to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should allow it. We have in pantsbuild/pants a
jvm_testprojects
resolve. Currently, it's unused, but it's convenient we have it defined already inpants.toml
so that we can add to it.
It's the default resolve, so it's not unused... it's used for the code under testprojects/src/jvm
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't use any jvm_artifact
s. Note that the lockfile has always been empty, and remains empty with this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sure. Not unused, just empty.
) | ||
|
||
|
||
@maybe_skip_jdk_test | ||
def test_coursier_resolve_noop_does_not_touch_lockfile(rule_runner: RuleRunner) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes, just DRY
return rule_runner | ||
|
||
|
||
@maybe_skip_jdk_test | ||
def test_coursier_resolve_creates_missing_lockfile(rule_runner: RuleRunner) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No changes, just DRY
|
||
|
||
@maybe_skip_jdk_test | ||
def test_updates_lockfile(rule_runner: RuleRunner) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the test right below. No changes, just DRY
|
||
@maybe_skip_jdk_test | ||
def test_coursier_resolve_updates_bogus_lockfile(rule_runner: RuleRunner) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted because this test doesn't seem like it's pulling its weight
@@ -154,6 +154,21 @@ class JvmProvidesTypesField(StringSequenceField): | |||
) | |||
|
|||
|
|||
class JvmArtifactCompatibleResolvesField(JvmCompatibleResolvesField): | |||
help = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing is hard...feedback welcomed for both these new help messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it sortof feels like rather than customizing the help for use with JvmArtifact
, you could expand the help for JvmCompatibleResolvesField
to indicate that it applies across all targets, and defines a universe of code that uses a particular thirdparty resolve... i.e., move some of the help you've added here onto the existing field instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to update JvmCompatibleResolvesField
in a followup, but I do think it's valuable for these to diverge. In particular, the behavior is different for how we validate that the entire transitive closure is compatible with each other. For jvm_artifact
, it by definition is the leaf(?) of the closure as it does not have the dependencies
field. So it doesn't make sense to talk about validating its dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm. Maybe. But I think that similar to interpreter_constraints
, you need to understand the big picture before you can successfully set the field anywhere. Splitting the big picture up across multiple fields makes that a bit harder. But there might be a few details that are only relevant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we should do a comprehensive audit of help
messages for JVM at the same time we write the docs for it. (I like writing docs, so I can do that when the time is right if you'd like)
# 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]
The next followup will add Reminder that we are still missing validation that the resolve chosen for consumption is valid for the entire transitive closure, per #13876. That will be a followup to this all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Before landing this, there is a thing to fix: the pantsbuild/pants
repo uses three different resolves. Because we don't have JVM lockfile staleness checks, these changes didn't cause check
to fail: but if you regenerate the lockfiles for the repo right now, it will likely cause some code to fail to build. You'll need to actually use the new field in a few places under https://github.com/pantsbuild/pants/tree/main/3rdparty/jvm to make the resolves accurate again.
CoursierResolvedLockfile, ArtifactRequirements, artifact_requirements | ||
CoursierResolvedLockfile, | ||
ArtifactRequirements, | ||
# TODO: error if no artifacts associated with a resolve? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least, it is common in tests.
@@ -154,6 +154,21 @@ class JvmProvidesTypesField(StringSequenceField): | |||
) | |||
|
|||
|
|||
class JvmArtifactCompatibleResolvesField(JvmCompatibleResolvesField): | |||
help = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it sortof feels like rather than customizing the help for use with JvmArtifact
, you could expand the help for JvmCompatibleResolvesField
to indicate that it applies across all targets, and defines a universe of code that uses a particular thirdparty resolve... i.e., move some of the help you've added here onto the existing field instead.
# 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]
…target-api [ci skip-rust] [ci skip-build-wheels]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Part of #13621.
As explained in #13621 (comment),
jvm_artifact
targets now declare explicitly which resolves they should be used with. Rather than where before we inferred this based on analyzing which first-party targets consumed the artifacts and worked backward by seeing the resolves those targets themselves used.Having
jvm_artifact
targets declare which resolves they use is necessary for us to update dependency inference so that you can only infer deps on artifacts that belong to your resolve. Whereas right now you can infer deps on the entire universe of artifacts. We need to know what's in a resolve before any dependency inference has happened.This change also brings clearer modeling for users, less magic. While there's more up-front work when you add a new
jvm_artifact
, it makes it much clearer what your intent is for where the resolve should be used.[ci skip-rust]
[ci skip-build-wheels]