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

Improve --pantsd-invalidation-globs implementation with actual fingerprinting #5567

Closed
kwlzn opened this issue Mar 7, 2018 · 4 comments
Closed
Assignees

Comments

@kwlzn
Copy link
Member

kwlzn commented Mar 7, 2018

noting Stu's suggestion on #5550

So, I think that this will work. It has some weaknesses related to symlinks I think, but that might be ok for the PYTHONPATH usecase.

Another approach that would be a bit more bulletproof, would be to request a Snapshot for the PathGlobs represented by the option, and to then recompute that Snapshot after every file invalidation event (and it would need to be every event, because the Snapshot does not tell you which symlinks it traversed... it just guarantees that it does), to determine whether the Snapshot's digest had changed. If the digest hadn't changed, this would take a few milliseconds... if it had changed, it would take a bit longer, but not much.

It may not be 100% general though, because if any of the globs represent symlinks outside of the working copy, we'd fail to cover them. Perhaps that requirement is fine?

@kwlzn kwlzn added the pantsd label Mar 7, 2018
@kwlzn kwlzn added this to the Daemon Backlog milestone Mar 7, 2018
@kwlzn kwlzn self-assigned this Mar 7, 2018
@kwlzn kwlzn removed their assignment Jul 18, 2018
@cosmicexplorer
Copy link
Contributor

I believe the implementation of this ticket should consider allowing globs outside of the buildroot, possibly allowing some templating with an environment variable, in order to support restarting pantsd if files in the pants repo change when running pants from source, but in a separate repo.

@stuhood
Copy link
Member

stuhood commented Mar 7, 2019

@cosmicexplorer : That would be ideal, but it is challenging to do with our current implementation of watchman, which can only watch things below the buildroot.

The "watch only what has been globbed" implementation of #4999 might allow for watching arbitrary paths though... cc @illicitonion

@stuhood
Copy link
Member

stuhood commented Mar 7, 2019

Ah, but even then: if pants is cooperating with the version control system to determine what has changed, it would need a split strategy inside and outside the buildroot. Non-trivial, but desirable.

@ity ity self-assigned this Mar 19, 2019
stuhood pushed a commit that referenced this issue Apr 16, 2019
)

### Problem

Improve --pantsd-invalidation-globs implementation with actual fingerprinting. See #5567 for details

### Solution

As a continuation of #7454, compute and save initial Snapshots keyed by globs. Re-compute Snapshots for every fs event and compare with originals to decide whether to restart
@blorente
Copy link
Contributor

Closed by #7531

stuhood pushed a commit that referenced this issue Apr 22, 2019
The invalidation globs for pantsd (improved in #5567) can be manually configured to cover everything that is known to cause pantsd to need to restart. But many of the things that should trigger a restart are already known to pants. A partial list:
  1. The content of any configured pants.ini files (due to #7022)
  2. The pythonpath of pants itself (since changes to loose-source plugins mean the code that pantsd is running might have changed)

We should automatically include these values (and likely others!) in the values that we use for --invalidation-globs. In cases where the options values point to files that exist outside of the buildroot, we should consider logging a warning (unless that ends up being too noisy).

Fixes #7595.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants