Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix install from failing with different /tmp filesystem #36

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Joshument
Copy link

@Joshument Joshument commented Oct 2, 2022

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.

@Joshument Joshument closed this Oct 2, 2022
@Joshument Joshument reopened this Oct 2, 2022
@LPGhatguy
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

LastTalon commented Oct 23, 2022

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
Copy link
Owner

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aftman self-install fails when /tmp is on a seperate filesystem
3 participants