Plumb symlink support through the Pants engine #16844

thejcannon · 2022-09-13T16:11:02Z

This plumbs everything needed to support symlinks through the engine without actually having them be picked up by path globbing (which will come later)

[ci skip-build-wheels]

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon

@tdyas or @stuhood can y'all comment on the todo!()s? Anything from actual code to copy+paste to high-level description of how to handle that case would be very nice 😄

thejcannon · 2022-09-13T16:15:02Z

src/rust/engine/fs/src/directory.rs

-  /// Return a pair of Vecs of the file paths and directory paths in this DigestTrie, each in
-  /// sorted order.
-  ///
-  /// TODO: This should probably be implemented directly by consumers via `walk`, since they
-  /// can directly allocate the collections that they need.
-  pub fn files_and_directories(&self) -> (Vec<PathBuf>, Vec<PathBuf>) {


I split this into 3 implementations, as only one caller needed all the items at the same time, and it was for __repr__, so perf doesn't matter that much.

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

src/python/pants/engine/fs.py

src/rust/engine/fs/src/directory.rs

src/rust/engine/fs/src/lib.rs

src/rust/engine/fs/store/src/lib.rs

tdyas · 2022-09-13T17:18:59Z

src/rust/engine/fs/store/src/snapshot_ops.rs

@@ -227,6 +238,7 @@ pub trait SnapshotOps: Clone + Send + Sync + 'static {
      directory::Entry::File(f) => {
        files.insert(path.to_owned(), f.digest());
      }
+      directory::Entry::Symlink(_) => todo!(),


Any change here depends on what (if any) changes are needed to the signature of DigestTrie::from_path_stats to support symlinks.

src/rust/engine/process_execution/src/remote_cache.rs

src/rust/engine/fs/src/directory.rs

stuhood

Nice!

src/rust/engine/fs/store/src/lib.rs

src/rust/engine/fs/src/directory.rs

stuhood · 2022-09-13T17:26:17Z

src/rust/engine/fs/src/directory.rs

+    }
+  }
+}
+
 // TODO: `PathStat` owns its path, which means it can't be used via recursive slicing. See
 // whether these types can be merged.
 enum TypedPath<'a> {


The fact that this code hasn't changed suggests that PathStat, which is the type produced while VFS::expand walks the filesystem has not been changed.

If you get tests passing without that change, then I think that what you will have accomplished is that our VFS code (which is what matches globs in the repo for PathGlobs->Digest, and which captures the outputs of processes from sandboxes) will never actually create a DigestTrie/Digest/Snapshot containing a symlink, BUT when one was created synthetically, it would present as such in memory, and be successfully materialized.

That's probably a good breaking point for a first PR, so you might want to avoid dipping into VFS yet if you can.

Yeah just started to muck with fs_test and noticed this behavior.

I like the idea of splitting into multiple PRs.

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

src/rust/engine/fs/src/directory.rs

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

src/rust/engine/fs/src/directory.rs

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon · 2022-09-14T01:20:39Z

Opening for review. LMK if we should tackle the path_stats in this PR, or wait for follow-up.

tdyas

Opening for review. LMK if we should tackle the path_stats in this PR, or wait for follow-up.

I'm fine with the from_path_stats changes going to a separate PR. I would like to see some tests in this PR for the changes made in this PR.

src/rust/engine/src/nodes.rs

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-build-wheels]

src/rust/engine/src/externs/fs.rs

stuhood

Thanks a lot!

