https://pantsbuild.org/ logo
#development
Title
# development
a

ancient-vegetable-10556

06/16/2021, 5:48 PM
Hi! I’m working on https://github.com/pantsbuild/pants/issues/12059 and I want a quick sanity check — would an option to watch the filesystem belong in the local store options, or is it more of an independent thing?
w

witty-crayon-22786

06/16/2021, 5:51 PM
more independent i think…
a

ancient-vegetable-10556

06/16/2021, 5:51 PM
Makes sense
w

witty-crayon-22786

06/16/2021, 5:51 PM
but i don’t think that we need a new option, per-se… i think that it might be the OR of
--pantsd
and
--loop
👍 1
h

hundreds-father-404

06/16/2021, 5:52 PM
Maybe
watch_filesystem
? For advanced options, they're usually set in
pants.toml
Copy code
[GLOBAL]
watch_filesystem = false
a

ancient-vegetable-10556

06/16/2021, 5:52 PM
I had already added an option, it was more just how I propagate the option into the system/an organisational thing
But Stu makes a good point, which I’ll go with for now.
Though, @witty-crayon-22786, I note that with filesystem watching enabled, Pants’ current behaviour is to restart a build if a dependent file is changed, and that has been mildly useful to me in the past
1
is that something we don’t care about enough to justify not adding a new option?
h

hundreds-father-404

06/16/2021, 5:55 PM
and that has been mildly useful to me in the past
Opposite for me, it's been mildly annoying. And some users have reported frustration iirc, that you're frozen from making changes until you get back results
w

witty-crayon-22786

06/16/2021, 5:56 PM
the restarts are a bit buggy. they don’t take into account graph cleaning, but really should.
1
a

ancient-vegetable-10556

06/16/2021, 5:56 PM
OK, that’s a good enough reason to just remove it from the picture
h

hundreds-father-404

06/16/2021, 5:56 PM
do we have an issue for that?
w

witty-crayon-22786

06/16/2021, 5:56 PM
sorry, “bit buggy” meaning “they restart more than they should”
1
h

hundreds-father-404

06/16/2021, 5:57 PM
that’s a good enough reason to just remove it from the picture
What do you mean? I'd rather we fix the buggy restarts than have an option to disable the buggy behavior iiuc, Pants should only restart upon file changes with
--loop
, right?
a

ancient-vegetable-10556

06/16/2021, 5:57 PM
Not presently
it restarts even on a single run if a dependent file changes
w

witty-crayon-22786

06/16/2021, 5:58 PM
no: it’s an important part of our guarantees that if the filesystem changes somewhere below a rule that it not observe the filesystem in a “half this way, half the other way” state.
👍 1
the issue is that many, many file changes that invalidate globs (“new file created in directory”) don’t actually impact any particular file: and graph cleaning determines that, and doesn’t re-run the rules. the bug is that before we can clean a node, we dirty it: and that will cancel it even in some cases where it doesn’t need to: see link above.
1
a

ancient-vegetable-10556

06/16/2021, 5:59 PM
So what’s the end goal? We use
pantsd OR loop
?
w

witty-crayon-22786

06/16/2021, 5:59 PM
yes.
a

ancient-vegetable-10556

06/16/2021, 5:59 PM
OK, I think I can do that
h

hundreds-father-404

06/16/2021, 6:00 PM
t’s an important part of our guarantees that if the filesystem changes somewhere below a rule that it not observe the filesystem in a “half this way, half the other way” state.
Hm, then wouldn't disabling file watching violate this guarantee? Iiuc,
--no-watch-filesystem
would mean that you are acknowledging some risk of invalid builds. Otherwise, Pants makes a best effort to guarantee valid builds
w

witty-crayon-22786

06/16/2021, 6:01 PM
true.
i guess it’s a question of whether we want to call it a guarantee or a best effort mechanism.
h

hundreds-father-404

06/16/2021, 6:02 PM
--no-watch-filesystem
== zero effort, compared to "best effort". So I do think a dedicated option makes sense
w

witty-crayon-22786

06/16/2021, 6:02 PM
i’m certain that there are races that are possible, so “guarantee” is probably too strong a word.
👍 1
@hundreds-father-404: let’s drop the word guarantee there then. because it’s not really possible for it to be a guarantee with POSIX
1
and for practicality’s sake, let’s do
pantsd OR loop
.
☝️ 1
a

ancient-vegetable-10556

06/16/2021, 6:04 PM
OK, that all makes sense
h

hundreds-father-404

06/16/2021, 6:04 PM
yeah sg on "best effort" So, @ancient-vegetable-10556 I recommend an option
--watch-filesystem
, registered near the
--pantsd
options ideally (order of registering impacts
./pants help-advanced global
). In the help message, explain that Pants normally makes a best effort to restart builds invalidated by filesystem changes. Disabling filewatching increases the risk of invalid builds
a

ancient-vegetable-10556

06/16/2021, 6:06 PM
I’m going to let you and Stu argue about how to add the option in, I think at this point I understand what I’m doing enough to plumb it between where the configuration needs to be set and the place in Rust where we consume it
✔️ 1
h

hundreds-father-404

06/16/2021, 6:06 PM
and for practicality’s sake, let’s do pantsd OR loop .
I think Stu is suggesting that we validate both
pantsd
and
loop
are not set if
watch_filesystem
is false? I do think we want a dedicated option because best effort > no effort
w

witty-crayon-22786

06/16/2021, 6:06 PM
eh, imo no new option.
h

hundreds-father-404

06/16/2021, 6:06 PM
How come?
h

hundreds-father-404

06/16/2021, 6:07 PM
It's fairly common to have
--no-pantsd
(e.g. CI) and
--no-loop
(default). That means in a lot of places we will no longer have any effort to check for invalid builds. That seems wrong. The default of file watching is a good one, regardless of pantsd Afaict, you should only set
--no-watch-filesystem
because of weird edges like Docker and M1s
w

witty-crayon-22786

06/16/2021, 6:08 PM
ok. i don’t feel strongly about it. but with the extra option we need to validate that filesystem watching is enabled if pantsd OR loop are.
h

hundreds-father-404

06/16/2021, 6:13 PM
a

ancient-vegetable-10556

06/16/2021, 7:50 PM
OK, here’s my first pass at making this work. Let me know if there’s anything I’m missing, or how to go about adding tests
(I’ll move the PR over to
/pantsbuild
once it’s more-or-less done
w

witty-crayon-22786

06/16/2021, 8:11 PM
looks good! can probably move it over
a

ancient-vegetable-10556

06/16/2021, 8:12 PM
Let me pull out some uglies 🙂
and then I will move it to upstream
w

witty-crayon-22786

06/16/2021, 8:17 PM
for something like this … testing is challenging. for a few reasons.
we’re checking that something that is always async … doesn’t happen. but we also know that it can’t be disabled if pantsd is enabled, so integration tests would be very challenging/racey
so, i would be fine letting this one slide by without a test.
a

ancient-vegetable-10556

06/16/2021, 8:21 PM
That makes some sense. I was wondering if there were anything in the config that were worth testing, but it’s really just the mutual exclusion, which is not likely to be a very edifying test
👍 2