-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[RAPPS] Add ExeInZip installer type to support running installers in zip files #7866
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Sorry, I don't work on rapps anymore. |
CAppDB db(CAppDB::GetDefaultPath()); | ||
db.UpdateAvailable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need yet another instance of the CAppDB
? There is one that has been already created when RAPPS has been started. Isn't it possible to use that one instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The "main" instance is lost in loaddlg.cpp (the download dialog).
- The download stuff happens on a background thread and CAppDB is probably not thread safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm ok... It may be of interest in the future to make this appdb a singleton or something like that (so that there's only one running globally for the given rapps instance).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if making all its methods thread safe is worth the effort of doing that.
Using a global lock instead would slow down the normal use case just to deal with this rare instance where we want to ask about a single property during install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment, but lgtm otherwise.
|
||
if (!GetTempPathW(_countof(TempDirBuf), TempDirBuf)) | ||
return E_FAIL; | ||
wsprintfW(UniqueDir, L"~%s-%u", RAPPS_NAME, GetCurrentProcessId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer using wsprintf
instead.
This feature is to help streamline the installing experience. It will extract the downloaded .zip file to a subfolder in %temp% and execute the the installer. After the installer has completed, the temporary directory is deleted.
For a package like Hxd, simply adding
Installer = ExeInZip
to the main section is enough because Rapps will default to the first .exe it finds in the root of the extracted zip. For .zip files with a folder structure or multiple .exe files, the file to execute needs to be specified.For the Royale theme (PR waiting), it would look something like this:
Notes:
zip
installer type but they have the concept ofNestedInstallerType
which we don't.Testbot runs (Filled in by Devs)