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

Don't WARN when a spec matches no relevant targets. #14904

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Mar 24, 2022

This warning has its uses (e.g., if you accidentally ran on the wrong target),
but on balance, with hindsight, it seems more distracting and confusing than useful.

This is especially true with glob specs or --changed: "Run all the tests under foo/::"
can be satisfied vacuously if there are no tests under foo/::, and ditto "Run all the tests
that are affected by git changes". So it's weird to issue a warning.

In such cases we should adhere to the principle that "the empty set is not a
special case", and just let things hold vacuously.

Of course we can still ERROR when a goal absolutely needs a target in order
to make any sense at all.

[ci skip-rust]

[ci skip-build-wheels]

@benjyw
Copy link
Contributor Author

benjyw commented Mar 24, 2022

@thejcannon
Copy link
Member

package and publish seem a little more... scary to ignore the warning, as they have more important side-effects than test. I'm 👍 on test but the others I'm on the fence and would love to hear other opinions.

@benjyw
Copy link
Contributor Author

benjyw commented Mar 24, 2022

package and publish seem a little more... scary to ignore the warning, as they have more important side-effects than test. I'm 👍 on test but the others I'm on the fence and would love to hear other opinions.

I just think there should be one uniform principle here, and I'm generally on the side of not special-casing the empty set.

This is particularly true re interaction with --changed. For example, "Package all the binaries affected by this change" might be zero binaries, and that's fine!

@benjyw benjyw requested a review from jsirois March 24, 2022 18:52
@benjyw
Copy link
Contributor Author

benjyw commented Mar 24, 2022

Addresses #14873

Copy link
Member

@thejcannon thejcannon left a comment

Choose a reason for hiding this comment

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

I kind of think the behavior should switch based on if the user is using --changed or not. But in the absence of that change (which is more techincal and involved) this is good.

@Eric-Arellano
Copy link
Contributor

I'm skeptical I'll agree with this, but definitely will keep an open mind. See #14437 for the opposite direction. I still have a review queue from being out this past week and am trying to prioritize those for now - I'd appreciate waiting to merge this till I can review (hopefully by tomorrow at latest).

@kaos
Copy link
Member

kaos commented Mar 25, 2022

How about not warn if you didn't provide any specs at all, and warn otherwise. That way:

./pants test # is silent
./pants test foo/:: # warns if there are no test targets in foo

This should play nicely with --changed-since as well, as if there are no changes, it ought to behave as no specs where provided.

Potential corner case is, if there are changed files, but none of them takes part for the requested goal..

@benjyw
Copy link
Contributor Author

benjyw commented Mar 25, 2022

What if there are changes but they don't affect any tests?

@kaos
Copy link
Member

kaos commented Mar 25, 2022

What if there are changes but they don't affect any tests?

Yeah, that's exactly what I was trying to hint at, in more general terms, with if there are changed files, but none of them takes part for the requested goal

@benjyw
Copy link
Contributor Author

benjyw commented Mar 25, 2022

Also, I think the assumption that this condition is likely to reflect user error is open to debate.

Except for possibly in the case where you literally do ./pants test because you thought it meant the same as ./pants test ::, in which case we should make the former do the same as the latter, not warn...

In any case, in the general category of "user used the wrong specs" errors, this only warns about a specific subset. What if the user used the wrong specs but they happened to have a test in them. Not the one the user intended but a test nonetheless: no warning. So this feels a bit ad-hoc.

I guess the other important case this does catch is ./pants test foo/bar which you thought meant the same as ./pants test foo/bar/:: but actually meant ./pants test foo/bar:bar which is often not a test target. But in this case, again, we should just do what the user obviously expects, not employ the Socratic method...

@thejcannon
Copy link
Member

In these situations it helps me to ignore implementation and enumerate the possible user scenarios and the desired outcome. Then map the implementation to those cases.

@benjyw
Copy link
Contributor Author

benjyw commented Mar 30, 2022

@Eric-Arellano @stuhood thoughts on this?

@stuhood
Copy link
Member

stuhood commented Mar 30, 2022

I think that the semantics of the --changed flag vs explicitly specified specs are a little bit different.

Silently doing nothing when a user has asked for something explicitly is not good. When they've asked for something via indirection like --changed, they're instead saying "if X, then Y", and so "if not X", they would expect Y not to happen anyway.

I do recognize that drawing a line between the --changed options and the specs might be challenging right now, since we use multiple invokes of the engine (strangely: via something which is already marked as a @rule, but which doesn't correctly mark git uncacheable, and so which is invoked directly here). Fixing that would probably be a good pre-req for making this warning more subtle (relates to #13757).

@Eric-Arellano
Copy link
Contributor

Silently doing nothing when a user has asked for something explicitly is not good. When they've asked for something via indirection like --changed, they're instead saying "if X, then Y", and so "if not X", they would expect Y not to happen anyway.

my thoughts - i do think the warning/error is very helpful. reminder i added it after lots of confusion from users why pants wasn't doing anything. the autogenerated snippet of what to run is important too

i agree changed since is confusimg, but that saying about babys and bath water hah

@benjyw
Copy link
Contributor Author

benjyw commented Mar 30, 2022

I think that the semantics of the --changed flag vs explicitly specified specs are a little bit different.

Silently doing nothing when a user has asked for something explicitly is not good. When they've asked for something via indirection like --changed, they're instead saying "if X, then Y", and so "if not X", they would expect Y not to happen anyway.

Arguably this is true for glob specs as well. ./pants test path/to/something:: probably means "if there are tests under that path, test them".

@stuhood
Copy link
Member

stuhood commented Mar 30, 2022

I think that the semantics of the --changed flag vs explicitly specified specs are a little bit different.
Silently doing nothing when a user has asked for something explicitly is not good. When they've asked for something via indirection like --changed, they're instead saying "if X, then Y", and so "if not X", they would expect Y not to happen anyway.

Arguably this is true for glob specs as well. ./pants test path/to/something:: probably means "if there are tests under that path, test them".

I agree somewhat. And that reminded me that we have done this all before! #8402.

@Eric-Arellano
Copy link
Contributor

I care the most about the case where you don't give specs and we expect them. i also generally think test dir:: should warn still, but am less strong on that. --changed-since should not warn

@benjyw
Copy link
Contributor Author

benjyw commented Mar 30, 2022

I agree somewhat. And that reminded me that we have done this all before! #8402.

Oh yeah! I agree that non-globs, non --changed, should warn.

In these cases, the user intent is likely "if there are relevant
targets, act on them", so warning is just annoying.

However we still want to retain the warning if the user provided
literal addresses, filesystem specs, or no specs at all.

[ci skip-rust]

[ci skip-build-wheels]
@benjyw benjyw force-pushed the no_empty_fieldsets_warning branch from 4c9a824 to 9ddf6bb Compare April 3, 2022 00:51
@benjyw
Copy link
Contributor Author

benjyw commented Apr 3, 2022

OK, fixed this to not warn only for --changed and :: globs. PTAL.

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Great :)

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Beautiful! Yay!

@benjyw benjyw merged commit e47a627 into pantsbuild:main Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants