Update Windows configuration to remove AppX ambiguity #3239

mokagio · 2024-07-25T06:16:29Z

What this does

In hindsight, it sounds pretty simple... This fix removes the appx node from the default configuration, electron-builder.json, so that CI generates only one set of appx files. The appx are then generated using a dedicated configuration. More on that later.

If you check the logs for the v2.22.1 build, you'll notice that two pairs of appx are being built. First as part of the npx electron-builder --win call which uses electron-builder.json...:

...then from the npx electron-builder --win --config=electron-builder-appx.json call:

You'll also notice that the first appx are code signed while the last are not. This sent me down a bit of a rabbit hole, more on that later, too.

So, it turns out that the code signing was not strictly broken, it just so happened that I downloaded the wrong appx artifacts from the release... 🤦‍♂️ That's an error only on my part, as our process checklist clearly states to download the ones with win-store in the name.

About code signing AppX

The short story is: We don't need to code sign AppX if we want to upload them to the Microsoft Partner Center. Upon uploading, Microsoft signs them for us. See also https://www.electron.build/configuration/appx#appx-package-code-signing.

Things got confusing when in the migration between CircleCI and Buildkite I made use of the certificate with publisher CN="Automattic, Inc.", O="Automattic, Inc.", S=California, C=US where before we were using CN=E2E5A157-746D-4B04-9116-ABE5CB928306.

So, even though I was able to code sign the appx with the new certificates (despite code siging being unnecessary, which I only learned afterwards) Microsoft rejected them because of the mismatch between the expected publisher.

About using two configurations

The reason for this setup is that, as far as I able to find, the configuration required to generate a code signed exe via the nsis target will conflict the appx configuration.

You can see the various experiments I went through to try and make both configurations play well together in #3235.

In practice, "certificateSubjectName": "Automattic, Inc." is required to sign the exe — which we need for it to run without looking dodgy, given it is distributed outside the store – but if that setting is present and so are the appx settings, there will be a failure, e.g. https://buildkite.com/automattic/simplenote-electron/builds/176#0190e45b-0298-4168-b47c-6e2aef7caec3, similar to what is reported in electron-userland/electron-builder#6698

As a personal note, I wish I'd been smarter and dig into why we had two configurations sooner. Also worth noting that WordPress Desktop also uses this approach, and I knew it, too, use electron-builder. The fact that two projects use the same odd configuration setup would have pointed more clearly to the config incompatibility.

"Proof" that it works

Here's a screenshot of the Partner Center where the appx packages from the latest build in this branch have passed validation:

And here is a screenshot of my Windows VM where the exe from the same build passes the Edge safety check etc.

Where to go from here

Things I'd like to look into:

Convert the config to YML so we can have comments and document why the setup exists close to where the setup is used. (In the meantime, I added a little note about code signing in the make task output)
Look into the crash on the electron-builder side, as well as the configuration hierarchy that results in the clash. Maybe there is a reason, but I don't see why we cannot set code sining settings at the nsis level so that only the exe will code sign, instead of having certificateSubjectName in the win root and resulting in it code signing the appx, too
If we really have to stick with the two config approach, then we should generated the ad hoc config at runtime with a script that
1. Reads electron-builder.json
2. Strips the unrelated settings
3. Injects the appropriate appx settings

...The default configuration should build it already

And properly fail if something goes wrong. This will prevent silent failure like the one in https://buildkite.com/automattic/simplenote-electron/builds/174#0190e36e-b92e-4c7d-a6aa-7c26a02197b0/233-243

mokagio · 2024-07-25T08:31:17Z

@codebykat @roundhill I wanted to add additional details on the implementation in the PR description but I run out of time today.

Here's a ping to have a look and maybe test the exe on your end. I'll put the finishing touches tomorrow.

codebykat · 2024-07-25T11:11:07Z

Oh, nice. I haven't tested this, but I'm looking at that transparent icon setting and wondering about these reported issues: #3057 and #3157

Possibly we want the icon to be not transparent? 🤔

roundhill · 2024-07-25T16:41:53Z

The .x64 exe worked great for me on Windows 10. I tried installing the .appx but it said there was a code sign error, which I assume is because that has to be done on the Partner Center Portal?

mokagio · 2024-07-25T22:32:52Z

I tried installing the .appx but it said there was a code sign error, which I assume is because that has to be done on the Partner Center Portal?

Correct @roundhill . The AppX are explicitly not signed by CI and we the Partner Center do that for us.

mokagio · 2024-07-26T05:07:19Z

Makefile

 	@npx electron-builder --win -p $(PUBLISH)
-ifeq ($(IS_WINDOWS),true)


I decided to remove the platform check under the assumption these will mostly run in CI.

I'm not aware of any requirement for building Windows on a non Windows machine either.

This way, the script is simpler to understand.

mokagio · 2024-07-26T05:07:55Z

Makefile

-	@echo Packaging .appx as well...
+	@echo "Packaging appx with dedicated configuration to work around code signing conflicts..."


