https://pantsbuild.org/ logo
h

hundreds-father-404

10/14/2021, 5:11 PM
An open question on what we want for CLI specs: when you run
./pants test src/py
, do you expect it to only run tests in
src/py
? 🧵
Background: We have a mechanism of "target generators being alias". When you have
python_tests
in a BUILD file, that is actually generating a
python_test
target for each file in the
sources
field. Pants runs on
python_test
targets, not
python_tests
, which only exist for boilerplate reduction We allow you to refer to that
python_tests
target as an alias for all of its generated targets:
./pants test src/py:tests
will result in running all the relevant
python_test
targets, rather than erroring that the target is
python_tests
so not runnable. Same with the
dependencies
field, we'll replace the
python_tests
target with its generated
python_test
targets
h

hundreds-father-404

10/14/2021, 5:14 PM
It's an open question when you run
./pants test src/py
how to handle matching a
python_tests
target defined in
src/py
. Does the user expect this alias mechanism to apply there too? If so, that means
./pants test src/py
might actually run over dozens of directories! Depends if the
python_tests
target is generating into a bunch of subdirectories
For project introspection, I think it's uncontroversial that we should not replace the target generator with the generated target. When you run
./pants peek src/py
, we want to show both the
python_tests
target and any
python_test
targets that are generated into that directory But for "build" goals like
test
, how do we handle
src/py
matching
src/py:tests
with
sources=["**/*_test.py"]
?
w

witty-crayon-22786

10/14/2021, 5:19 PM
(as mentioned elsewhere: i think aliases are useful, and should work consistently… will leave it at that.)
h

hundreds-father-404

