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

Add lmdb_store to travis cache #8042

Merged
merged 1 commit into from Jul 12, 2019

Conversation

@illicitonion
Copy link
Contributor

commented Jul 11, 2019

This allows local process execution to be cached across runs

See #8041 for discussion of keeping its size down.

@Eric-Arellano
Copy link
Contributor

left a comment

To be extra defensive at the risk of repetition / obviousness: we do not have the risk of a change in a source file not triggering a re-run of the test, correct? I believe this to be true because:

  1. In V2, a test must declare every single dependency for that target itself, unlike V1 that combines all targets dependencies. This means that we do run the risk of an implicit dependency changing but not registering a change in the test, because it's impossible to have implicit dependencies.
  2. The source files will change, so that cache entry cannot be used and all dependent nodes will also be invalidated, right? This is true even though we use intrinsic / built-in rules to get the Digests, not ExecuteProcessRequest? Specifically
    # Gather sources and adjust for the source root.
    # TODO: make TargetAdaptor return a 'sources' field with an empty snapshot instead of raising to
    # simplify the hasattr() checks here!
    # TODO(7714): restore the full source name for the stdout of the Pytest run.
    sources_snapshots_and_source_roots = []
    for maybe_source_target in all_targets:
    if hasattr(maybe_source_target, 'sources'):
    tgt_snapshot = maybe_source_target.sources.snapshot
    tgt_source_root = source_roots.find_by_path(maybe_source_target.address.spec_path)
    sources_snapshots_and_source_roots.append((tgt_snapshot, tgt_source_root))
    all_sources_digests = yield [
    Get(
    Digest,
    DirectoryWithPrefixToStrip(
    directory_digest=snapshot.directory_digest,
    prefix=source_root.path
    )
    )
    for snapshot, source_root
    in sources_snapshots_and_source_roots
    ]
    sources_digest = yield Get(
    Digest, DirectoriesToMerge(directories=tuple(all_sources_digests)),
    )
@illicitonion

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

The directory where we materialize the files (for local execution) or the machine being used for remote execution, only has files which were declared as dependencies. This means that there are a few ways to change files and not invalidate the cache, but they're all pretty sketchy. They all rely on looking up files outside of the working directory; if your test does something like sys.path.insert(0, "/home/travis/pantsbuild/pants") it can violate our assumption that you can't easily accidentally access files from outside the working directory. Similarly, you can open("/home/travis/pantsbuild/panst/pants.ini").

Outside of the tests themselves, rules can potentially do sketchy things. If you had a rule which called get_buildroot() and then injected that as an env var or command line arg or something, the process being run may use that to access files it shouldn't. And if you edit that file, then run the process again on the same machine, we'll hit the cache, because we haven't declared a dep on the files in the buildroot, just on the string which is the path to it itself.

For the most part, our guard rails against doing this are code review. At some point we may introduce some kind of sandboxing to enforce those constraints, rather than rely on people not doing the wrong thing.

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

Got it. Thank you for explaining that all.

It sounds like my main concern—that someone makes a normal change to src/.../util/strutil.py and somehow tests/.../util/test_strutil.py does not rerun its Pytest ExecuteProcessRequeset, for whatever reason—is not a concern. I'm less concerned about the edge cases you identified.

Add lmdb_store to travis cache
This allows local process execution to be cached across runs

@stuhood stuhood force-pushed the twitter:dwagnerhall/cache-lmdb-store branch from 5c1f750 to 498465b Jul 12, 2019

@stuhood stuhood merged commit 346dcaa into pantsbuild:master Jul 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:dwagnerhall/cache-lmdb-store branch Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.