Cool 🙂 Thanks

In the mean time, I will be hunting down the other places that we manually populate sources 🙂 I see many deprecations in my future

@witty-crayon-22786 FWIW it’s hard to retrofit “snapshot all the things if they’re not already snapshotted” because there isn’t one place that happens… When we set

sources

, we don’t currently have a

context

(and it’s a public API so I can’t just add one - if these weren’t public APIs, this would be much easier). We could do the snapshotting at the call-site, but that’s what we did for cloc, and is what I’m trying to stop doing with this work 🙂

Yea, I was thinking callsite. There should be very, very few targets for which you have to do it. Not a long term solution obviously, but

Yeah, my plan is for @gorgeous-alligator-94763 to do the hacky callsite ingesting, and then for me to swap in a more efficient implementation with this work

👍

Should I take that comment as a "this is a reasonable approach, write those tests" or a "if you wanted to write those tests, this is how you would, buuuut..."?

@average-vr-56795: hm?

Sent that when you'd just made the "here's how to do testing" comment without the rest of the discussion about the PR - follow up comments make it clear! Thanks!

...i still don't know what that is, but ok!

@aloof-angle-91616: hey! hope you're feeling well

better — just got your notif

from some offline discussion: one thing we think should probably happen before landing https://github.com/pantsbuild/pants/pull/5815 is something like https://github.com/pantsbuild/pants/issues/5920

because there is some definite risk of low OSX coverage given that you're developing on linux and we don't run any tests on osx shards right now

i’m switching to the osx laptop every so often to make sure there’s parity — but with you on the rationale

if I have paths to files that I want to read contents for via the engine, what’s the best product to request for that? can that handle files outside of the build root via absolute paths?

it cannot handle absolute paths, no. those would need to come in from outside.

hm

which files are you trying to read?

pants config files

@rough-minister-58256: bootstrap completely expands them, right?

yeah, but does I/O. I’ve inverted that to a coroutine that yields file paths and receives file contents to load w/ an

@rule

adaptor, but the no absolute paths throws a bit of a wrench into things for e.g. ~/.pantsrc. may need to regroup.

could punt on absolute file loading via an error or something for now, then circle back to adding absolute file support to make that work end to end - or regroup to fetch those file contents outside of the engine, but it’s a little tricky since config loading is a two stage process: load initial known configs, parse options w/ those, which reveal other configs to load, reparse with those etc.

should’ve considered the absolute path bit sooner

FilesContent

is read from a

Snapshot

, and if you try to convert a

PathGlobs

pointing outside the build root into a

Snapshot

, we throw an error. with no further context, to me it seems that if there's an obvious way to represent file paths that originate outside the host build root in a way that we can store and e.g. pass to a remote process, we could remove that restriction. one way to do that might be to convert paths outside the build root into a new subdirectory of

.pants.d

when storing them -- then, reifying the files would just insert them into that subdirectory, as if they were there in the first place. the unknown on that is what that path conversion process entails (mangling?)

so basically absolute file loading has an error with a descriptive message, so you can't accidentally do it afaik

hm, yeah thats an interesting idea.

I wonder if a more transient variant of

FilesContent

would also make sense that could directly read files from disk in this case as dead simple engine-handled-I/O?

we cannot safely snapshot files outside the buildroot because we do not currently have a way to watch them for changes

note ‘transient’