-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix(linux): continue update process even if AppImage is not available #8539
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 I follow here, shouldn't the
appImageFile
always exist? We already check thatappImageFile !== null
to get to this logic.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.
appImageFile
is process.env.["APPIMAGE"]so it doesn't check whether the actual AppImage file exists, but the path is set as an environment variable.
For example, I set the path like this in code.
If we set process.env.APPIMAGE like above, then
APPIMAGE env is not defined
error is not coming out and starts download process.After the next version of AppImage downloded, then it checks whether the AppImage exists in the path, if not, it emits
ENOENT: no such file or directory, unlink '/home/***/Documents/***-1.4.1.AppImage'
I know myself where to put the previous AppImage (cause I did set the path),
but others who use this app don't know where to put it, they would just download AppImage somewhere in for example /Downloads folder and the update process will not work.
References:
APPIMAGE env is not defined (#3167)
https://stackoverflow.com/questions/52442650/electron-builder-linux-updates-appimage-env-is-not-defined
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.
Can you provide an example of your usecase to override
process.env.APPIMAGE
?If you truly need to override the AppImage path and the file doesn't exist yet, maybe create an empty/stub file at that path first?
Your link refrences are issues due to user misconfiguration (such as Webpack's DefinePlugin) in the respective projects. We can easily add to the documentation site for specifying the potential side effects that can occur with bundler misconfiguration.
APPIMAGE
env var is a dedicated var set by the system, it should always exist and I'd prefer this logic to retain that logic expecting the file it represents to exist.https://docs.appimage.org/packaging-guide/environment-variables.html#id1
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 mentioned References as just for the same error message.
APPIMAGE env is not defined
It's not related to webpack actually.
I edited my comment, so could you please review again? Thank you.
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 don't think this is a reasonable PR change, but you still haven't explained your use case to custom set/override
process.env.APPIMAGE
. Can you please elaborate on that?Wouldn't running the AppImage from the Downloads folder result in that file in the Downloads folder being updated? (e.g. correctly) Are you relying on some specific user environment configured for custom paths to work?
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 set the custom path because it emits error
not defined
. No other reason actually..If I set the path to
${process.env.HOME}/Downloads/***-${app.getVersion()}.AppImage
, then it would work.The path should be defined in the code, so if someone changed the download path, update is not working..
This could be great, so I did thought about it once that scan the user' file system first, if the AppImage exists, then set that path to be
process.env.APPIMAGE
But this logic is little bit complicated I think, so I wanted to do process even if the user had downloaded previous version AppImage to anywhere, the next version would download successfully and mv to defined path automatically.
Then for the next update, the downloaded next version is in the right path, so the update is work as the code said.
In my thought, it is little bit awkward process to whom install the app for the first time.
So if I add try-catch to
doInstall
function to go on even the AppImage is not available, very first installer's update would work, and the next update will go on normally after the first update.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.
So on thinking on this further, I can't accept this PR change. If the
unlink
fails for a prod user running in current configuration but the relaunch sequence is able to proceed via your try-catch, there will be an infinite auto-update loop because it can't replace the original file with the new update viamv
. (unlink
is required https://stackoverflow.com/questions/1712033/replacing-a-running-executable-in-linux/1712051#1712051)If you'd still like to proceed with your unique use case, please try using
patch-package
to patch your changes so that you can build your app with it.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 have tried this in my project, and the result was
(set path)
process.env.APPIMAGE = ${process.env.HOME}/Documents/*.AppImage
[No try-catch]
no such file or directory
➡️ Update process stops and restarts current version[try-catch]
These are the whole process, I have tried several times but infinite loop was not found.
Thank you for your valuable time reviewing these long comments.
Whether my PR is accepted or not is not important, but I think I found a little bit awkward process while developing, so I was just giving my opinion.
Thank you again!
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.
This seems like unexpected behavior to me from what the default AppImage ecosystem expects, and as with AppImage Launcher. Happy to be read resources that state otherwise though.
What'd I'd recommend though for your app's logic is on launch of the app, detect the current APPIMAGE path and if it's not in Documents, locally run
mv
the current file represented APPIMAGE path to /Documents, then reset theprocess.env.APPIMAGE=<new path in /Documents>
, after which you can eitherunlink
the old file (or leave it), and then just runquitAndInstall
or restart the app yourself viaexecFileSync(process.env.APPIMAGE); app.quit();
. If you choose not tounlink
the old file, if the user already created a .desktop file or registered it with AppImageLauncher, they'll still launch the legacy version AFAIK.So with all this occurring, I would also recommend presenting a notification to the user for confirmation that you're moving the application path and also determine if you need to update/re-register the app with the .desktop file or with AppImage Launcher (if it's possible, I'm personally not familiar with it)
Alternatively, you could patch-package the try-catch, however I'd still recommend the .desktop file check and update AppImage Launcher