https://pantsbuild.org/ logo
Join Slack
Powered by
Picking symlinks back up. Looks like there's possi...
# development
b
Picking symlinks back up. Looks like there's possibly new code to have to handle. so @witty-crayon-22786 @fast-nail-55400 Qs: • 
Paths.store_paths
-> I'm not sure if 
Paths
should be symlink aware/oblivious • 
<http://directory.rs:paths_of_child_dir|directory.rs:paths_of_child_dir>
-> Not sure what the method does, exactly
w
• 
Paths.store_paths
-> I’m not sure if 
Paths
should be symlink aware/oblivious
oblivious, i think. it’s essentially just supposed to be “`Snapshot` but without capturing content”
b
That was my assumption
Bummer to implement 😅
w
hm… PathGlob expansion should give you back a stat and a full path, so i think it’s basically “use the type information from the Stat, but the actual name from the path”
looking at 
paths_of_child_dir
b
I guess for 
store_paths
the question is, is a symlink a file? or is it the dir/file it points to (which we can't know so its nothing?)
w
• 
<http://directory.rs:paths_of_child_dir|directory.rs:paths_of_child_dir>
-> Not sure what the method does, exactly
it looks like it selects the children of a given name from a flat list… i.e., if you had a list of 
['a/1.txt', 'a/2.txt', 'b/3.txt']
and called it with 
a
, it would give you 
['1.txt', '2.txt']
I guess for 
store_paths
the question is, is a symlink a file? or is it the dir/file it points to (which we can’t know so its nothing?)
um, i don’t think so… PathGlobs expansion gives you back a PathStat, which has everything you need: see above
…basically, if a PathStat is a Dir, that means that the final destination of the symlink (if any) was a dir. the 
path
of the pathstat was its “symbolic” path in the filesystem.
so you want to render the symbolic path, using the type of the PathStat
b
I'm having trouble fitting it all in 🧠 Looking at the code now
After glob matching, would any of the stats even be a 
link
?
w
as it stands: no. a 
PathStat
cannot represent a symlink
i think that we did discuss potentially needing two modes of PathGlobs expansion: 1. one which was aware of symlinks, and thus rendered PathStats which never contained symlinks (because it has already expanded them) 2. another which actually returned a new PathStat variant which was a link, rather than expanding
(1) is what we always do today.
b
gahh I wish I didn't put this down. I vaguely remember that, but don't remember why we landed there
w
yea, don’t take that as “definitely correct”. i think i remember discussing it, but not sure.
assuming that we continued to use that first mode for capturing the sources fields of targets, we would remain symlink oblivious within the user’s workspace
and if we only used the second mode inside of sandboxes… etc?
b
I think that's ideal
w
from a type perspective, it would probably be easiest for this to remain a single method 
fn expand
which returns 
PathStats
, but to essentially (be able to) assert that the resulting stats didn’t contain any links if you called it in mode 1
but maybe two methods makes sense. unclear.
😵 1
but also: that boundary is where i think it makes sense to break in terms of PRs: if you can avoid touching PathGlobs expansion in your first PR, that would be safest i think.
(i guess that the challenge that you had was in getting test data for that…? but still…)
…oh. or did we decide that you could use 
CreateDigest
to make test data?
b
that I do remember! yes, for now
w
k, yea.
b
w
hah, yes. whether it’s true is another question, but
b
So I think I can, for now, toss 
PathStat::Link
w
yes. next PR
b
things are getting clearer
Oh I guess it's needed, because path stats are how Digests are created (via 
CreateDigest
)
w
i thought that you had moved that to TypedPath
😵 1
b
Ah `.into`: 
DigestTrie::from_unique_paths(path_stats.iter().map(|p| p.into()).collect(), &file_digests)?;
Welp there's a lifetime issue 😕
TypedPath
needs a lifetime of the 
items
var, but we 
async move
the block which constructs the 
Trie
. I'm assuming the async 
store_file_bytes_batch
has to occur before trie creation?
w
um… i’d need to look at it.
b
Yeah I'm signing off anyways. We're closer though 😈
w
thanks! have a good one. feel free to post it if you have an error and i can try to fix it.
❤️ 1
b
https://github.com/pantsbuild/pants/pull/16844 (and therefore 
thejcannon:symlinks
) has it. note I moved the trie creation out, but had to use 
.unwrap()
w
pushed a fix: essentially, rather than creating a reference to each item as you consumed 
items
, you needed to iterate over references into the list: https://github.com/pantsbuild/pants/pull/16844/commits/9859cd8767707d8811643e48d1483a9a0c264729 … think of the list of 
items
as sitting on the stack, and 
for item in &items
as iterating over references to the existing list in place, while 
for item in items
consumes 
items
, such that each 
item
“moves into” the body of the 
for
loop (and is dropped at the end of each iteration)
(
bytes.clone()
is cheap here, because 
Bytes
is reference counted)
b
Ah yeah that makes sense!
w
clippy may yell, because i think that 
&path
is redundant, since it will already be a reference.
b
Thanks! Will pick up tomorrow
And yeah, I "understood" moving in C++, so conceptually it makes sense to me. However the nicer Rust-style of "moving" still isn't noticeable by default.
Open in Slack
PreviousNext
Powered by