"Modifiable" input files for incremental builds

[ag-eitilt]

Much as we'd like to have Wake be the only build system managing compilation, that's not a realistic assumption to make when working with languages that have strong ecosystems. In those cases, a solution for passing multiple files at once to a build tool and having that tool determine which need to be recompiled is often optimal, but Wake's sandboxing prevents that determination from occurring. This allows Runners to selectively poke holes in the sandbox to pick up artifacts from previous compilation, and to write to those same files if the tool determines the artifact needs to be recompiled. Crucially, that's only exposed at the level of the Runner, and user-facing Plans don't have any way of adding modifiable files -- the intent is to allow defining an incremental runner for each tool (a la incrementalCRunner for C+Make in the test¹), but not to allow this knob as a free-for-all footgun.

Additionally, the PR properly marks files in the previous visible list as not being writable within the sandbox, regardless of their permissions outside it. That is the existing behaviour as enforced by the FUSE implementation, and why simply having the Runners adding files to RunnerInputs.Visible rather than the new RunnerInputs.Modifiable wasn't sufficient.

Side note: I wouldn't have had any hope of finding the second tier of sandbox file in 289bdc1 if it weren't for the AI assistant.

I'd still want to go back through before merging this to add documentation, double-check the generated C test code meets my coding style, etc., but I wanted to get this posted first to get discussion going; if we determine that this is too unsafe, I wouldn't want to have to throw out all the time to polish it up.

Footnotes

1. C+Make isn't actually something that should be implemented using this method; that setup, at the least, is well-served by just using Wake's caching -- better-served, actually, since it enables hash-based recompilation rather than timestamp-based. It is, however, a very simple model to build a test around. ↩

[ag-eitilt]

Most of the existing runners seem fine enough ignoring the modifiable inputs. This along with mkJobCacheRunner are the pair I'm most concerned about. So long as the invariant that the same outputs are produced from the same inputs holds up, it should be fine to cache, but it feels a bit too easy to have a buggy/malicious artifact slip through and poison the cache.

[AbrarQuazi]

Don't have any issue with this first pass for implementation, other than some questions. I don't think that this is an unsafe change, and am ok with you going further along with this, as I see this as a value add to what the sandbox should be allowed to do (ignoring or writing to incremental compile collaterals) as long as we properly gate what gets to be defined as a Modifiable input file

[AbrarQuazi]

Is there any guarantees for this? What prevents someone from adding a file that affects the outputs?

[AbrarQuazi]

One question I have is, how are we handling caching of individual steps of an incremental compilation? Do we want to cache these individual steps, or only cache the final output?

[ag-eitilt]

Is there any guarantees for this? What prevents someone from adding a file that affects the outputs?

Entirely down to this comment. This is an instruction to whoever's writing the Runner about what they need to be sure of themselves, rather than a description of any internal processes that get run for them.

One question I have is, how are we handling caching of individual steps of an incremental compilation? Do we want to cache these individual steps, or only cache the final output?

If I'm reading the question correctly, I think the "incremental" wording is maybe slightly misleading. So far as Wake is concerned, there's only one layer of output, which should be cached or not according to the standard decision tree. The "incremental" comes in when the Job gets run later on as part of a separate invocation,¹ at which point the only difference is that this field allows pre-populating the FUSE sandbox with files without requiring they be read-only within the sandbox, nor that they be tracked in the job_hash. After the second instance of the Job returns, it's processed and cached identically to the first instance.²

Footnotes

1. Or, technically, it might be part of the same invocation if the Plan specifies ReRun. That's obviously not going to have an impact on caching. I'll have to look at how well it deals with that case; I think the answer is "just as poorly as if a non-incremental Job gets run multiple time at once", but it's worth actual exploration. ↩

2. I know I was planning to enforce that anything in the modifiable list avoids output conflicts by removing anything listed here from the output-paths list before handing it over to the Plan's FnOutputs. I'm pretty sure that made it in, but I can't remember whether it actually did or not, and meanwhile the test filters them out manually. Another thing to look into, and which will need fixing to at least some degree, even if it's just removing an extraneous editRunnerOutputOutputs. ↩

[AbrarQuazi]

What is the behavior like before?Did all non-writeable (readonly) files have write permissions? That seems odd if true.

[ag-eitilt]

Writes were always being blocked; I don't have the error message handy any more, but was something about "permission denied", and I'm pretty sure it was just the standard one you'd get from any write-protected file. This was entirely fixing a rendering bug along the lines of

$ ls -l input.ext 
-rw-r--r-- 1 root root 0 Sep 23 15:39 input.ext
$ echo test >> input.ext
Permission denied
$ whoami
root

[AbrarQuazi]

Can you explain why we need to remove the files_modifiable from the visible list, specifically how it breaks idempotency if we keep it? As far as I know, these modifiable files are not considered when hashing, so it doesnt break our determinism

[ag-eitilt]

Good question. I'll need to try to remember why that was; this might be a remnant of an earlier effort where I was trying to treat them as additional inputs internally, rather than this final form where it turns out they work better if you treat them as if they're files that got written by the Job in the process of setting up the sandbox.

I think the code's good since it's following the same model as the files_wrote erasures, but the comment can definitely be improved -- though it's actually maybe redundant depending on the relation between files_modifiable and files_wrote earlier on.

[AbrarQuazi]

this function is used to help remove directories from fuse right? So incremental build files may exist from a previous run, but we need this to allow wakefuse to allow it to unlink and succeed. With the incremental file that is left over, is Wake responsible for cleaning that up or the other build tool?

[ag-eitilt]

I should probably have a test targetting this. I don't think the other build tool comes into play here, but I've lost whatever knowledge I gleaned from this in context. Another place where it would really help if the code was properly commented (ironically despite the fact that it does have a comment -- the only new info that gives us is that this function is somehow connected to deletion and to is_writeable).