10/14/2021, 5:21 PM
I agree they're useful, I backtracked from my proposal in https://github.com/pantsbuild/pants/issues/13086 to get rid of aliases, mostly due to your feedback. (Thank you for that!) I also fully buy in to expanding aliases for address literals like
./pants test src/py:tests
(when it's a build goal), and with
dependencies
My hesitation is specifically whether it's useful vs. confusing to expand aliases with a directory spec
w

witty-crayon-22786

10/14/2021, 5:22 PM
it’s confusing not to, imo. if
./pants peek src/py
will show the alias,
./pants test src/py
should run it.
h

hundreds-father-404

10/14/2021, 5:23 PM
Fwit, I offer this design principle in the doc. Doesn't mean we need to agree with it, but my mental model
Pants should do what the user expects, even if that means behavior is slightly different between goals (project introspection vs. build goals).
How do we know what a user expects? That question changes the answer here for sure. Which is this other design principle I offer
Users mostly think in terms of files and directories. BUILD files / targets are usually only a means to an end.
c

curved-television-6568

10/14/2021, 5:27 PM
Does the user expect this alias mechanism to apply there too?
If I run
./pants test src/py
and have a
python_tests(sources=["**/*_test.py"])
in there, I’d expect to run the tests for all those sources.
h

hundreds-father-404

10/14/2021, 5:28 PM
Okay, thanks Andreas! (Personally, I would expect to run
./pants test src/py:tests
or
./pants test src/py::
to say "all the tests")
c

curved-television-6568

10/14/2021, 5:28 PM
Hmm… but that’s perhaps because I’m getting used to targets, rather than files now… 😛
I think it brings a bit of confusion to allow either paths OR spec addresses on the command line…
h

hundreds-father-404

10/14/2021, 5:29 PM
I learned the phrase the Curse of Knowledge only this past month: https://en.wikipedia.org/wiki/Curse_of_knowledge it's a thing I'm trying to be more aware of for me
👍 1
c

curved-television-6568

10/14/2021, 5:30 PM
I didn’t know it was as a thing, but I have that issue a lot at work… 😄
w

witty-crayon-22786

10/14/2021, 5:31 PM
another important example is `*_binary`: if i have only a
*_binary
in a directory and no files, it should match. and then not matching for target types that own files would be inconsistent
h

hundreds-father-404

10/14/2021, 5:33 PM
a bit of confusion to allow either paths OR spec addresses on the command line…
Part of https://docs.google.com/document/d/1WWQM-X6kHoSCKwItqf61NiKFWNSlpnTC5QNu3ul9RDk/edit is that address specs vs. file specs is no longer as relevant as it used to be I at first proposed
dir::
(addresses) vs
dir/...
(file semantics), then we generally agreed it's confusing for the user to have to know the nuances of
dir:
vs
dir
, and
dir::
vs
dir/...
. 95% of Pants users will likely never read the majority of ours, particularly users who aren't on our Slack and only use Pants because their org uses it. Not worth the subtly
another important example is *_binary: if i have only a *_binary in a directory
Agreed it should match. They live in that directory
w

witty-crayon-22786

10/14/2021, 5:42 PM
a related example from v1 (which is a bit of a tangent here, but bear with me) was to define a
target
alias with a short name that referred to something deeper in the directory structure, so that you could do
./pants run shortname
,
./pants test shortname
, etc. for that to work, it’s necessary to expand the alias… but because we didn’t have a native facility for that, it got hacked into a bunch of goals
c

curved-television-6568

10/14/2021, 5:43 PM
Hmm… perhaps this have been considered and discarded before, but given how hard it is to come to reason with all this, I don’t really think there is one “right way” that will prove intuitive for users (it shouldn’t be that hard to find, if so). So with that, I’m pondering on the feasibility of splitting the two concepts of path specs vs target specs on the command line, so we don’t have to treat them the same (as we can’t always tell them apart, so what the user intended must be consistent regardless of spec type, which is difficult to achieve here). So, that could be that we signal with a flag, or require a fixed prefix or some such for target specs, in order to separate them from path specs. Example sketch
Copy code
./pants test //src/py/utils  # interpret as target address
./pants test src/py/utils  # interpret as path spec
./pants test src/py/utils:tests  # still path spec, if there is not a dir "utils:tests" it is an error
./pants test --targets src/py/utils:test  # all specs treated as target address specs due to flag
I’m not really up to speed on all the nuances to be able to reason if this will improve the situation considerably or not.. but thought worth exploring (or discarding)… 😉 (I’ll be afk now, time to put some kids to bed..)
👋 3
w

witty-crayon-22786

10/14/2021, 5:56 PM
@curved-television-6568: yea, right now both
*
wildcard matching and
::
matching exist: the latter is intended to match targets, and the former to match files, regardless of whether there are associated targets. but whether they should be merged (and whether there should be a filtering flag like the one you mentioned) is an open question.
re: matching: i think it’s also important to note that the directory syntax that we’re replacing here is
dir
expanding to `dir:dir`: if we stop matching the target in that case, it’s a more significant behavior change, and seemingly closer to a regression (maybe in the eye of the beholder, but.).
h

hundreds-father-404

10/14/2021, 6:01 PM
if we stop matching the target in that case, it’s a more significant behavior change
Changing the semantics of
dir
being
dir:dir
in particular can be feature gated to not break the deprecation policy as badly, demonstrated by https://github.com/pantsbuild/pants/pull/13219/commits/1c93cafff6d51e291596a9041a33b51966292c8f Maybe confusing that I used
./pants test src/py
in my example rather than
src/py:
. I was using
src/py
because that's what users will be typing once we finish completely deprecating it beign shorthand for
src/py:py
w

witty-crayon-22786

10/14/2021, 6:08 PM
hm. didn’t realize that that had already landed. but i don’t think that that deprecation is necessary if aliases are matched, since it’s strictly more being included.
h

hundreds-father-404

10/14/2021, 6:13 PM
Purely the refactor landed. We eagerly convert
DirLiteralSpec
into
AddressLiteralSpec
. I reverted all breaking changes from it
h

happy-kitchen-89482

10/14/2021, 6:31 PM
I think
/pants test src/py
to the user means
/pants test src/py/test_*.py
(or whatever their test file naming conventions are)
Or in other words "run all the test files in `src/py`"
👍 1
Users don't think about targets unless they must, and we don't want users to think about targets unless they must.
w

witty-crayon-22786

10/14/2021, 6:33 PM
and if the only thing in that directory is a target that is an alias for files?
h

happy-kitchen-89482

10/14/2021, 6:33 PM
Then nothing, if the files themselves are not in that directory
To the user, there are no tests in that dir
note that the user can always address the generator target by alias
I'm just saying that (implicit or explicit) globs shouldn't include them
w

witty-crayon-22786

10/14/2021, 6:34 PM
so you’re saying that
./pants run src/py
should run a binary target, even if it has no files, but
./pants test src/py
should ignore a test target?
h

hundreds-father-404

10/14/2021, 6:35 PM
*a tests target generator. Which solely exists for boilerplate reduction
h

happy-kitchen-89482

10/14/2021, 6:35 PM
A binary target can be referenced by its entry point file
w

witty-crayon-22786

10/14/2021, 6:35 PM
@happy-kitchen-89482: and if the file isn’t in that directory?
h

happy-kitchen-89482

10/14/2021, 6:35 PM
And users will typically want to do that
If the file isn't in that directory then
./pants run src/py
should not run it
c

curved-television-6568

10/14/2021, 6:36 PM
Pants run src/py doesn’t make sense, really…
h

happy-kitchen-89482

10/14/2021, 6:36 PM
That is, assuming we're agreed that
src/py
means "just the dir, not its subdirs", and I think we all agree on that at this point?
w

witty-crayon-22786

10/14/2021, 6:36 PM
@curved-television-6568: currently it will run the default target in that directory
h

happy-kitchen-89482

10/14/2021, 6:36 PM
Right, and I am fine removing that ability
./pants run src/py/main.py
Is far more intuitive
h

hundreds-father-404

10/14/2021, 6:36 PM
It is a little more nuanced than "only targets that have files". A
go_binary
is file-less. But it does reside in the directory where it is defined, so
./pants run src/go
will run any
go_binary
targets defined there
h

happy-kitchen-89482

10/14/2021, 6:37 PM
or
./pants run src/py:tgt
if you want to address the target itself
w

witty-crayon-22786

10/14/2021, 6:37 PM
but… why? why would we not match aliases? this just seems like removing something useful and consistent unnecessarily.
h

happy-kitchen-89482

10/14/2021, 6:37 PM
Because it's confusing
and unintuitive
and I don't see how it's that useful
w

witty-crayon-22786

10/14/2021, 6:38 PM
to match targets, rather than only files?
h

happy-kitchen-89482

10/14/2021, 6:38 PM
yes
h

hundreds-father-404

10/14/2021, 6:38 PM
why would we not match aliases?
For a user who doesn't really care about targets and BUILD files or even Pants, it's confusing that
./pants test src/py
might run
src/py/foo
,
src/py/foo/bar
, *
h

happy-kitchen-89482

10/14/2021, 6:39 PM
Are there use-cases where there really is no file and you want to act on multiple targets? There might be
w

witty-crayon-22786

10/14/2021, 6:39 PM
and
./pants peek src/py
and
./pants list src/py
shouldn’t render those targets either, you’re saying?
h

happy-kitchen-89482

10/14/2021, 6:42 PM
Just had a thought: What if addresses with colons always match targets (so
src/py:
and
src/py::
match all targets in the dir/subdirs), but addresses without colons always match files/dirs, and if you want to do so recursively you use
-r
?
So basically if you want to step into the domain of targets you can, but if you don't you have a very intuitive way not to (
-r/--recursive
is reasonably common in other tools)
Basically I'm proposing a more intuitive version of the distinction between
::
and the hypothesized
...
👍 1
h

