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.

Sign up for GitHub

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

Jump to bottom

callRunner: Use PowerShell to run commands on Windows #3006

Merged
merged 4 commits into from
Feb 6, 2024
Merged

callRunner: Use PowerShell to run commands on Windows #3006

merged 4 commits into from
Feb 6, 2024

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Aug 23, 2023 •
edited
Loading

PowerShell's Start-Process cmdlet & its -Wait parameter are used to wait for a process tree to exit. This means we can now accurately track the "Playing" status of games on Windows.

TODO:

  • Since we now modify commandParts, hiding login tokens in getRunnerCallWithoutCredentials no longer works.
    Adding a check like if (runnerPath === 'powershell') should work, although I don't exactly like this solution

Closes #2046

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)

@CommandMC CommandMC added the pr:wip WIP, don't merge. label Aug 23, 2023
@CommandMC CommandMC self-assigned this Aug 23, 2023
flavioislima
flavioislima approved these changes Sep 4, 2023
View reviewed changes
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

@arielj
Copy link
Collaborator

arielj commented Oct 13, 2023

can we merge this? I don't use windows so I have no idea, are we waiting for more tests on windows?

@CommandMC
Copy link
Collaborator Author

For one, the TODO in the PR description still needs to be addressed.
Other than that, I think the backticks for the case that the argument includes a space aren't quite correct

@CommandMC CommandMC force-pushed the feat/playing-windows branch from ef8bfb3 to f806f30 Compare November 7, 2023 00:43
@CommandMC CommandMC added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:wip WIP, don't merge. labels Nov 7, 2023
@Kajot-dev
Copy link
Contributor

I've tested it on windows and can confirm it works.

As of the TODO: It would require quite bit of refactoring, but all in the same file, so I think it's doable. First we would have to extract a few functions:

  • commandParts sanity check from getRunnerCallWithoutCredentials to a separate sanitizeCommandParts
  • commandParts conversion to powershell's Start-Process to convertToPowershell
  • command formatting (env, and runnerPath) from getRunnerCallWithoutCredentials to formatCommand

This way getRunnerCallWithoutCredentials would call saniztizeCommandParts and then formatCommand and would have its functionality unchanged. However, callRunner would no longer call getRunnerCallWithoutCredentials, but instead saniztizeCommandParts then if its windows convertToPowershell twice (once for sanitized and not-sanitized commandParts) and lastly formatCommand for logging purposes.

TLDR:

Extract: sanitizeCommandParts, convertToPowershell, formatCommand

Changes in functions:

  • getRunnerCallWithoutCredentials calls sanitizeCommandParts, then formatCommand (functionality unchanged)
  • callRunner calls sanitizeCommandParts, then if windows convertToPowershell x2 (sanitized and not-sanitized) and lastly formatCommand

This way sanitization happens before converting to Powershell and without changes to the functionality of getRunnerCallWithoutCredentials

flavioislima
flavioislima requested changes Nov 14, 2023
View reviewed changes
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.

Tested here and legendary is broken since my user name has spaces on it.
all commands are failing.

@flavioislima
Copy link
Member

@CommandMC do you want to include this one on the next release?

@CommandMC
Copy link
Collaborator Author

I sure wouldn't complain if this would end up in the next release

The issue you mentioned should be resolved now. Sorry it took so long, somehow missed the review notification

@CommandMC CommandMC requested review from a team, arielj, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team December 14, 2023 18:40
@CommandMC CommandMC force-pushed the feat/playing-windows branch 2 times, most recently from dead43a to c5ef092 Compare December 18, 2023 18:28
@CommandMC CommandMC requested a review from flavioislima January 2, 2024 14:33
@CommandMC CommandMC linked an issue Jan 9, 2024 that may be closed by this pull request
Time played #3419
Closed
@arielj
Copy link
Collaborator

arielj commented Jan 17, 2024

@flavioislima can you confirm if the issue you had is resolved?

flavioislima
flavioislima approved these changes Feb 4, 2024
View reviewed changes
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.

looks good now

@flavioislima flavioislima added this to the 2.13.0 milestone Feb 5, 2024
@CommandMC
callRunner: Use PowerShell to run commands on Windows
8155cb6 
PowerShell's `Start-Process` cmdlet & its `-Wait` parameter are used to wait
for a process tree to exit. This means we can now accurately track the "Playing"
 status of games on Windows.
@CommandMC
Properly quote runner path
d9aa0d0
@CommandMC
Simplify argument quoting a bit
ecb6061 
There's no need to do different things depending on whether the argument
 contains a space, we can just always use the "longer" triple-quote form
@CommandMC CommandMC force-pushed the feat/playing-windows branch from c5ef092 to d3d39a1 Compare February 5, 2024 22:44
@CommandMC CommandMC merged commit dd86ec7 into main Feb 6, 2024
9 checks passed
@CommandMC CommandMC deleted the feat/playing-windows branch February 6, 2024 01:59
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@flavioislima flavioislima flavioislima approved these changes

@arielj arielj Awaiting requested review from arielj arielj was automatically assigned from Heroic-Games-Launcher/development

@Etaash-mathamsetty Etaash-mathamsetty Awaiting requested review from Etaash-mathamsetty Etaash-mathamsetty was automatically assigned from Heroic-Games-Launcher/development

@Nocccer Nocccer Awaiting requested review from Nocccer Nocccer was automatically assigned from Heroic-Games-Launcher/development

@imLinguin imLinguin Awaiting requested review from imLinguin imLinguin was automatically assigned from Heroic-Games-Launcher/development

Assignees

@CommandMC CommandMC

Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

Time played Launching games on Windows isn't properly tracked.
4 participants
@CommandMC @arielj @Kajot-dev @flavioislima