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

Switches the app to a single process #779

Merged
merged 17 commits into from
Nov 27, 2023
Merged

Switches the app to a single process #779

merged 17 commits into from
Nov 27, 2023

Conversation

halgari
Copy link
Collaborator

@halgari halgari commented Nov 16, 2023

This is a rewrite of the CLI portion of the app to support a single main process and "client" CLI processes that communicate over TCP localhost connections.

  • This mostly delegates the heavy lifting to the NexusMods.SingleProcess github project
  • Opening a GUI window when the main process is active fires off a CLI command to the main process to open a new window
  • Executing a CLI command while the main process is active sends the CLI commands to the main process and pipes the response back to the client process' terminal
  • Starting a CLI process while no main process is active will start the main process then pipe the commands to it
  • The main process will only exit after 5 seconds of no connected clients have passed (configurable)

This change also implements a new CLI verb registration system:

  • Verbs are now simply a static method on some class annotated via an attribute
  • Their arguments are either [Injected] (filled from DI) or [Option(...)] which are considered CLI options
  • Verbs are added to DI via .AddVerb(() => MethodName)

Not handled in this PR and will need to be handled in the future:

  • Switching SQLite to single-process mode
  • Removal of all IPC code from the app (IPC job queue, messaging etc)
  • Rerouting of logging messages to by default not show up in the console windows
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Merging #779 (eda30cd) into main (fd90b03) will increase coverage by 0.86%.
Report is 6 commits behind head on main.
The diff coverage is 38.29%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #779      +/-   ##
==========================================
+ Coverage   62.49%   63.36%   +0.86%     
==========================================
  Files         611      585      -26     
  Lines       18564    17863     -701     
  Branches     1305     1254      -51     
==========================================
- Hits        11602    11319     -283     
+ Misses       6620     6204     -416     
+ Partials      342      340       -2     
Flag Coverage Δ
Linux 62.67% <38.29%> (+0.92%) ⬆️
Windows 62.55% <38.29%> (+0.85%) ⬆️
clean_environment_tests 63.34% <38.29%> (+0.87%) ⬆️
network_tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ment/NexusMods.FileExtractor/FileExtractorVerbs.cs 100.00% <100.00%> (ø)
...hiveManagement/NexusMods.FileExtractor/Services.cs 100.00% <100.00%> (ø)
src/Games/NexusMods.Games.TestHarness/Services.cs 100.00% <100.00%> (ø)
...tworking.NexusWebApi.NMA/Messages/NXMUrlMessage.cs 100.00% <100.00%> (ø)
...g/NexusMods.Networking.NexusWebApi.NMA/Services.cs 100.00% <100.00%> (ø)
...rking/NexusMods.Networking.NexusWebApi/Services.cs 100.00% <100.00%> (ø)
...g/NexusMods.Networking.NexusWebApi/Types/NXMUrl.cs 89.83% <100.00%> (+0.35%) ⬆️
...pes/DownloadHandlers/NxmDownloadProtocolHandler.cs 0.00% <ø> (ø)
...usMods.DataModel/CommandLine/Verbs/ArchiveVerbs.cs 100.00% <100.00%> (ø)
....DataModel/CommandLine/Verbs/FileHashCacheVerbs.cs 100.00% <100.00%> (ø)
... and 39 more

... and 16 files with indirect coverage changes

@Sewer56
Copy link
Member

Sewer56 commented Nov 23, 2023

About to start reviewing the App side.

I reviewed the library side, since nobody has done that prior.

Requested changes on the library side have been submitted as 'issues' on the library repo:
https://github.com/Nexus-Mods/NexusMods.SingleProcessApp/issues

CC. @Greg-Nexus Can you set up labels over there?

Copy link
Member

@Sewer56 Sewer56 left a comment

Choose a reason for hiding this comment

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

No issues on the App side, all's good.

All requested changes and PR related feedback concern the library side itself. These can be found under the issues section in the library repo.

In general, I'm a fan of the new way to create verbs, and it's really surprising how clean this works (the command handler doesn't need to know anything about the fact it's talking over the network at all!).

Though, at the same time, I have reservations about the two-process design (having an idling client which does nothing), and its impact on startup time and RAM usage. Relevant details are covered in library issues above.

Misc Note: FOMOD CLI installation is also now dead. This wasn't mentioned anywhere, but figured I might aswell.

@Sewer56
Copy link
Member

Sewer56 commented Nov 27, 2023

Although I still have very heavy reservations regarding the client process sitting in RAM, doing nothing, the App side can be merged. Library side can be fixed at a later time.

PS. (I'm down to do it, since I'm familiar with said code now too, as I had to review it)

@halgari halgari merged commit 63ecc9e into main Nov 27, 2023
6 checks passed
@halgari halgari deleted the single-process-v3 branch November 27, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants