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

Rework MAD cleanup handles #817

Merged
merged 16 commits into from
Dec 6, 2024

Conversation

Alystrasz
Copy link
Contributor

@Alystrasz Alystrasz commented Sep 7, 2024

Changes

  • Mod is not marked as successfully installed when the process failed;
  • When mod downloading/extraction fails, mod directory is removed.

Testing

  1. Install this PR;
  2. Enable MAD (allow_mod_auto_download 1);
  3. Try to join Space battle server and cancel mod downloading/extraction;
  4. Check your local profile, the remote mod should not appear in the runtime/remote/mods directory;
  5. Check your temporary directory (for me on Windows, it's C:\Users\Remy\AppData\Local\Temp), there should be no mod archive.

Closes #756.


TODOs
  • ZIP archive is not deleted when download is cancelled (but it is when cancelling extraction)
  • Mod is not removed when cancelling archive extraction

@github-actions github-actions bot added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Sep 7, 2024
@Alystrasz
Copy link
Contributor Author

Waiting for #815 to be merged so we have a simple way to cancel MAD process.

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 7, 2024
@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 7, 2024
@GeckoEidechse GeckoEidechse added the MAD Related to mod-auto-download label Sep 7, 2024
@Alystrasz Alystrasz added the depends on another PR Blocked until the PR it depends on is merged label Nov 12, 2024
@wolf109909
Copy link
Contributor

Tested on NorthstarCN v1.18 branch and it works fine.
Scenario 1: Unplugging ethernet while mod download is happening: Prompted mod verify or download error.
Scenario 2: Trying to join the server without ethernet connection: Immediately prompted mod verify or download error.
Scenario 3: Joining the server with Ethernet reenabled: Verify and download begins normally, with the download state being reset after the first download state update.
Overall pretty robust, but my internet restricts me from doing more stressful tests due to unstable connection speed to thunderstore and github without a proxy. (literally failed to download once even without touching my network, and the error was handled just fine ! )

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Nov 22, 2024
@Alystrasz Alystrasz marked this pull request as ready for review November 22, 2024 21:11
@Alystrasz Alystrasz added almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge and removed needs testing Changes from the PR still need to be tested merge conflicts Blocked by merge conflicts, waiting on the author to resolve depends on another PR Blocked until the PR it depends on is merged labels Nov 22, 2024
@Alystrasz Alystrasz marked this pull request as draft November 22, 2024 21:45
@Alystrasz Alystrasz marked this pull request as ready for review November 22, 2024 23:43
@Alystrasz Alystrasz requested a review from RoyalBlue1 November 23, 2024 01:56
Copy link

@NachosChipeados NachosChipeados left a comment

Choose a reason for hiding this comment

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

Mod directory doesn't seem to get removed when cancelling extract
https://github.com/user-attachments/assets/1932a97b-5df1-4fef-90fa-d0ab6940ae01

Edit: turns out there was an extra Northstar.dll in my profile folder and it was messing with my testing. See the updated review below

Copy link

@NachosChipeados NachosChipeados left a comment

Choose a reason for hiding this comment

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

Works fine, both the archive and the mod folder get deleted during the download/extract processes

2024-11-28.20-13-35.mp4

Copy link

@NachosChipeados NachosChipeados left a comment

Choose a reason for hiding this comment

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

I think the code looks good

@Alystrasz Alystrasz added READY TO MERGE This mergeable right now and removed needs code review Changes from PR still need to be reviewed in code almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Dec 6, 2024
@GeckoEidechse
Copy link
Member

I don't have the opportunity rn to give this is a proper review. I trust @Alystrasz with the suggested change here, especially as @NachosChipeados confirmed it working in testing.

As such merging rn without further reviews.

@GeckoEidechse GeckoEidechse changed the title fix: Rework MAD cleanup handles Rework MAD cleanup handles Dec 6, 2024
@GeckoEidechse GeckoEidechse merged commit feab262 into R2Northstar:main Dec 6, 2024
4 checks passed
@Alystrasz Alystrasz deleted the fix/mad-cleanup-handles branch December 8, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MAD Related to mod-auto-download READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Issue on mod download failure
4 participants