Skip to content

Update Windows configuration to remove AppX ambiguity #3239

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 8, 2024
Merged

Update Windows configuration to remove AppX ambiguity #3239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
841cca8
Remove conditional AppX build when on Windows...
mokagio Jul 19, 2024
424339a
Use shared dedicated script to run `npm install` in Windows CI
mokagio Jul 25, 2024
e4672ea
Print path when failing to find Windows certificate in CI
mokagio Jul 25, 2024
0f7e2d8
Reintroduce dedicated config file for AppX build
mokagio Jul 25, 2024
ce94383
Add clarifying comment in `Makefile`
mokagio Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .buildkite/commands/package_windows.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ If (Test-Path $certPath) {
[System.Environment]::SetEnvironmentVariable('CSC_LINK', $certPath, [System.EnvironmentVariableTarget]::Machine)
Write-Host "Environment variable CSC_LINK set to $certPath"
} else {
Write-Host "[!] certificate.pfx file does not exist."
Write-Host "[!] Certificate file does not exist at given path $certPath."
Exit 1
}

Expand All @@ -38,8 +38,8 @@ Import-PfxCertificate -FilePath $certPath -CertStoreLocation Cert:\LocalMachine\
Write-Host "--- :windows: Installing make"
choco install make

Write-Host "--- :npm: Installing dependencies"
npm ci --legacy-peer-deps
bash ".\.buildkite\commands\install_node_dependencies.sh"
If ($LastExitCode -ne 0) { Exit $LastExitCode }

Write-Host "--- :lock_with_ink_pen: Decrypting secrets"
make decrypt_conf
Expand Down
12 changes: 6 additions & 6 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,14 @@ package: build-if-changed

.PHONY: package-win32
package-win32:
@echo Packaging .exe...
@echo "Packaging exe..."
@npx electron-builder --win -p $(PUBLISH)
ifeq ($(IS_WINDOWS),true)
Copy link
Contributor Author

@mokagio mokagio Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@echo Packaging .appx as well...
# Note: the configuration required to generate a code signed exe via the `nsis` target will conflict with the `appx` configuration.
# In practice, "certificateSubjectName": "Automattic, Inc." is required to sign the exe, but if that setting is present and so are the `appx` settings, there will be a failure.
# Hence the need for a separate configuration here.
# See also in https://github.com/electron-userland/electron-builder/issues/6698
@echo "Packaging appx — with dedicated configuration to work around code signing conflicts..."
@npx electron-builder --win -p $(PUBLISH) --config=./electron-builder-appx.json
else
@echo Skipping packaging .appx because we are not running on a Windows machine.
endif

.PHONY: package-osx
package-osx: build-if-changed
Expand Down
8 changes: 3 additions & 5 deletions electron-builder-appx.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
"productName": "Simplenote",
"appId": "com.automattic.simplenote",
"directories": {
"output": "release",
"buildResources": "resources"
"output": "./release",
"buildResources": "./resources"
},
"files": ["desktop", "dist", "shared"],
"win": {
Expand All @@ -29,7 +29,5 @@
"name": "simplenote",
"schemes": ["simplenote"]
}
],
"afterSign": "./after_sign_hook.js",
"afterAllArtifactBuild": "./after_sign_hook.js"
Comment on lines -33 to -34
Copy link
Contributor Author

@mokagio mokagio Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@spencertransier spencertransier Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@mokagio mokagio Jul 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spencertransier correct

]
}
8 changes: 0 additions & 8 deletions electron-builder.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,9 @@
{
"target": "nsis",
"arch": ["ia32", "x64"]
},
{
"target": "appx",
"arch": ["ia32", "x64"]
}
Copy link
Contributor

@AliSoftware AliSoftware Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@mokagio mokagio Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

]
},
"appx": {
"backgroundColor": "transparent",
"showNameOnTiles": true
},
"nsis": {
"artifactName": "Simplenote-win-${version}-${arch}.${ext}",
"deleteAppDataOnUninstall": true,
Expand Down