https://pantsbuild.org/ logo
Join Slack
Powered by
Hi! I’m working on <https://github.com/pantsbuild/...
# development
a
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
more independent i think…
a
Makes sense
w
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
Maybe 
watch_filesystem
? For advanced options, they're usually set in 
pants.toml
Copy code
[GLOBAL]
watch_filesystem = false
a
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
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
the restarts are a bit buggy. they don’t take into account graph cleaning, but really should.
➕ 1
a
OK, that’s a good enough reason to just remove it from the picture
h
do we have an issue for that?
w
sorry, “bit buggy” meaning “they restart more than they should”
➕ 1
h
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
Not presently
it restarts even on a single run if a dependent file changes
w
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
So what’s the end goal? We use 
pantsd OR loop
?
w
yes.
a
OK, I think I can do that
h
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
true.
i guess it’s a question of whether we want to call it a guarantee or a best effort mechanism.
h
--no-watch-filesystem
== zero effort, compared to "best effort". So I do think a dedicated option makes sense
w
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
OK, that all makes sense
h
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
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
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
eh, imo no new option.
h
How come?
w
h
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
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
a
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
looks good! can probably move it over
a
Let me pull out some uglies 🙂
and then I will move it to upstream
w
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
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
Open in Slack
PreviousNext
Powered by