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

Change the PathGlobs, SnapshotSubset, and UrlToFetch intrinsics to return Digest #10449

Merged
merged 10 commits into from Jul 27, 2020

Conversation

Eric-Arellano
Copy link
Contributor

The engine knows how to go from Digest -> Snapshot, but not the other way around. By changing these three intrinsics, rules have much more flexibility.

Now, the claim in https://www.pantsbuild.org/v2.0/docs/rules-api-file-system#core-abstractions-digest-and-snapshot that "The core building block is a Digest" is true.

--

This also renames SnapshotSubet to DigestSubset, given that it takes as input Digest and returns a Digest.

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
@Eric-Arellano
Copy link
Contributor Author

There are three legit test failures in fs_test.py that await Get(Snapshot, PathGlobs) no longer correctly resolves all .dirs. I can't reproduce this issue when testing with some files in our repo, so I suspect that the problem is due to a symlinked folder in the test.

Any help would be much appreciated. I think this is a real bug on the code to go from Digest -> Snapshot. https://travis-ci.com/github/pantsbuild/pants/jobs/364916874#L673

The originals seem incorrect. These tests now emulate how an actual FS works if you were to expand the Tar file and use `ls` to traverse the directories. These fixes also make logical sense; if we have `a/b` in the output dirs, then I would expect `a` to show up too.

# Rust tests will be skipped. Delete if not intended.
[ci skip-rust-tests]
@Eric-Arellano
Copy link
Contributor Author

Eric-Arellano commented Jul 25, 2020

There are three legit test failures

Nevermind, the original was wrong. There's a bug in the original implementation.

Given this code:

    sn = await Get(Snapshot, PathGlobs(["src/python/pants/util/*.py"]))
    print(sn.dirs)
    sn2 = await Get(Snapshot, Digest, sn.digest)
    print(sn2.dirs)

We get:

()
('src', 'src/python', 'src/python/pants', 'src/python/pants/util')

@coveralls
Copy link

coveralls commented Jul 25, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling 5020012 on Eric-Arellano:return-snapshot into 9ca7b52 on pantsbuild:master.

Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'd recommend getting a review from someone more versed in this before merging.

Copy link
Sponsor Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/rust/engine/src/intrinsics.rs Outdated Show resolved Hide resolved
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

Successfully merging this pull request may close these issues.

None yet

4 participants