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

[Ref/Fix] Improving checkDiskSpace #3440

Merged
merged 4 commits into from
Mar 14, 2024
Merged

[Ref/Fix] Improving checkDiskSpace #3440

merged 4 commits into from
Mar 14, 2024

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Jan 15, 2024

Our checkDiskSpace IPC function had a couple issues in edge-case scenarios, which could lead to Heroic freezing. Those are now resolved

To reproduce the issue (in the current Heroic release)

  1. Use Heroic on Windows
  2. Specify a nonexistent path in the install dialog

It'd seem that an issue like this was also discovered on Linux by a user on our Discord (here). I can't reproduce this myself, but it might have the same root cause as the Windows issue


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

This is going to be used outside Legendary in the next commit
Doing this within Heroic allows us to use already-established patterns
(dynamic import, helper functions like `genericSpawnWrapper`, validation
using Zod)
This was a little overcomplicated, and lead to issues on Windows (where
you don't necessarily have an existing root folder)
@CommandMC CommandMC requested review from a team, arielj, flavioislima, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team January 15, 2024 10:37
@CommandMC CommandMC self-assigned this Jan 15, 2024
@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Jan 15, 2024
@flavioislima
Copy link
Member

flavioislima commented Jan 29, 2024

Just a follow-up for this one. Currently the isWrittable won't work on windows due a nodejs limitation there:
nodejs/node#34395

One workaround I found in the past was to write a method that tries to write a test.txt file on the folder and returns true or false with the return. That should do it.

the problem is that then we need to have two methods, but its fine. Or use only this test write file on all platforms, should work as well.

- As Flavio mentioned, `access` doesn't seem to work on Windows, so I
  replaced that with some PowerShell commands that check the same thing
- We have to call isWritable with the full path, as you can of course
  modify permissions on any folder, not just on a root directory/mount
  point

Since this info is now always accurate, we might want to make the
respective warning on the Frontend an error instead
@CommandMC CommandMC force-pushed the ref/check-disk-space branch from 93f5e80 to 49fc1bd Compare January 31, 2024 09:59
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

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

lgtm

@CommandMC CommandMC merged commit fcd30b4 into main Mar 14, 2024
9 checks passed
@CommandMC CommandMC deleted the ref/check-disk-space branch March 14, 2024 21:24
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants