chore: implement electron-configure-package-win32.sh

[jviotti]

This script configures an Electron package on Windows.

Signed-off-by: Juan Cruz Viotti [email protected]

[lurch]

Could / should that be included in package.json?

[jviotti]

Done!

[lurch]

Probably makes sense to only show these warnings when you're actually about to do the code-signing step, rather than showing them all the time to all users.

[lurch]

And it probably makes sense to nest the ifndef CODE_SIGN_CERTIFICATE_PASSWORD inside the ifndef CODE_SIGN_CERTIFICATE too

[lurch]

Ahhh, I now see that the code-signing step is simply skipped if these env-vars aren't set.

Still it feels wrong to always display them though, even for users who are just building locally and will never be able to code-sign? Can't think of a good solution off the top of my head though.

Oooh! What if we instead had the code-signing as a precursor to the publishing step, so that only users who attempt to run the publishing targets will get warned about missing CODE_SIGN env-vars?

[jviotti]

Oooh! What if we instead had the code-signing as a precursor to the publishing step, so that only users who attempt to run the publishing targets will get warned about missing CODE_SIGN env-vars?

The thing is that code-signing right before publishing might mean re-doing the whole packaging step. In OS X for example, we create a temporary read-write DMG, and we have to code-sign inside the DMG and then convert to a read-only DMG.

How would you handle that right before publishing? I'm not saying that it can't be done, but it means re-doing a lot of work that took us to the publishing step in the first place.

[jviotti]

And it probably makes sense to nest the ifndef CODE_SIGN_CERTIFICATE_PASSWORD inside the ifndef CODE_SIGN_CERTIFICATE too

Done!

[lurch]

I guess one option would be to have a non-signed version as the "standard" DMG, and then have a special code-signed DMG required by the publish step? That's probably a bit too much of a step though just to silence a couple of warnings ;-)
Or a simple solution could just be a SILENCE_CODE_SIGN_WARNINGS ?= 0 in the Makefile, so the user could set a SILENCE_CODE_SIGN_WARNINGS=1 env-var if they wanted to.

OTOH, perhaps it's inconsistent behaviour to have the Makefile generate two different versions of the same file, based on the value of an env-var? (makes it hard to tell them apart?)
Consider if:

1. You run make without the CODE_SIGN env-vars set
2. Make creates the unsigned package
3. You run make again with the CODE_SIGN env-vars set
4. Make doesn't do anything, because it thinks the package is already up to date
5. You run 'make publish'
6. Whoops, make has now uploaded the unsigned packages!

What if instead, there was e.g. a release/out/signed directory, and make publish depended on release/out/signed/Etcher-1.0.0-beta.17-darwin-x64.dmg, which in turn depended on the temporary read-write DMG (could it be given a different name and made non-temporary?). And then release/out/Etcher-1.0.0-beta.17-darwin-x64.dmg would also depend on the same 'temporary' read-write DMG, and would always be the non-signed package.
Would that work? ;)

[lurch]

And for those scripts where you have the option to verify the signature, maybe it would make sense to sign the package / exe in the temporary directory, and only move it to release/out/signed/ after it's been verified.

[jviotti]

I like the signed/ subdirectory idea. I propose to do it in another PR so we can do it for all the operating systems. Regarding the warning, what about simply not printing the warning, but report that the binary is not going to be signed, in make info?

If you agree on that, lets merge the PR as it is now, and I'll send a new one right away to make that change, so we do it for all operating systems at once.

[lurch]

As I said here I think it'd make sense for the "standard build" e.g. make electron-installer-dmg to produce unsigned images (and have them also be snapshot versions by default), and then have a special e.g. signed-electron-installer-dmg target to create (production-version?) files in the signed/ subdirectory, and only do the check-for-certificate-vars step as part of the signed-electron-installer-dmg rules, and upgrade it from a warning to an error.
(If that all makes sense? 😉 )

[jviotti]

Sounds good to me :) Let me know if I can help with any OS X testing (if you don't have access to OS X yet, you might want to ping the operations team to ship you a Mac Mini or something like that anyway)!

[lurch]

In the darwin section, you've used -c "$(APPLICATION_COPYRIGHT)" - consistency is always good

[jviotti]

Yeah, single letter options might lead to a lot of confusion. I'll use -m in this case, but maybe we should look for a getopts alternative that allows us to use longer option names?

[lurch]

...and (again, for consistency) could you change -r "$(APPLICATION_COPYRIGHT)" to -c "$(APPLICATION_COPYRIGHT)" ? 😄

[jviotti]

Oops, I missed this comment. Done!

[lurch]

Don't forget to update this if #959 gets merged first

[jviotti]

Done!

[lurch]

This should probably do the same TMPDIR= trick that you use in electron-installer-appimage.sh ?

[jviotti]

I think I prefer to be explicit about it and ask it as an option rather than relying on the environment at all.

[lurch]

Fair enough

[lurch]

This should be changed after #946 gets merged

[jviotti]

Done!

[lurch]

Yep ;)

[lurch]

Great to see the win32 build moving to the same Makefile & Bash setup as linux and darwin 👍

[lurch]

Curiosity finally got the better of me... ;-) electron/node-rcedit#23

[lurch]

Need to remove this function :)

[jviotti]

Oops, done!

[lurch]

As this script runs a win32 executable (rcedit.exe), it ought to include the only-running-on-windows check

[jviotti]

Indeed. In theory, this script could run in the other operating systems on top of Wine, but lets ignore that for now :)

[lurch]

By re-ordering some of the lines...

--- a/scripts/build/electron-configure-package-win32.sh
+++ b/scripts/build/electron-configure-package-win32.sh
@@ -95,16 +95,9 @@ fi
 unzip "$ARGV_ELECTRON_PACKAGE" -d "$ARGV_OUTPUT"
 
 mv "$ARGV_OUTPUT/electron.exe" "$ARGV_OUTPUT/$ARGV_APPLICATION_NAME.exe"
-rm -f "$ARGV_OUTPUT/resources/default_app.asar"
-
-cp "$ARGV_ASAR" "$ARGV_OUTPUT/resources/app.asar"
 cp "$ARGV_LICENSE" "$ARGV_OUTPUT/LICENSE"
-
-if [ -d "$ARGV_ASAR.unpacked" ]; then
-  cp -rf "$ARGV_ASAR.unpacked" "$ARGV_OUTPUT/resources/app.asar.unpacked"
-fi
-
 echo "$ARGV_VERSION" > "$ARGV_OUTPUT/version"
+rm -f "$ARGV_OUTPUT/resources/default_app.asar"
 
 RCEDIT_VERSION=v0.7.0
 RCEDIT="$ARGV_DOWNLOAD_DIRECTORY/rcedit.exe"
@@ -126,3 +119,9 @@ RCEDIT="$ARGV_DOWNLOAD_DIRECTORY/rcedit.exe"
   --set-icon "$ARGV_ICON"
 
 upx -9 "$ARGV_OUTPUT/*.dll"
+
+cp "$ARGV_ASAR" "$ARGV_OUTPUT/resources/app.asar"
+
+if [ -d "$ARGV_ASAR.unpacked" ]; then
+  cp -rf "$ARGV_ASAR.unpacked" "$ARGV_OUTPUT/resources/app.asar.unpacked"
+fi

...it's possible to get this script to much more closely match electron-configure-package-linux.sh. Have I ever mentioned I like consistency? ;-)

[jviotti]

Have I ever mentioned I like consistency? ;-)

Haha, I think I heard something about that! :P

I applied the diff!

[jviotti]

Is this good to merge?

[lurch]

There's two reasons I hadn't approved this yet:

• chore: implement electron-configure-package-win32.sh #962 (comment)
• And I didn't want to lose this comment until you'd replied to it

[jviotti]

Oops, I missed those comments, sorry for that!