search

mrc-ide / orderly2

https://mrc-ide.github.io/orderly2/
Other
7 stars 2 forks source link

Mark files as read-only when copying them into the archive. #141

Closed plietar closed 4 months ago

plietar commented 4 months ago

It is easy for a user to inspect a packet's result by opening it up in their editor and accidentally edit and save the file. If they didn't have a file store and no other packet contains a copy of the file then the packet may be irremediably corrupted.

Marking the file as read-only is an easy way to prevent this. Of course a user can always force their way around it, but this should at least prevent most accidental occurrences.

A side-effect of this change is that file copied out of the archive will now be read-only as well, since copying the files preserves the access mode. This is desirable when copying files from a dependency, but may not be when copying outside of a packet context. If this causes problems we can introduce some option to orderly_copy_files to add the write bit back on files it copies.

Files that were put into the file store were already being marked as read-only, so this change makes the two implementations more consistent. The executable bit was being cleared as well, which seems unnecessary to me and in some contexts even wrong (a user may well want to include a bash script in their report). I've changed the chmod operation from "a-wx" to "a-w". This doesn't matter on Windows, where executable bits don't exist.

A few places around the codebase were using unlink(recursive=TRUE) to remove directories, which on Windows fails silently if the directory contains read-only files. One example of this was the temporary draft directory for a packet, which would not be deleted on completion of a run if a file from a dependency had been copied in. While unlink has a force option to change the permissions before deleting, I've decided to use fs::dir_delete instead, which always allows deleting read-only files, and which does not fail silently. For consistency I've replaced all the uses of unlink, even some that we don't expect to be removing files from the archive or file store.

plietar commented 4 months ago

Looks like Windows is genuinely failing tests for me, not just #142. Will look into it.