Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Copy package to destination if move failed during resource installation #1938

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

Merged
cmaglie merged 1 commit into arduino:master from RangerCD:feat-copy-if-move-failed
Oct 24, 2022

Conversation

Copy link
Contributor

@RangerCD RangerCD commented Oct 22, 2022

Please check if the PR fulfills these requirements

See how to contribute

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)

What kind of change does this PR introduce?

This PR improves compatibility with antivirus software.

What is the current behavior?

CLI reports

Installing Seeeduino:CMSIS@5.7.0...
Error during install: Cannot install tool Seeeduino:CMSIS@5.7.0: moving extracted archive to destination dir: rename C:\Users\XXX\AppData\Local\Arduino15\tmp\package-2842439214\ARM.CMSIS.5.7.0 c:\Users\XXX\AppData\Local\Arduino15\packages\Seeeduino\tools\CMSIS5円.7.0: Access is denied.

if an antivirus software is still scanning temp files when CLI trys to move them to destination dir. This is a common error if you are using an antivirus software, and there are lots of discussion.

All previous suggestion is to disable antivirus temporarily. It's not a good solution because:

  • Disabling antivirus is risky, even just for a couple minutes to install a package, or you might forget to enable it again
  • User might not allowed to disable antivirus, due to security policy, especially in a lab

Some related issues, all suggested to solve manually:

What is the new behavior?

CLI will try to copy temp files if move failed, no error will be reported even if antivirus is scanning temp files.

To demonstrate new behavior, I've added debug logs in my demo and the output looks like:

Installing Seeeduino:CMSIS@5.7.0...
+Failed to move dir
+Copy succeeded
Seeeduino:CMSIS@5.7.0 installed

Does this PR introduce a breaking change, and is titled accordingly?

No

Other information

Copy takes a little bit more time than move, but it's only called if move failed. And it's definitely faster and safer than searching for error message, disabling antivirus, and rerun installation.

Temp files will not be left permanently, they will be removed by defer tempDir.RemoveAll() here.

per1234, cmaglie, and aliphys reacted with heart emoji
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Oct 22, 2022
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

Thank you for the patch and for the clear explanation, I never understood the reason for these random Access denied errors, now it's clear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmaglie cmaglie cmaglie approved these changes

Assignees
No one assigned
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /