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

Change extract folder for each instance for portable app or preventing delete extracted resources when closing second instance? #5764

Closed
kobakou opened this issue Apr 3, 2021 · 21 comments
Assignees
Labels

Comments

@kobakou
Copy link

@kobakou kobakou commented Apr 3, 2021

  • Version: 22.9.1
  • Electron Version: 9.4.0
  • Electron Type (current, beta, nightly): current
  • Target: portable

Backgrounds:

  • build as portable apps for windows
  • we want to run multiple instances of this app at the same time

Issues:

  • From v20.40, the folder that is extracted when the app is launched is unique for each Build.
  • As a result, when two or more apps are running, closing the second one will delete the extracted folder and some files will be deleted
  • It may result in the first app not working properly.

How can I resolve this issue, such as changing extract folder for each launch instance or preventing delete extracted resources when closing the second instance?

I found a similar post in the issues log, but I can not find a resolution.
#5382 (comment)

Thanks,
Kou

@kobakou
Copy link
Author

@kobakou kobakou commented Apr 7, 2021

I found related reports. I think this use-case is to be considered.
#4105

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Apr 8, 2021

Here's a quick patch I whipped up. Please apply it to your project and report back on how it works 🙂

Use unpackDirName: false in the config, which internally does not define nsi var $UNPACK_DIR_NAME. The portable app will default back to $PLUGINSDIR\app when set false (pre-v20.40 behavior).
Note: passing true will break the logic because this was scrappy.

diff --git a/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js b/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js
index a64e26f..8f5c29d 100644
--- a/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js
+++ b/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js
@@ -428,7 +428,10 @@ class NsisTarget extends _core().Target {
     if (isPortable) {
       const portableOptions = options;
       defines.REQUEST_EXECUTION_LEVEL = portableOptions.requestExecutionLevel || "user";
-      defines.UNPACK_DIR_NAME = portableOptions.unpackDirName || (await (0, _builderUtil().executeAppBuilder)(["ksuid"]));
+
+      if (portableOptions.unpackDirName !== false) {
+        defines.UNPACK_DIR_NAME = portableOptions.unpackDirName || (await (0, _builderUtil().executeAppBuilder)(["ksuid"]));
+      }
 
       if (portableOptions.splashImage != null) {
         defines.SPLASH_IMAGE = path.resolve(packager.projectDir, portableOptions.splashImage);
diff --git a/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts b/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts
index 5962e6a..44371c2 100644
--- a/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts
+++ b/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts
@@ -152,10 +152,11 @@ export interface PortableOptions extends TargetSpecificOptions, CommonNsisOption
     readonly requestExecutionLevel?: "user" | "highest" | "admin";
     /**
      * The unpack directory name in [TEMP](https://www.askvg.com/where-does-windows-store-temporary-files-and-how-to-change-temp-folder-location/) directory.
+     * Use `false` to let Windows generate the folder name at runtime.
      *
      * Defaults to [uuid](https://github.com/segmentio/ksuid) of build (changed on each build of portable executable).
      */
-    readonly unpackDirName?: string;
+    readonly unpackDirName?: string | boolean;
     /**
      * The image to show while the portable executable is extracting. This image must be a bitmap (`.bmp`) image.
      */
diff --git a/node_modules/app-builder-lib/templates/nsis/portable.nsi b/node_modules/app-builder-lib/templates/nsis/portable.nsi
index e908549..fa4f3b9 100644
--- a/node_modules/app-builder-lib/templates/nsis/portable.nsi
+++ b/node_modules/app-builder-lib/templates/nsis/portable.nsi
@@ -30,7 +30,11 @@ Section
     HideWindow
   !endif
 
-  StrCpy $INSTDIR "$TEMP\${UNPACK_DIR_NAME}"
+  StrCpy $INSTDIR "$PLUGINSDIR\app"
+  !ifdef UNPACK_DIR_NAME
+    StrCpy $INSTDIR "$TEMP\${UNPACK_DIR_NAME}"
+  !endif
+
   RMDir /r $INSTDIR
   SetOutPath $INSTDIR
 

Loading

@mmaietta mmaietta self-assigned this Apr 8, 2021
@kobakou
Copy link
Author

@kobakou kobakou commented Apr 10, 2021

@mmaietta
Thank you for providing patches. When I applied your patch and added the option as unpackDirName: false, then the extracted folder will be changed by runtimes as I expected!

Your patches work well, I hope your option will add to master branch!

BTW, this behavior is the same as pre-v20.40 but the %temp%\nsXXX.tmp folder does not delete when app closed...

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Apr 10, 2021

Glad to hear it worked!

I'm not sure what to do with the temp folder being deleted after..., I just took a look at the git history to figure out that patch haha

Any advice or ideas on where to put that in an nsis script?

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Apr 11, 2021

This change is related to them.
#3799
3be0181

I suppose/hope @develar makes the right advice for us!! 🙏

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Apr 12, 2021

Yeah, that's where I backtracked to as well. I didn't immediately notice anything for cleaning up a portable app's resources on exit. $PLUGINSDIR is a temp folder provided by Windows, so I'm wondering if it's auto-cleaned up later by the system not explicitly on app-close.

Loading

@selinalisz0
Copy link

@selinalisz0 selinalisz0 commented Apr 14, 2021

@mmaietta
Hi, I've tried your solution and it works after I changed from

if (portableOptions.unpackDirName !== false)

to

if (!portableOptions.unpackDirName)

Do you have a plan to put the patch in the next build?

Thanks,
Selina

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Apr 15, 2021

Hmmm, that doesn't make sense to me. That shouldn't have worked if your config value was false.
We want it to use unpackDirName or the ksuid whenever the unpackDirName config value is true, undefined, or string (regardless of length). Only when explicitly false do we not want to set an extract folder path.
Can you confirm whether you had unpackDirName: false in your electron-builder config?

Loading

mmaietta added a commit to mmaietta/electron-builder that referenced this issue Apr 15, 2021
@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Apr 15, 2021

@kobakou I think I may have determined the fix for cleaning up the install dir. Can you try this patch? Need confirmation for opening a PR.

diff --git a/node_modules/app-builder-lib/templates/nsis/portable.nsi b/node_modules/app-builder-lib/templates/nsis/portable.nsi
index e908549..e5436bb 100644
--- a/node_modules/app-builder-lib/templates/nsis/portable.nsi
+++ b/node_modules/app-builder-lib/templates/nsis/portable.nsi
@@ -30,7 +30,11 @@ Section
     HideWindow
   !endif
 
-  StrCpy $INSTDIR "$TEMP\${UNPACK_DIR_NAME}"
+  StrCpy $INSTDIR "$PLUGINSDIR\app"
+  !ifdef UNPACK_DIR_NAME
+    StrCpy $INSTDIR "$TEMP\${UNPACK_DIR_NAME}"
+  !endif
+
   RMDir /r $INSTDIR
   SetOutPath $INSTDIR
 
@@ -82,6 +86,6 @@ Section
 	ExecWait "$INSTDIR\${APP_EXECUTABLE_FILENAME} $R0" $0
   SetErrorLevel $0
 
-  SetOutPath $PLUGINSDIR
+  SetOutPath $EXEDIR
 	RMDir /r $INSTDIR
 SectionEnd

Main part is changing it to use $EXEDIR which I discovered when stumbling across this: https://nsis-dev.github.io/NSIS-Forums/html/t-326463.html

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Apr 15, 2021

@mmaietta
wow! Fantastic! I have confirmed closing app cleans also the folder not only the contained files.
But ksuid based temp folder is broken? Could you check unpackDirName option by your side again?

  • no unpackDirName : nsXXX.tmp <- ksuid is expeced
  • unpackDirName: false : nsXXX.tmp
  • unpackDirName: 'strings' : strings

Thanks.

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Apr 23, 2021

@mmaietta , I'm looking forward to opening your PR. If I can do your help, please ask me!

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented May 13, 2021

@kobakou, sorry for the delayed reply here. Things got swamped on my side.
If you wouldn't mind just messing around with the patch until it works for those 3 cases, I'd be happy to open a PR and include you in the credits 🙂
There's some larger issues ongoing with 22.11 that I need to prioritize.

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented May 14, 2021

@mmaietta thank you for coming back and sharing your status! It sounds good for me.
Of course, I respect your prioritize. 👍

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Jun 9, 2021

I hope this issue will get high order on the list for the next release.

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Jul 11, 2021

Finally circling back on this ticket but I'm struggling to figure out where I left off 😅

I'll need to figure out how to create some unit tests around this to fix the behavior. Maybe I can stub the ksuid generator.

  • no unpackDirName : nsXXX.tmp <- ksuid is expeced
  • unpackDirName: false : nsXXX.tmp
  • unpackDirName: 'strings' : strings

After that I'll probably post back and request you to run another test with a .patch file. Aiming to get this into the next release

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Jul 15, 2021

@mmaietta, thanks a lot for coming back!

I expect XXX in nsXXX.tmp will be randomly generated for every launch.

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Jul 24, 2021

@kobakou could you please verify from your side all 3 flows with this patch file? You'll need to use v22.11.9
I switched the logic to be cleaner:

string => string
false | empty string | undefined => ksuid
true => nsXXX.tmp

diff --git a/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js b/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js
index ef4dfd7..3279522 100644
--- a/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js
+++ b/node_modules/app-builder-lib/out/targets/nsis/NsisTarget.js
@@ -214,11 +214,14 @@ class NsisTarget extends core_1.Target {
         }
         this.configureDefinesForAllTypeOfInstaller(defines);
         if (isPortable) {
-            const portableOptions = options;
-            defines.REQUEST_EXECUTION_LEVEL = portableOptions.requestExecutionLevel || "user";
-            defines.UNPACK_DIR_NAME = portableOptions.unpackDirName || (await builder_util_1.executeAppBuilder(["ksuid"]));
-            if (portableOptions.splashImage != null) {
-                defines.SPLASH_IMAGE = path.resolve(packager.projectDir, portableOptions.splashImage);
+            const { unpackDirName, requestExecutionLevel, splashImage } = options;
+            defines.REQUEST_EXECUTION_LEVEL = requestExecutionLevel || "user";
+            // https://github.com/electron-userland/electron-builder/issues/5764
+            if (typeof unpackDirName === "string" || !unpackDirName) {
+                defines.UNPACK_DIR_NAME = unpackDirName || (await builder_util_1.executeAppBuilder(["ksuid"]));
+            }
+            if (splashImage != null) {
+                defines.SPLASH_IMAGE = path.resolve(packager.projectDir, splashImage);
             }
         }
         else {
diff --git a/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts b/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts
index 5962e6a..44371c2 100644
--- a/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts
+++ b/node_modules/app-builder-lib/out/targets/nsis/nsisOptions.d.ts
@@ -152,10 +152,11 @@ export interface PortableOptions extends TargetSpecificOptions, CommonNsisOption
     readonly requestExecutionLevel?: "user" | "highest" | "admin";
     /**
      * The unpack directory name in [TEMP](https://www.askvg.com/where-does-windows-store-temporary-files-and-how-to-change-temp-folder-location/) directory.
+     * Use `false` to let Windows generate the folder name at runtime.
      *
      * Defaults to [uuid](https://github.com/segmentio/ksuid) of build (changed on each build of portable executable).
      */
-    readonly unpackDirName?: string;
+    readonly unpackDirName?: string | boolean;
     /**
      * The image to show while the portable executable is extracting. This image must be a bitmap (`.bmp`) image.
      */
diff --git a/node_modules/app-builder-lib/templates/nsis/portable.nsi b/node_modules/app-builder-lib/templates/nsis/portable.nsi
index e908549..e5436bb 100644
--- a/node_modules/app-builder-lib/templates/nsis/portable.nsi
+++ b/node_modules/app-builder-lib/templates/nsis/portable.nsi
@@ -30,7 +30,11 @@ Section
     HideWindow
   !endif
 
-  StrCpy $INSTDIR "$TEMP\${UNPACK_DIR_NAME}"
+  StrCpy $INSTDIR "$PLUGINSDIR\app"
+  !ifdef UNPACK_DIR_NAME
+    StrCpy $INSTDIR "$TEMP\${UNPACK_DIR_NAME}"
+  !endif
+
   RMDir /r $INSTDIR
   SetOutPath $INSTDIR
 
@@ -82,6 +86,6 @@ Section
 	ExecWait "$INSTDIR\${APP_EXECUTABLE_FILENAME} $R0" $0
   SetErrorLevel $0
 
-  SetOutPath $PLUGINSDIR
+  SetOutPath $EXEDIR
 	RMDir /r $INSTDIR
 SectionEnd

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Jul 25, 2021

@mmaietta
Thank you for your great works!
I tried true option, but it was recognized as "string" and "true" folder was created in temp folder.
I'm not sure what I'm doing... Please advise me for using this option. Other options(undefined, 'string') work well.

        portable: {
            requestExecutionLevel: 'user',
            unpackDirName: true,
        },

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Jul 25, 2021

@mmaietta

I've referred to this PR, and make changes in scheme.json. Then all options work well!!
https://github.com/electron-userland/electron-builder/pull/6093/files#diff-85ce848f8db5dc85e9715ea5b2ee1d84d71b5d9dd2620948cb9a72355cc18ef2

Thanks!

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Jul 28, 2021

So, just to confirm. The reason this occurred was due to the scheme.json not being up-to-date? After updating the scheme.json locally, it all worked as expected?

I tried true option, but it was recognized as "string" and "true" folder was created in temp folder.

Please review my PR and mark as approved if it looks good to you
#6093

Loading

@kobakou
Copy link
Author

@kobakou kobakou commented Jul 28, 2021

@mmaietta
Added ! Thank you! #6093 (review)

Loading

mmaietta added a commit that referenced this issue Jul 29, 2021
…unch (#6093)

* Allowing boolean flag to `unpackDirName` to utilize `$PLUGINSDIR` (unique to per-app-launch) when set explicitly to `true`. Implements #5764, #5382, #4105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants