please icon indicating copy to clipboard operation
please copied to clipboard

Possible correctness problem: track outputs

Open burdiyan opened this issue 3 years ago • 5 comments

It seems like Please doesn't recognize situations when the output of a given target is modified from the outside after the build. I know this is an extremely weird thing to happen, but it can bite hard when using codegen targets and LinkGeneratedSources config option.

So if you accidentally modify the linked output, then Please will not warn you about it, and will silently skip the target during next build, which may lead to correctness issues. And when caching is enabled, even removing the output doesn't help, Please would still create the modified version of a file. The only way out of it is to force the build with --rebuild, but you have to know that you're facing this problem (which is quite hard if someone else accidentally modified the file).

I would expect Please to automatically recognize these situations and either issue a warning message, or just rebuild the target when this happens. Maybe with an opt-in configuration parameter.

burdiyan avatar Dec 28 '22 20:12 burdiyan

This issue has been automatically marked as stale because it has not had any recent activity in the past 90 days. It will be closed if no further activity occurs. If you require additional support, please reply to this message. Thank you for your contributions.

stale[bot] avatar Apr 07 '23 02:04 stale[bot]

Bump

burdiyan avatar Apr 10 '23 10:04 burdiyan

Sorry, I think this one got missed. This is a shortcoming of our caching setup, where we use hard links to improve performance, so if they're all pointing to the same inode. Changing the file will change the cached artefact. Copying files around would have huge perf impacts on filesystems that don't support copy-on-write so I'm hesitant to do so... Running plz clean will fix this, so you don't have to --rebuild however I don't like suggesting that as a solution to these sorts of problems. As you say, Please should be able to detect this.

I think we would need to investigate the performance impact of re-hashing the output of the file before using it but I expect that's going to be huge...

Tatskaari avatar Apr 11 '23 13:04 Tatskaari

We should be able to store the last-modified time of the inode when writing the attributes, and check this hasn't changed when checking the hash actually. :bulb:

Tatskaari avatar Apr 11 '23 13:04 Tatskaari

Right, I think at all times when you need to hash a file to check for "dirtiness" of a target, it could be useful to check the modification time first, and then fallback to hashing the entire file. With this little trick, I assume doing it for outputs shouldn't be a big issue. At least for those that are code generated and linked into the source tree.

burdiyan avatar Apr 11 '23 15:04 burdiyan