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

Comments

5 participants
@kwlzn
Copy link
Member

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 added this to TODO in Pants Daemon via automation Mar 7, 2018

@kwlzn kwlzn removed their assignment Jul 18, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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 added a commit that referenced this issue Apr 16, 2019

Improve `--pantsd-invalidation-globs` using Snapshot fingerprints (#7531
)

### 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

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Closed by #7531

@blorente blorente closed this Apr 16, 2019

Pants Daemon automation moved this from TODO to Done Apr 16, 2019

stuhood added a commit that referenced this issue Apr 22, 2019

pantsd auto invalidates pants.ini and all pythonpath of pants (#7599)
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
You can’t perform that action at this time.