Regarding absolute symlinks: it should just amount to ignoring symlinks dests which are: Path::new(...).is_absolute() (https://doc.rust-lang.org/nightly/std/path/struct.Path.html#method.is_absolute) in walk?

stuhood · 2022-10-27T22:00:24Z

src/rust/engine/fs/src/directory.rs

+            // We don't return a Result, so log and move on
+            warn!("Exceeded the maximum link depth while traversing links. Stopping traversal.");


This should probably include some information on the path that was traversed?

I'm guessing we'd want to trim the path. Suggestion on some Rust-y code that'd do that?

thejcannon · 2022-10-28T01:06:30Z

Yeah I just hit the buzzer before I implemented absolute paths. It's up next.

thejcannon · 2022-10-28T19:07:14Z

Absolute paths are donezo. Only problem I think is a symlink to a single ".". I thin it needs to be handled very specially, possibly in multiple places (where we know what the preceeding path comp is).

thejcannon · 2022-10-28T19:23:31Z

Ah I think it's a combo of handling a symlink with a bare ".", And peeking for a "." In the components loop in entry_helper.

thejcannon · 2022-11-02T18:18:41Z

Well that was painful...

thejcannon · 2022-11-02T18:24:55Z

Hehe my abbreviated SHA for the commit "im dead inside" ends in d3ad

src/rust/engine/fs/src/directory.rs

thejcannon · 2022-11-02T18:27:53Z

src/rust/engine/fs/src/directory.rs

      path_so_far.push(component);
-      let component = component.as_os_str();
+      logical_path.push(component);
+      if component == Component::ParentDir {


This very well could be a while loop, but the perf seems negligible and I'm tired of staring at the code.

stuhood

Thanks a lot for persevering here, and for being so thorough with test coverage!

stuhood · 2022-11-02T19:39:47Z

src/rust/engine/fs/src/directory_tests.rs

+use crate::MAX_LINK_DEPTH;
+use hashing::EMPTY_DIGEST;
+use std::path::{Path, PathBuf};
+


Thanks for doing this... I'm lightly embarassed not to have had rust side tests for walk/entry already... but on the other hand, it used to be a lot simpler, heh.

Having these tests is the only way I survived honestly. Turnaround time for changes went from minutes to seconds, and the ability to log/debug easily was invaluable.

thejcannon · 2022-11-03T15:22:33Z

Thanks a lot for persevering here, and for being so thorough with test coverage!

I was not about to half-ass this. Especially because bugs here would absolutely wreck havoc on plugin authors and the end-user.

thejcannon · 2022-11-03T18:21:14Z

Hmm wasn't expecting test_digest_entries_handles_symlinks to timeout ❗

thejcannon · 2022-11-03T19:01:09Z

Hahahaha it was a panic! from me forgetting about symlink-aware code 😂

thejcannon · 2022-11-03T21:33:37Z

Oh one thing to note is when we're walking a Trie as symlink oblivious, directory symlinks are not passed through the function as directories.

E.g. self/foo.bar might be an entry for a file file while self is not a directory entry. In the case that self is symlink.

Cc @stuhood

thejcannon · 2022-11-04T00:56:18Z

src/rust/engine/fs/src/directory_tests.rs

+fn walk_too_many_links_subdir() {
+  let tree = make_tree(vec![
+    TypedPath::File {
+      path: Path::new("a/file.txt"),
+      is_executable: false,
+    },
+    TypedPath::Link {
+      path: Path::new("a/self"),
+      target: Path::new("."),
+    },
+  ]);
+  assert_walk(
+    &tree,
+    (0..MAX_LINK_DEPTH)
+      .into_iter()
+      .map(|n| ("a/".to_string() + &"self/".repeat(n.into()) + "file.txt"))
+      .collect::<Vec<_>>(),
+    vec!["".to_string(), "a".to_string()],


Oh yeah @stuhood here's what I'm talking about. a/self/file.txt is a file (entry is for a/file.txt, but the only directory entries are root and a.

I think conceptually it makes sense. There is no entry for a/self. But an interesting quirk.

Tom Dyas and others added 5 commits September 12, 2022 12:29

checkpoint

f4d3fc0

[ci skip-build-wheels]

symlinks

95a9084

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

big bag of changes

1e842ee

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

switch todos

185f77a

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

update docstring

27a6b48

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon commented Sep 13, 2022

View reviewed changes

more LinkEntry

a320f2c

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

tdyas reviewed Sep 13, 2022

View reviewed changes

stuhood reviewed Sep 13, 2022

View reviewed changes

thejcannon added 2 commits September 13, 2022 12:34

toms

86b83af

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stus

c2ed35d

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon changed the title ~~WIP: Add symlink support to the Pants engine~~ WIP: Plumb symlink support through the Pants engine Sep 13, 2022

thejcannon added 2 commits September 13, 2022 13:46

make walk symlink param

fa06229

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

symlinky

c1c1a8b

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

tdyas reviewed Sep 14, 2022

View reviewed changes

src/rust/engine/fs/src/directory.rs Outdated Show resolved Hide resolved

noclone

cd2ce50

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

tdyas reviewed Sep 14, 2022

View reviewed changes

src/rust/engine/fs/src/directory.rs Outdated Show resolved Hide resolved

copy

2c63f5a

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon marked this pull request as ready for review September 14, 2022 01:20

tdyas reviewed Sep 14, 2022

View reviewed changes

src/rust/engine/src/nodes.rs Outdated Show resolved Hide resolved

stuhood mentioned this pull request Sep 14, 2022

experimental_shell_command outputs do not preserve symlinks #16854

Closed

thejcannon added 3 commits September 14, 2022 09:01

more renames

517dd15

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

fix vec

ab43d4f

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

clippy and others

651e0f1

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon added the category:bugfix Bug fixes for released features label Sep 14, 2022

thejcannon added 3 commits September 14, 2022 10:16

more plumbing

e7ab9eb

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

pathbuf

67746b5

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

checkin

8ced34f

[ci skip-build-wheels]

stuhood reviewed Sep 14, 2022

View reviewed changes

src/rust/engine/src/externs/fs.rs Outdated Show resolved Hide resolved

stuhood approved these changes Oct 27, 2022

View reviewed changes

were so close

d929fb0

im dead inside

f5ad3ad

oops

1e6e8a0

thejcannon commented Nov 2, 2022

View reviewed changes

thejcannon added 2 commits November 2, 2022 13:46

move

187209e

Merge branch 'main' into symlinks

e0ca257

stuhood approved these changes Nov 2, 2022

View reviewed changes

thejcannon added 2 commits November 2, 2022 19:56

clippy

21a94c6

test

b9e386f

off by one error

04075f7

thejcannon added 2 commits November 3, 2022 13:49

hahahahahahahaha

9c0b17d

no logging

8042e15

move it back

aa0ed99

thejcannon commented Nov 4, 2022

View reviewed changes

thejcannon added 3 commits November 4, 2022 06:51

Merge main

196f8f6

Merge branch 'main' into symlinks

2365747

wtf merge

02f9f48

thejcannon merged commit e9ae0a3 into pantsbuild:main Nov 4, 2022

thejcannon deleted the symlinks branch November 4, 2022 15:37

thejcannon mentioned this pull request Nov 8, 2022

remoting: support symlinks attribute in directories #10271

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plumb symlink support through the Pants engine #16844

Plumb symlink support through the Pants engine #16844

thejcannon commented Sep 13, 2022 •

edited

thejcannon left a comment

thejcannon Sep 13, 2022

tdyas Sep 13, 2022

stuhood left a comment

stuhood Sep 13, 2022 •

edited

thejcannon Sep 13, 2022

thejcannon commented Sep 14, 2022

tdyas left a comment

stuhood left a comment

stuhood Oct 27, 2022

thejcannon Oct 28, 2022

thejcannon commented Oct 28, 2022

thejcannon commented Oct 28, 2022 •

edited

thejcannon commented Oct 28, 2022

thejcannon commented Nov 2, 2022

thejcannon commented Nov 2, 2022

thejcannon Nov 2, 2022

stuhood left a comment

stuhood Nov 2, 2022

thejcannon Nov 2, 2022

thejcannon commented Nov 3, 2022

thejcannon commented Nov 3, 2022

thejcannon commented Nov 3, 2022

thejcannon commented Nov 3, 2022 •

edited

thejcannon Nov 4, 2022

		// We don't return a Result, so log and move on
		warn!("Exceeded the maximum link depth while traversing links. Stopping traversal.");

Plumb symlink support through the Pants engine #16844

Plumb symlink support through the Pants engine #16844

Conversation

thejcannon commented Sep 13, 2022 • edited

thejcannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

stuhood Sep 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon commented Sep 14, 2022

tdyas left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon commented Oct 28, 2022

thejcannon commented Oct 28, 2022 • edited

thejcannon commented Oct 28, 2022

thejcannon commented Nov 2, 2022

thejcannon commented Nov 2, 2022

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon commented Nov 3, 2022

thejcannon commented Nov 3, 2022

thejcannon commented Nov 3, 2022

thejcannon commented Nov 3, 2022 • edited

Choose a reason for hiding this comment

thejcannon commented Sep 13, 2022 •

edited

stuhood Sep 13, 2022 •

edited

thejcannon commented Oct 28, 2022 •

edited

thejcannon commented Nov 3, 2022 •

edited