https://pantsbuild.org/ logo
Powered by
b

bitter-ability-32190

10/18/2022, 7:33 PM
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

witty-crayon-22786

10/18/2022, 7:34 PM
• 
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

bitter-ability-32190

10/18/2022, 7:34 PM
That was my assumption
Bummer to implement 😅
w

witty-crayon-22786

10/18/2022, 7:35 PM
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

bitter-ability-32190

10/18/2022, 7:38 PM
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

witty-crayon-22786

10/18/2022, 7:38 PM
• 
<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

bitter-ability-32190

10/18/2022, 7:48 PM
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

witty-crayon-22786

10/18/2022, 7:51 PM
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

bitter-ability-32190

10/18/2022, 7:53 PM
gahh I wish I didn't put this down. I vaguely remember that, but don't remember why we landed there
w

witty-crayon-22786

10/18/2022, 7:54 PM
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

bitter-ability-32190

10/18/2022, 7:55 PM
I think that's ideal
w

witty-crayon-22786

10/18/2022, 7:57 PM
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

bitter-ability-32190

10/18/2022, 7:59 PM
that I do remember! yes, for now
w

witty-crayon-22786

10/18/2022, 7:59 PM
k, yea.
b

bitter-ability-32190

10/18/2022, 8:00 PM
w

witty-crayon-22786

10/18/2022, 8:00 PM
hah, yes. whether it’s true is another question, but
b

bitter-ability-32190

10/18/2022, 8:01 PM
So I think I can, for now, toss 
PathStat::Link
w

witty-crayon-22786

10/18/2022, 8:02 PM
yes. next PR
b

bitter-ability-32190

10/18/2022, 8:02 PM
things are getting clearer
Oh I guess it's needed, because path stats are how Digests are created (via 
CreateDigest
)
w

witty-crayon-22786

10/18/2022, 8:15 PM
i thought that you had moved that to TypedPath
😵 1
b

bitter-ability-32190

10/18/2022, 8:17 PM
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

witty-crayon-22786

10/18/2022, 8:30 PM
um… i’d need to look at it.
b

bitter-ability-32190

10/18/2022, 8:30 PM
Yeah I'm signing off anyways. We're closer though 😈
w

witty-crayon-22786

10/18/2022, 8:33 PM
thanks! have a good one. feel free to post it if you have an error and i can try to fix it.
❤️ 1
b

bitter-ability-32190

10/18/2022, 8:44 PM
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

witty-crayon-22786

10/18/2022, 9:38 PM
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

bitter-ability-32190

10/18/2022, 9:51 PM
Ah yeah that makes sense!
w

witty-crayon-22786

10/18/2022, 9:53 PM
clippy may yell, because i think that 
&path
is redundant, since it will already be a reference.
b

bitter-ability-32190

10/18/2022, 10:01 PM
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.
Powered by