hundreds-father-404

10/14/2021, 6:44 PM
Worth considering. I like
-r
more than
dir/...
h

happy-kitchen-89482

10/14/2021, 6:44 PM
So
./pants -r peek src/py
and
./pants peek src/py::
are different things
That way we don't have to even teach people about the colon
unless they need it
h

hundreds-father-404

10/14/2021, 6:45 PM
And then I really like that that fixes the weirdness of how we normally want
./pants tailor
to be recursive. We'd "fix" it to not recurse by default, and you use
./pants -r tailor dir
for recursion
h

happy-kitchen-89482

10/14/2021, 6:45 PM
So in that example,
/pants peek src/py
doesn't render those targets, but
/pants peek src/py:
does
h

hundreds-father-404

10/14/2021, 6:45 PM
A limitation of
-r
is that you can't have some dirs be recursive and others be non-recursive, whereas
test dir another_dir::
would handle that. But probably fine
w

witty-crayon-22786

10/14/2021, 6:45 PM
-r
doesn’t let you control recursion on a spec by spec basis
coke 1
h

happy-kitchen-89482

10/14/2021, 6:46 PM
OK
Is that a dealbreaker?
You could say the same for
rm
1
h

hundreds-father-404

10/14/2021, 6:47 PM
Less of a dealbreaker imo than the clunkiness of
dir/... another-dir/...
w