A tiny attempt of help future readers understand the need for the dedicated config

mokagio · 2024-07-26T05:08:43Z

electron-builder-appx.json

-  "afterSign": "./after_sign_hook.js",
-  "afterAllArtifactBuild": "./after_sign_hook.js"


Notice that these were the same script. If you look into it, the only thing it does is notarizing for macOS. I decided to remove it to keep the config as small as possible.

To check my understanding of this file: it's fine to remove the calls to the after_sign_hook.js script because they only do macOS notarization and this file only deals with the Windows AppX build, correct?

@spencertransier correct

Makefile

AliSoftware · 2024-08-08T15:03:25Z

electron-builder.json

-      },
-      {
-        "target": "appx",
-        "arch": ["ia32", "x64"]
      }


@mokagio I was about to suggest adding some comments to mention that the .appx is instead generated with a custom config via make package-win32 so it's properly code-signed, + link to this PR's description for the breadcrumbs…

…but then I just realized this is a .json file, which doesn't accept comments 😢

That being said, it seems that electron-builder supports configuration files in YAML (or js) format. So WDYT of migrating the project to a electron-builder.yml file instead of .json in a future PR, so we can start adding such comments to it?

Sure, 100% agree. I thought I had written it somewhere as a TODO myself, but I couldn't find a reference to share here.

Every time I see a JSON used for config these days my first instinct is to ask if we can make it into a YAML. Being able to comment and use anchors and aliases makes the config work simpler.

Co-authored-by: Olivier Halligon <[email protected]>

mokagio added 4 commits July 19, 2024 19:43

Remove conditional AppX build when on Windows...

841cca8

...The default configuration should build it already

Use shared dedicated script to run npm install in Windows CI

424339a

And properly fail if something goes wrong. This will prevent silent failure like the one in https://buildkite.com/automattic/simplenote-electron/builds/174#0190e36e-b92e-4c7d-a6aa-7c26a02197b0/233-243

Print path when failing to find Windows certificate in CI

e4672ea

Reintroduce dedicated config file for AppX build

0f7e2d8

roundhill marked this pull request as ready for review July 25, 2024 16:34

roundhill marked this pull request as draft July 25, 2024 16:34

mokagio changed the title ~~Update Windows configuration to successfully upload to Microsoft Partner Center~~ Update Windows configuration to remove AppX ambiguity Jul 26, 2024

mokagio marked this pull request as ready for review July 26, 2024 05:05

mokagio requested review from codebykat, roundhill and a team July 26, 2024 05:05

mokagio commented Jul 26, 2024

View reviewed changes

AliSoftware approved these changes Aug 8, 2024

View reviewed changes

Add clarifying comment in Makefile

ce94383

Co-authored-by: Olivier Halligon <[email protected]>

mokagio merged commit 98dfd5c into release/2.22.2 Aug 8, 2024
7 checks passed

mokagio deleted the mokagio/fix-windows-code-signing branch August 8, 2024 21:52

mokagio mentioned this pull request Aug 27, 2024

Merge 2.22.2-beta.3 into trunk #3248

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Windows configuration to remove AppX ambiguity #3239

Update Windows configuration to remove AppX ambiguity #3239

mokagio commented Jul 25, 2024 •

edited

Loading

mokagio commented Jul 25, 2024

codebykat commented Jul 25, 2024

roundhill commented Jul 25, 2024

mokagio commented Jul 25, 2024

mokagio Jul 26, 2024

mokagio Jul 26, 2024

mokagio Jul 26, 2024

spencertransier Jul 27, 2024

mokagio Jul 28, 2024

AliSoftware Aug 8, 2024

mokagio Aug 8, 2024

		@npx electron-builder --win -p $(PUBLISH)
		ifeq ($(IS_WINDOWS),true)

		@echo Packaging .appx as well...
		@echo "Packaging appx with dedicated configuration to work around code signing conflicts..."

		"afterSign": "./after_sign_hook.js",
		"afterAllArtifactBuild": "./after_sign_hook.js"

Update Windows configuration to remove AppX ambiguity #3239

Update Windows configuration to remove AppX ambiguity #3239

Conversation

mokagio commented Jul 25, 2024 • edited Loading

What this does

About code signing AppX

About using two configurations

"Proof" that it works

Where to go from here

mokagio commented Jul 25, 2024

codebykat commented Jul 25, 2024

roundhill commented Jul 25, 2024

mokagio commented Jul 25, 2024

mokagio Jul 26, 2024

Choose a reason for hiding this comment

mokagio Jul 26, 2024

Choose a reason for hiding this comment

mokagio Jul 26, 2024

Choose a reason for hiding this comment

spencertransier Jul 27, 2024

Choose a reason for hiding this comment

mokagio Jul 28, 2024

Choose a reason for hiding this comment

AliSoftware Aug 8, 2024

Choose a reason for hiding this comment

mokagio Aug 8, 2024

Choose a reason for hiding this comment

mokagio commented Jul 25, 2024 •

edited

Loading