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

Allow --no-watch-filesystem with --pantsd #21451

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

x0f0
Copy link

@x0f0 x0f0 commented Sep 25, 2024

Addresses #21448

Allows not watching the fs while keeping pants around.

This is useful when running multiple commands in a read-only environment like CI where there are resource constraints that limit file watching.

@kaos
Copy link
Member

kaos commented Sep 25, 2024

I think it could be worth-while to discover why this restriction was added in the first place, to see if that no longer applies, or if different trade-offs should be made now.

@huonw
Copy link
Contributor

huonw commented Nov 2, 2024

My theory (unverified) would be that using --no-watch-filesystem with Pantsd is a very large foot-gun: it'd be very easy for someone to run, say pants --no-watch-filesystem test ::, edit some code, and then run it again, and not realise that they're getting incorrect/unexpected results, because Pantsd is operating in the background without an obvious trace.

This seems subtle enough that it seems worth protecting us users from our own mistakes/lack-of-knowledge. I reckon I'd make this mistake, even being a Pants Maintainer: I don't remember all the interactions between flags/behaviours at all times, and I really don't want to have to.

So, if we're allowing this (and the motivation seems reasonable), I think we should make it a more explicit/intentional opt-in, e.g.:

  1. an extra flag that is "stronger" --no-watch-filesystem-even-with-pantsd (i.e. passing --no-watch-filesystem still gives the same error with pantsd, but passing that one doesn't)
  2. evolving the flag into one with enum options --watch-filesystem=... with values yes, no, never-even-with-pantsd (or something)
  3. an extra flag that allows this case, --allow-no-watch-filesystem-with-pantsd, so one must pass --no-watch-filesystem --allow-no-watch-filesystem-with-pantsd together
  4. Maybe something else?

Thank you for taking the time to contribute, @x0f0!

@x0f0
Copy link
Author

x0f0 commented Nov 6, 2024

@huonw , yes it's a footgun if used in a transient environment. However, I'm specifically working in a clean slate CI environment where I don't control the max user watches.

@huonw
Copy link
Contributor

huonw commented Nov 6, 2024

Yeah, I understand the motivation and it makes sense, I think.

The connection to the footgun concern is that loosening the restriction in the simplest way (as written) allows usage in CI environments (yay!), but it allows usage locally too (not good!).

There's no hint in the flag name (or the current docs, but docs should be a last resort) of the risks involved. Someone coming in with a different problem/context could easily accidentally start using this flag inappropriately.

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.

4 participants