witty-crayon-22786

10/14/2021, 6:48 PM
i think that the scope of this discussion has gotten too large. i would suggest that we table everything except fixing the fact that
:
and
::
don’t match generated targets, and defer the rest until after 2.8.x. the actual bug that exists right now is that
./pants peek ::
and
./pants list ::
don’t show you all targets, and that’s really the only thing that imo MUST be fixed for 2.8.x
but i believe that completely changing how recursion and globs work in general is too much to do at this juncture, and has involved too many pivots.
h

hundreds-father-404

10/14/2021, 6:49 PM
Oh! @happy-kitchen-89482 I really like that this solves the problem from yesterday of what to do with
SourcesSnapshot
(
count-loc
and
validate
). If you use
:
and
::
, that means only operate on sources owned by targets, like today If you use
dir
or
-r dir
, that means all sources regardless of targets
w

witty-crayon-22786

10/14/2021, 6:53 PM
-r
might be a replacement for
*
syntax, but i feel like we’ve gone totally off the rails to be discussing a third globbing/recursion semantics for what started as a bugfix so that Asher can get a graph of the whole repo with
./pants peek ::
h

hundreds-father-404

10/14/2021, 6:54 PM
Our histories are different there: for me, the motivation was always that Tom and me found specs semantics to be extremely confusing for Go, enough to block it being announcable
w

witty-crayon-22786

10/14/2021, 6:55 PM
the more the scope increases here, the more ill advised it is to be trying to get it in before 2.8.0. there is an immediate bug to be fixed.
@hundreds-father-404 sure: and that’s the bug that
::
and
:
don’t match file/generated addresses: let’s fix that.
basically, i think that moving away from 111 for go and terraform was a mistake, and introduced an otherwise-unnecessary, seemingly urgent need to overhaul our specs and ship https://github.com/pantsbuild/pants/issues/12286 … without moving to “metadata at the root of the repo”,
./pants test src/go/pkg
does what you expect, addresses are short without adding `#`s, and it’s just
:
and
::
that have a glaring bug (across all languages).
concretely, i think that we should: 1. revert to 111 for go/terraform 2. fix the bug that
:
and
::
don’t match file/generated targets 3. defer complete overhauls of
**
vs
-r
vs
::
until we can do them without the false pretext (imo) of fixing a bug
signing off for a bit.
h

happy-kitchen-89482

10/14/2021, 8:07 PM
Fair, I don't want to squeeze this into 2.8.0 until there is some consensus
And I don't want to entangle the long term goal with any short-term fixes
I also don't want us to lose sight of the long term goal
h

hundreds-father-404

10/14/2021, 8:14 PM
I agree with punting on changing
dir
to be shorthand for
dir:
, and punting on it meaning something else. I think it's important we get directory specs right. (And adding dir specs would prompt a bunch of other changes I'd feel compelled we'd need to do) I'm putting up now the "bug fix" that gets
:
and
::
behaving how Stu believes they should. I believe it's a big step forward, even if I'd like to improve it even more (what this thread is about) I don't agree with reverting the Go changes. It's experimental, so we're not committing to things. Thanks to
./pants tailor
, it should be easy for Pants 2.9 to stop doing the target generation and instruct alpha users that they need to now use
./pants tailor
. The docs won't mention generated address syntax, they'll only talk about using
dir:
and
dir::
to run, which this pending bug fix mostly gets right
Which means that I think after this bug fix today, the remaining 2.8 priorities for me personally to want to land would be: • landing
overrides
for
python_sources
et al (only a few tests away) • figuring out
python_tests
generating
conftest.py
as
python_test
, which it isn't. Might cause major issues with lockfiles in the future. https://github.com/pantsbuild/pants/issues/13238 • (stretch goal)
./pants mend
Alright, typing out the PR description in https://github.com/pantsbuild/pants/pull/13263 helped me to rethink this discussion. I realize I totally see what Stu and Andreas are talking about with
dir:
and
dir::
, that they should continue to expand target generators even with build goals like
test
But I also still believe
./pants test dir
should only test
dir/*
and not expand target generators. (I also think
./pants package dir
should package all file-less targets like
go_binary
, which is where this gets tricky.
dir
is not strictly shorthand for
'dir/*'
, it would include file-less targets) And what I realize now is I disagree with our earlier decision that
dir
should ==
dir:
. I think it's useful to have `dir`/`-r dir` and also `dir:`/`dir::` I agree with punting on directory specs in Pants 2.8. Let's land https://github.com/pantsbuild/pants/pull/13263 (when I finish it), and call it a day for now with specs
❤️ 1
w

witty-crayon-22786

10/15/2021, 1:14 AM
But I also still believe 
./pants test dir
 should only test 
dir/*
 and not expand target generators. (I also  think 
./pants package dir
 should package all file-less targets like 
go_binary
, which is where this gets tricky. 
dir
 is not strictly shorthand for 
'dir/*'
, it would include file-less targets)
yes, that is where things get tricky. because it is inconsistent for some goals to match targets and others not to.
1
👍 1
thanks for iterating on this though.
1
❤️ 1
h

hundreds-father-404

10/22/2021, 8:16 PM
Not to restart another bike shed...but fyi I think we actually won't need something like
dir/...
, even if we decide for
dir
and
dir:
to have different semantics around target generators To recap, I'm thinking that
dir:
should replace a target generator with all its generated targets, whereas
dir
should not and only run on targets resident to that directory. If we're recursing into subdirs, I don't think that really matters whether we replace target generators or not. We'll already end up globbing over every target generated by it because those targets must be in subdirectories. The end result should be the same That's a good thing! We only need
dir
,
dir:
,
dir::
,
dir/f.py
, and
dir:tgt
. (I suspect we can under-document
dir:
in favor of
dir
also)
w

witty-crayon-22786

10/22/2021, 9:23 PM
To recap, I’m thinking that 
dir:
 should replace a target generator with all its generated targets, whereas 
dir
 should not and only run on targets resident to that directory.
i don’t think it’s a question of “replacing” so much as “expanding”… the caller can decide whether things end up “replaced” (confusingly, by requesting either
Target
or
UnexpandedTarget
). but
Specs
calculation should either expand an alias or not, without replacing it.
but yea, agreed we don’t need
dir/…
i’m pretty sure about the above point, but i know that the terminology is overlapping and odd
from the comment on your
UnexpandedTargets
ticket:
await Get(UnexpandedTargets, Specs(SiblingAddresses(..))): new: includes BOTH the generator and the generated, as in your example
await Get(Targets, Specs(SiblingAddresses(..)): as now, includes only the generated targets, since the aliases will have been expanded and deduped with the generated
await Get(UnexpandedTargets, Specs(SingleAddress(..)): as now, only the alias (as in your example)