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

feat: allow self renamer to work if tempdir is on a different device #797

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

Conversation

davidkna
Copy link

Standards checklist:

  • The PR title is descriptive.
  • I have read CONTRIBUTING.md
  • The code compiles (cargo build)
  • The code passes rustfmt (cargo fmt)
  • The code passes clippy (cargo clippy)
  • The code passes tests (cargo test)
  • Optional: I have tested the code myself

For new steps

  • Optional: Topgrade skips this step where needed
  • Optional: The --dry-run option works with this step
  • Optional: The --yes option works with this step if it is supported by
    the underlying command

Currently, the self renamer fails with the following error if the tempdir is on a different device from the system temporary directory:

Error:
   0: The system cannot move the file to a different disk drive. (os error 17)

This PR works around this by using a temporary file in the binary directory instead in that case.

If you developed a feature or a bug fix for someone else and you do not have the
means to test it, please tag this person here.

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 2.29%. Comparing base (4d66431) to head (b4372ee).

Additional details and impacted files
@@           Coverage Diff            @@
##            main    #797      +/-   ##
========================================
- Coverage   6.81%   2.29%   -4.52%     
========================================
  Files         37      30       -7     
  Lines      11820    4566    -7254     
========================================
- Hits         806     105     -701     
+ Misses     11014    4461    -6553     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@SteveLauC SteveLauC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for your contributing to Topgrade!

I wonder can we do std::fs::copy(); std::fs::remove_file() here?

@davidkna
Copy link
Author

I wonder can we do std::fs::copy(); std::fs::remove_file() here?

It would be possible to use this, but std::fs::rename() is something of a atomic operation which helps avoid issues around concurrent access and possibly mangling files.

@SteveLauC
Copy link
Member

Yeah, that is one benefit of rename().

May I step back a little bit, and ask you what is the functionality of this SelfRenamer, I have never used the self-update feature and I am not the author of that code.

From my understanding, the renamer itself is simply doing a round-trip, I do not quite get why it is necessary on Windows.

@davidkna
Copy link
Author

@SteveLauC The self-renamer is supposed to make self-upgrades on Windows work because executable files are locked while they are running and cannot be changed. That likely means that remove_file would not work in this case.

Looking at the surrounding code, this doesn't clean up the old executable on exit in the case of upgrade. With this new case using the binary directory, leaving the temporary executable in-place should be avoided, so I will convert this PR to a draft.

@davidkna davidkna marked this pull request as ready for review July 27, 2024 11:03
@davidkna
Copy link
Author

davidkna commented Jul 27, 2024

Executable cleanup is now implemented via the self-replace crate that was already a transitive dependency via self_update.

The clippy issues seem to be unrelated, but I can add fixes for those if preferred.

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

Successfully merging this pull request may close these issues.

2 participants