fix(pkg): Create temporary directories on same FS

[Leonidas-from-XIV]

The problem in #10213 is that Temp.create creates a temporary directory and then Path.rename is used to move the files to the actual target directory. This unfortunately only works if the directory that Temp.create picks and ~target are on the same file system.

Multiple solutions exist:

1. Attempt to write to the same filesystem, e.g. by hoping that the parent directory of target is also on the same file system as ~target. This isn't necessarily true, but given we write to _build it should be true in most cases.
2. Write to a temporary directory but then copy the files manually. This makes the whole operation non-atomic and incurs potentially a lot of file I/O

This PR attempts #1. An unfortunate consequence of this is that Path.mkdir_p is necessary to create the parent directory and if it didn't exist, in the case of failure it is left behind and not deleted. We could probably solve this by keeping track of directories that are created in such case, if it proves to be an issue. Input welcome!

Fixes #10213

[rgrinberg]

Wouldn't it be possible to use the same trick that I did for tar? Create the temporary directory inside the build directory if the target is inside the build directory. I think that should be enough.

[Leonidas-from-XIV]

Yes, that's a good solution, I'll rework it.

[Leonidas-from-XIV]

I've changed it in the places where this was possible but for Fetch the target can be anywhere, so assuming _build and the target are on the same FS isn't really possible.

I dislike duplicating temp_dir_in_build, maybe it should go in Temp and add some atexit hook to be deleted?

[rgrinberg]

I've changed it in the places where this was possible but for Fetch the target can be anywhere, so assuming _build and the target are on the same FS isn't really possible.

Yes, they're not always going to match, but we can just check if target is in the build dir. That should be good enough in almost all cases.

I dislike duplicating temp_dir_in_build, maybe it should go in Temp and add some atexit hook to be deleted?

You can create a new module for it in dune_pkg. Note that there's no need to add a hook manually - there's one added automatically.

By the way, I think we are creating too many temporary directories for some of these jobs. For example, we should reuse a temp directory for downloading and untaring.

[rgrinberg]

Is this ready for review now?

[gridbugs]

@Leonidas-from-XIV Just making sure we don't forget about this change. This is necessary for package management to work on machines where /tmp is a separate fs. I need to manually apply this patch whenever I want to test dune package management on my machine.

[Leonidas-from-XIV]

Yes, sorry, I've been busy with other stuff. I just looked through it and I think it can be reviewed. Just rebased it on main.

By the way, I think we are creating too many temporary directories for some of these jobs. For example, we should reuse a temp directory for downloading and untaring.

Possibly, but creating temporary directories is cheap (1 inode) and fast and the PR as-is solves the failure I get on every dune runtest. Optimizing the amount of temporary directories could be done in a future PR, without breaking anything.

[rgrinberg]

I think this now assumes that target always lives in Path.Build.t? We should do one of the following:

1. changing target to be Path.Build.t
2. assert that this is the case
3. revert back to the old code for handling non build directories.

#3 would be my preference as we need code like this in other parts of dune.

[Leonidas-from-XIV]

Good point, there was indeed an issue where it would write to the wrong place. I changed it to check for the build folder and only create it in the build folder in that case. However in the other case we cannot use Temp.create with rename as Temp.create will (on my system) always create in /tmp and it will always fail unless the target is in /tmp as well.

So I used the "subdir of parent dir" approach here as well.

[rgrinberg]

LGTM. I just made some minor adjustment to reduce repetitive code.