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

Build as Self Contained and Selfhost CefSharp SubProcess #693

Merged

Conversation

DubyaDude
Copy link
Contributor

@DubyaDude DubyaDude commented Nov 22, 2023

This changes the build of VRCX to be self-contained, removing the requirement of installing dotnet, and not having to deal with potential bad dotnet installs.

Pros

Cons

  • Larger Installer - The installer increased from 161MB to 227MB
  • Larger install - The VRCX folder after the first launch went from 356MB to 516MB
  • More files - The VRCX folder after the first launch went from 992 files to 1,450

Pro/Con

  • We are distributing our own runtime now. This allows for much better control of the runtime the app runs off possibly reducing the 'Works on my machine' type of issues. However, this means the responsibility of keeping the runtime up to date now falls on us developers rather than the end-user. (NOTE: because of the GitHub runner publishing builds, runtime will be automatically upgraded to the latest patch version the runner has, which is pretty kept up to date)

Not Changed

  • CPU or RAM usage has not been significantly affected.

@DubyaDude DubyaDude changed the title Added self contained parameter to builds VRCX Build as Self Contained Nov 22, 2023
@DubyaDude DubyaDude changed the title VRCX Build as Self Contained Build VRCX as Self Contained Nov 22, 2023
@DubyaDude
Copy link
Contributor Author

@Myrkie @Natsumi-sama @BenjaminZehowlt
Mind giving this a test run to see if anything significant breaks?

@DubyaDude DubyaDude marked this pull request as ready for review November 22, 2023 14:46
@DubyaDude
Copy link
Contributor Author

@Okabintaro
Copy link

@Okabintaro Can you try this install? https://github.com/vrcx-team/VRCX/suites/18400997359/artifacts/1066185171

That one works fine! I also made an issue for my previous problem in #695.

@TayouVR
Copy link
Contributor

TayouVR commented Nov 22, 2023

could this be an extra build artifact, offering two versions; one self contained and one using the system .net install?
Though I'm not sure how that would work with the updater, I guess it would have a flag or smth to know to update to the self contained or the other version.

@DubyaDude
Copy link
Contributor Author

could this be an extra build artifact, offering two versions; one self contained and one using the system .net install? Though I'm not sure how that would work with the updater, I guess it would have a flag or smth to know to update to the self contained or the other version.

While that's something that could be done, would require effort.
I don't see the cons listed as being enough to push us towards that. If you're short on disk space to be worried by 160MB, I think you will run into storage issues with or without self-contained.
And regarding the 'we control runtime version', it'll be upgraded to the latest every build by the GitHub runner, so keeping it updated isn't really tedious.

I don't see any significant enough of a downside of this.

@Natsumi-sama
Copy link
Member

Sometimes more options isn't better, this certainly one of those times, it's not worth the time explaining the difference in builds every time to people that just don't care or notice.

@TayouVR
Copy link
Contributor

TayouVR commented Nov 22, 2023

it's not worth the time explaining the difference in builds every time

fair, thought of that too, I just like having options. (and I like having my dependencies installed system wide - shared across apps)

@Natsumi-sama
Copy link
Member

Mind giving this a test run to see if anything significant breaks?

It seems building with --self-contained causes CefSharp.BrowserSubprocess.exe to be excluded, we could go down the route of building our own BrowserSubprocess into the main exe I've seen that done by other people using Cef.

@DubyaDude
Copy link
Contributor Author

DubyaDude commented Nov 22, 2023

Mind giving this a test run to see if anything significant breaks?

It seems building with --self-contained causes CefSharp.BrowserSubprocess.exe to be excluded, we could go down the route of building our own BrowserSubprocess into the main exe I've seen that done by other people using Cef.

I removed that removal from the csproj though, should be included now, no?
I tested a quick launch using Sandbox, dotnet NOT installed, using the build artifact here and it seems to work fine at login, haven't tested past that.

- <CefSharpExcludeSubProcessExe>true</CefSharpExcludeSubProcessExe>

Removal was removed here: https://github.com/vrcx-team/VRCX/pull/693/files#diff-fcda6e0752ad20a00c16d8f4d2461f9c44e0a954c09bbefd406bc79465a61a38L34

@Natsumi-sama
Copy link
Member

Yeeeeea that was it, I'm not sure how I manged to miss that.

@DubyaDude DubyaDude changed the title Build VRCX as Self Contained Build VRCX as Self Contained and Selfhost CefSharp SubProcess Nov 23, 2023
@DubyaDude DubyaDude changed the title Build VRCX as Self Contained and Selfhost CefSharp SubProcess Build as Self Contained and Selfhost CefSharp SubProcess Nov 23, 2023
@Natsumi-sama Natsumi-sama merged commit f053f2b into vrcx-team:master Dec 4, 2023
1 check passed
Natsumi-sama added a commit to Natsumi-sama/VRCX that referenced this pull request Dec 4, 2023
* Added self contained parameter to builds

* Remove dotnet install from installer

* Fixed missing SubProcessExe

* Self hot browser subprocess

---------

Co-authored-by: Natsumi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] VRCX showing .NET 8 Install/Update Prompt even when it is installed.
4 participants