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

extraMetadata: version must be semver on Windows, but can be whatever on macOS and Linux #7173

Closed
faern opened this issue Oct 4, 2022 · 0 comments · Fixed by #7174
Closed
Assignees

Comments

@faern
Copy link
Contributor

faern commented Oct 4, 2022

  • Electron-Builder Version: 23.3.3
  • Node Version: v16.14.2
  • Electron Version: 19.0.13
  • Electron Type (current, beta, nightly): ???
  • Target: Issue exists on Windows. We build for macOS and Linux also.

TLDR:

The following way of injecting our app's version works fine on macOS and Linux, but fails with a semver error on Windows:

distribution.js:

extraMetadata: {
  version: '2022.6'
},

On Windows this results in:

[12:04:15] 'builder-win' errored after 29 s
[12:04:15] TypeError: Invalid Version: 2022.6
    at new SemVer (C:\Users\build\mullvadvpn-app\gui\node_modules\semver\classes\semver.js:38:13)
    at AppInfo.getVersionInWeirdWindowsForm (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\appInfo.ts:78:27)
    at WinPackager.signAndEditResources (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\winPackager.ts:288:46)
    at C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\winPackager.ts:378:21
    at bound (node:domain:421:15)
    at runBound (node:domain:432:12)
From previous event:
    at bound (node:domain:421:15)
    at MappingPromiseArray.runBound (node:domain:432:12)
    at processImmediate (node:internal/timers:464:21)
    at process.topLevelDomainCallback (node:domain:152:15)
    at process.callbackTrampoline (node:internal/async_hooks:128:24)
From previous event:
    at WinPackager.signApp (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\winPackager.ts:376:27)
    at WinPackager.doSignAfterPack (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\platformPackager.ts:329:16)
    at WinPackager.doPack (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\platformPackager.ts:314:7)
    at WinPackager.pack (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\platformPackager.ts:136:5)
    at Packager.doBuild (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\packager.ts:441:9)
    at Object.executeFinally (C:\Users\build\mullvadvpn-app\gui\node_modules\builder-util\src\promise.ts:12:14)
    at Packager._build (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\packager.ts:376:31)
    at Packager.build (C:\Users\build\mullvadvpn-app\gui\node_modules\app-builder-lib\src\packager.ts:337:12)
    at Object.executeFinally (C:\Users\build\mullvadvpn-app\gui\node_modules\builder-util\src\promise.ts:12:14)
[12:04:15] 'pack-win' errored after 57 s

Expected

I expect the version format requirements to 1) be the same everywhere and 2) not need to be semver formatted.

Longer explanation

The version format of our app is $year.$number. For example 2022.6. The version field in package.json requires this field to be semver. So we have always appended a .0 to our version to make it semver compatible. We have a hacky build script that injects this into package.json when building. We now wanted to improve on this hack. We found a solution that works sooo nicely on Linux and macOS, but fails on Windows (see TLDR).

In our actual code, the version string in distribution.js is not hardcoded, it looks like:

version: execSync('cargo run --bin mullvad-version', { encoding: 'utf-8' }).trim()

To use existing Rust code to compute the desired product version. This includes git hashes etc for development builds. Normal versions are 2022.6-beta1, 2022.7-dev-afafaf (-dev-$git_hash[0..6]), but that is besides the point here.

We don't see any reason why applications must have versions compatible with semver. Semver is mostly for libraries IMO, where there is API compatibility to think about.

The relevant code that crashes is here:

const parsedVersion = new SemVer(this.version)
. We don't think this should require the app being semver at all. Can this maybe be replaced with opportunistically trying semver, and if it fails, it fills in some reasonable defaults? The code does not care about more than just major, minor and patch. So it could just split the version string at . and iterate, assigning the first part to major, the second to minor and so fourth, but default to version 0 when the string split iterator comes to an end.

Why allowing non-semver?

So package.json requires the version to be correct semver. Why should extraMetadata: version not when what it does is override the version field in package.json, they should have the same requirements? I'd argue no! Because package.json is for both libraries and applications, but most commonly for libraries. And for libraries it makes a lot of sense to version with semver. npmjs.com would be very messy if packages did not adhere to one versioning scheme. However, electron-builder is for applications, and not libraries. Applications need not be semver, because they don't have a public API to provide stability guarantees for)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants