hundreds-father-404
01/17/2020, 11:48 PMcmd_line_spec_parser.py
. Several ambiguities:
1. does src/python/pants/util
mean src/python/pants/util:util
or the directory src/python/pants/util
- For now, filesystem specs don’t work with directories (PathGlobs
doesn’t), we’ll stick with expanding to src/python/pants/util:util
2. Not all files have endings, e.g. the file ./pants
I believe it is safe for the parser to use Path.is_file()
. We already do Path.exists()
in arg_splitter.py
. So, I’m thinking these heuristics:
- Path.is_file()
-> filesystem spec
- *
in the arg -> filesystem spec because it’s a glob
- otherwise, address specspecs
vs. goals for us, fortunately. That is, it already clarified whether cache.java
refers to the goal cache.java
or the file `cache.java`; and whether haskell
refers to directory haskell
vs. the goal haskell
aloof-angle-91616
01/17/2020, 11:50 PMdoes src/python/pants/util mean src/python/pants/util:util or the directory src/python/pants/utilso right now, for a lot of cases, it seems like the desired behavior when selecting a directory would be to just operate on all the targets within that directory
::
hundreds-father-404
01/17/2020, 11:51 PM--owner-of
aloof-angle-91616
01/17/2020, 11:51 PM::
hundreds-father-404
01/17/2020, 11:53 PMwell i was thinking we wouldn’t need to change any semanticsIt’s confusing to me for
src/python/pants/util
to sometimes mean util/
and sometimes util:util
. It’s confusing because it’s not transparent.
We’d need to first drop support for src/python/pants/util
expanding to src/python/pants/util:util
, which I’d be in favor of (in a followup - not close to being ready yet)aloof-angle-91616
01/17/2020, 11:55 PMto sometimes mean util/ and sometimes util:util.well, until now, it has always just meant
util:util
. the proposed new behavior i believe you're describing is to have it now sometimes mean util/
as well, which you've described as confusing and not transparent.hundreds-father-404
01/17/2020, 11:55 PMPath.exists()
. We use in the same file os.path.realpath()
(== Path.resolve()
)aloof-angle-91616
01/17/2020, 11:55 PM::
because if not, then we may not need to introduce the util/
interpretation at allhundreds-father-404
01/17/2020, 11:56 PMaloof-angle-91616
01/18/2020, 12:00 AMwitty-crayon-22786
01/18/2020, 1:18 AMI believe it is safe for the parser to usethis isn't safe without relativizing to the buildroot. but it is in general a sandbox escape. We already doPath.is_file()
inPath.exists()
. So, I’m thinking these heuristics:arg_splitter.py
hundreds-father-404
01/18/2020, 1:19 AMbase/cmd_line_spec_parser.py
witty-crayon-22786
01/18/2020, 1:20 AMaverage-vr-56795
01/20/2020, 12:16 PMBUILD
file will change the meaning of a command line… That feels like a bad property of the UI…hundreds-father-404
01/21/2020, 5:15 PMI don’t love that creating a BUILD file will change the meaning of a command line… That feels like a bad property of the UI…What do you mean by this? Is this in reference to disambiguating between directories vs. targets? To clarify, I 100% want to punt on this decision (But ftr, I really like Benjy’s suggestion to add a global option that allows users to specify which semantics they want)
aloof-angle-91616
01/21/2020, 5:16 PMaverage-vr-56795
01/21/2020, 5:37 PM./pants cloc foo
mean in the following directory layouts:
1. foo is a file
2. foo is a directory
3. foo is a directory and foo/BUILD
exists
I don’t think a flag should change that meaning, and ideally that meaning wouldn’t change even if filesystem changes are made to change which one of 1/2/3 is the case. Otherwise when I write my script, and I write it slightly sloppily because I don’t understand the edge cases, my script may start doing the wrong thing when faced with a different filesystem than I tested it on.hundreds-father-404
01/21/2020, 5:38 PMfoo
is a file, treat it as a file
2) Otherwise, always try to expand to foo:foo
, even if there is no BUILD file or directoryaloof-angle-91616
01/21/2020, 5:39 PMhundreds-father-404
01/21/2020, 5:40 PM./pants buildgen src/python 3rdparty/python
on a brand new Pants repo with no BUILD filesaloof-angle-91616
01/21/2020, 5:40 PM./pants buildgen src/python:: 3rdparty/python::
?hundreds-father-404
01/21/2020, 5:41 PM./pants buildgen src/python:: 3rdparty/python::
means you are using address specs, but there are no BUILD files yet so this won’t actually match against anythingaloof-angle-91616
01/21/2020, 5:42 PMbuildgen
taskhundreds-father-404
01/21/2020, 5:42 PM./pants buildgen 'src/python/**/*' '3rdparty/python/**/*'
aloof-angle-91616
01/21/2020, 5:42 PM/**/*
better than ::
?average-vr-56795
01/21/2020, 5:43 PMok, but we’re creating pants targets right? why isBecause it’s a really weird special case, both in the user interface and in the enginebetter than/**/*
?::
aloof-angle-91616
01/21/2020, 5:43 PMBecause it’s a really weird special casei think we can just avoid erroring when
::
doesn't match any targets though? the reason i like this is because right after you run buildgen
, you can immediately change the command-line goal, and not have to change any address specs, so you can run ./pants list src/python:: 3rdparty/python::
immediately. that seems SUPER intuitive to me.hundreds-father-404
01/21/2020, 5:45 PMok, but we’re creating pants targets right? why is /**/* better than ::?
cmd_line_spec_parser.py
needs some way to know “they want filesystem specs” vs. “they want address specs”. The engine may only pass one type of Specs to the goal, otherwise we get graph ambiguity that there are two ways to compute things like BuildFileAddresses
.
So, we must reserve ::
to always mean address specs and never try to co-opt it for filesystem specs.
This means we need some sort of glob for filesystem specs to complement ::
. *
is a natural glob that is already supported by PathGlobs
and is natural to CLI usersaloof-angle-91616
01/21/2020, 5:45 PM::
because you then have to change the pants command line specs to operate on the targets you just generated.hundreds-father-404
01/21/2020, 5:49 PMfor the case of buildgen, i feel like they’re inappropriate.Buildgen is meant to work on a brand new repo, though, with no targets. So,
./pants buildgen ::
won’t resolve anything on a first run.
We could hack the AddressSpecs ::
to be converted into filesystem specs, which is what Toolchain currently does, but that’s not great to change the semantics.
After that first run, you can use ./pants buildgen ::
of course.average-vr-56795
01/21/2020, 5:49 PMhundreds-father-404
01/21/2020, 5:50 PMDo we have any cases where we actually really want file specs for anything other than buildgen?Well, to clarify, we want to support either filesystem specs or address specs for goals. Another example:
./pants test tests/python/pants_test/util/test_strutil.py
aloof-angle-91616
01/21/2020, 5:55 PMWe could hack the AddressSpecs :: to be converted into filesystem specs, which is what Toolchain currently does, but that’s not great to change the semantics.that's not what i'm thinking of. i'm thinking of them representing target globs. the
buildgen
task would read the Specs
as target globs, convert each Spec
into whatever type of input buildgen
needs, then run it. i'm not thinking of making ::
into filesystem globs.hundreds-father-404
01/21/2020, 5:56 PM./pants test tests/python/pants_test/util/test_strutil.py
?
Leveraging how users already interact with most tools. For example, you’d run Pytest as pytest tests/python/pants_test/util/test_strutil.py
. Same with running MyPy, isort, and Flake8.
Users can now use tab completion on specs, which currently only works when specifying the directory
Finally, you no longer need to lookup what the target name is that corresponds to that file (a huge pain for me doing the Py3 migration)aloof-angle-91616
01/21/2020, 5:57 PMFinally, you no longer need to lookup what the target name is that corresponds to that file
./pants --owner-of=tests/python/pants_test/util/test_strutil.py test
doesn't work?hundreds-father-404
01/21/2020, 5:58 PMI’m thinking of them representing target globs. the buildgen task would read the Specs as target globs,Please checkout https://docs.google.com/document/d/15xphZcFnowytF0Qu2sO1PG7wHAWoyLof7hK0jVa5T8E/edit#heading=h.dn7v2rykapob if you haven’t recently. It was rewritten this past week to account for a) a new understanding of the different types of use cases we have. There are 4 types of goals, not the original 2 I thought. b) having had spent the past two weeks in this spec code, a new understanding of how this can actually be implemented. For example, we dropped the idea of a top level
Specs
average-vr-56795
01/21/2020, 6:04 PMPlease checkout https://docs.google.com/document/d/15xphZcFnowytF0Qu2sO1PG7wHAWoyLof7hK0jVa5T8E/edit#heading=h.dn7v2rykapob if you haven’t recently. It was rewritten this past week to account forI will have a read 🙂
aloof-angle-91616
01/21/2020, 6:14 PMhappy-kitchen-89482
01/21/2020, 8:26 PM./pants test src/python/foo/bar_test.py
is intuitive, ./pants test src/python/foo:bar
is less so. It requires reasoning about a new conceptual domain.
Targets may still need to exist (e.g., to express deps and interpreter constraints), but the user should only have to think about them when necessary.aloof-angle-91616
01/21/2020, 8:27 PM