LPGhatguy / aftman

Aftman, the prodigal sequel to Foreman
MIT License
162 stars 19 forks source link

fix install from failing with different /tmp filesystem #36

Open Joshument opened 2 years ago

Joshument commented 2 years ago

fixes #35

This changes the fs_err::rename() function from a (likely unintended) use case to fs_err::copy() instead, which supports multiple filesystems in an operation.

LPGhatguy commented 2 years ago

So this line is intentionally a move, which the comment hints at.

On some platforms, at least Windows, moving an executable allows the original file path to be overwritten. If that's changed to a copy, updating an executable while it's running will no longer work. That's an important feature that we need to preserve!

Joshument commented 2 years ago

I've updated the pull request to take a different approach to the problem. Instead of using /tmp, it creates it's own temp directory using ~/.aftman/tmp (on Unix). However, this does include a fs_err::remove_dir_all(), so I would be a little careful just to make sure that I did not do anything that would cause a major issue, though I tested it on my device and it works fine.

Joshument commented 2 years ago

Hi, I'm just checking in on if I can get an update on this fix, as it makes the program unusable for me if I want to use it with the rojo vscode extension (which I'm gathering pulls from somewhere out of my control).

LastTalon commented 2 years ago

I'm not really involved in this, but having looked at the problem and solution I think that a better overall solution probably involves doing both. Creating our own separate tmp directory seems incorrect to me. In fact, that sort of flies in the face of being able to control your filesystem's tmp directory (such as with tmpfs), since we're just ignoring it entirely then.

Instead, it seems like we're using tmp correctly in the first instance, but the problem comes when trying to move it out of tmp. What seems more correct to me would be to copy it to the bin directory (most likely with a temporary name), then move that (within the same directory, unless you're doing a lot of messing around this will be on the same partition and filesystem) to its correct name.

That's just my first impression take here. Looking for feedback on that solution.

Joshument commented 2 years ago

the problem is that the temporary directory cannot be used at all if we want the executable to be moved around. I'm not really sure how to incorporate the temporary filesystem in this situation if it's just not possible to move between file systems.

LastTalon commented 2 years ago

Right, this is why I suggested first copying it from one filesystem to the other within the directory its meant to be i.e. ~.aftman/bin, using a temporary filename e.g. mybin.tmp. This directory should always be on the correct filesystem and should allow moving the newly copied file to the currently running executable no problem.

LPGhatguy commented 1 year ago

One nasty hangup with maintaining our own tempdir or with renaming the running executable is that we can't manually cleanup Aftman's own old executable from within Aftman on Windows. The final call to remove_dir_all breaks Aftman on Windows as a result.

I've been thinking about this problem on and off. I think the only way to solve this problem well is likely to have a #[cfg(windows)] implementation that does what Aftman does today, and a #[cfg(not(windows))] that copies the files. We should make the platform-specific bits be as small as possible.

Joshument commented 1 year ago

Good point, the original implementation works better if it's doable. If I have some time I'll see if I can rewrite it again.