18.2.0 and above don't support non-numeric fourth version component

[gtritchie]

Preflight Checklist

• I have read the contribution documentation for this project.
• I agree to follow the code of conduct that this project follows, as appropriate.
• I have searched the issue tracker for a bug that matches the one I want to file, without success.

Issue Details

• Electron Packager Version:
  • The problem started with 18.2.0 (and 18.3.0, 18.3.1, 18.3.2))
• Electron Version:
  • I've used 28, 29, 30 and hit the problem but don't think the Electron version matters here
• Operating System:
  • Windows-specific (I'm using 11, our failing builds happen on Server 2019)
• Last Known Working Electron Packager version::
  • 18.1.3

Expected Behavior

Prior to 18.2.0, a version string (i.e. in package.json "version") of the form "2024.07.0-hourly+56.pro14" was accepted by electron-packager during the packaging process on Windows. We've been using this format for several years. It continues working for Mac and Linux.

Actual Behavior

Starting with 18.2.0 (presumably due to this PR) an error occurs during packaging: Incorrectly formatted version string: "2024.07.0-hourly+56.pro14". Component "pro14" could not be parsed as an integer

To Reproduce

• Clone https://github.com/gtritchie/my-electron-app (on Windows); this is just the "getting started" tutorial app
• Note that package.json contains the troublesome version string, and pins electron-packager to 18.3.2

npm i
npm run package

Gives the error Incorrectly formatted version string: "2024.07.0-hourly+56.pro14". Component "pro14" could not be parsed as an integer.

Edit package.json and change @electron/packager version to 18.1.3 and:

npm i
npm run package

Succeeds.

Additional Information

NA

[welcome]

👋 Thanks for opening your first issue here! If you have a question about using Electron Packager, read the support docs. If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. Development and issue triage is community-driven, so please be patient and we will get back to you as soon as we can.

To help make it easier for us to investigate your issue, please follow the contributing guidelines.

[MariaSemple]

Some added context - I think the new version parsing code may just need some additional handling for the portion of the version after the +, which is the <build> and can contain alphanumeric strings, ., and -, if you're following the SemVer spec. In the example above 56.pro14 can be treated as one component of the version (the build number).

[erickzhao]

Hey @MariaSemple @gtritchie, looking into this issue it does seem like this is a regression in behaviour, but I think the previous functioning behaviour was unintended.

On Windows, Packager modifies the VERSIONINFO .exe resource, which contains the FILEVERSION and PRODUCTVERSION resources. These resources don't follow semver and instead are of the MAJOR.MINOR.PATCH.BUILD format:

Binary version number for the file. The version consists of two 32-bit integers, defined by four 16-bit integers. For example, "FILEVERSION 3,10,0,61" is translated into two doublewords: 0x0003000a and 0x0000003d, in that order. Therefore, if version is defined by the DWORD values dw1 and dw2, they need to appear in the FILEVERSION statement as follows: HIWORD(dw1), LOWORD(dw1), HIWORD(dw2), LOWORD(dw2).

The buildVersion and appVersion options map to those two properties respectively.

In v18.1.3 and prior, this resource metadata used to be set via the rcedit executable:

packager/src/win32.ts

Lines 45 to 47 in a810162

	if (this.opts.appVersion) {
	rcOpts['product-version'] = rcOpts['file-version'] = this.opts.appVersion;
	}

Under the hood, the previous implementation of this code looked like:

bool parse_version_string(const wchar_t* str, unsigned short *v1, unsigned short *v2, unsigned short *v3, unsigned short *v4) {
  *v1 = *v2 = *v3 = *v4 = 0;
  return (swscanf_s(str, L"%hu.%hu.%hu.%hu", v1, v2, v3, v4) == 4) ||
         (swscanf_s(str, L"%hu.%hu.%hu", v1, v2, v3) == 3) ||
         (swscanf_s(str, L"%hu.%hu", v1, v2) == 2) ||
         (swscanf_s(str, L"%hu", v1) == 1);
}

Reading the code, I think the intent was always to only accept between 1 and 4 period-separated short integers. I think the original parse_version_string implementation might have been using one of the fallback conditions (e.g. parsing 2024.07.0-hourly+56.pro14 as 2024.07.0.0. (Note: can either of you check the produced .exe to confirm if that's the case?)

v18.2.0 refactored the implementation to drop rcedit for a JS-only implementation:

packager/src/resedit.ts

Lines 21 to 33 in d9655d4

	function parseVersionString(str: string): ParsedVersionNumerics {
	const parts = str.split('.');
	if (parts.length === 0 || parts.length > 4) {
	throw new Error(`Incorrectly formatted version string: "${str}". Should have at least one and at most four components`);
	}
	return parts.map((part) => {
	const parsed = parseInt(part, 10);
	if (isNaN(parsed)) {
	throw new Error(`Incorrectly formatted version string: "${str}". Component "${part}" could not be parsed as an integer`);
	}
	return parsed;
	}) as ParsedVersionNumerics;
	}

However, the parseInt implementation is less forgiving than swscanf_s and explicitly throws if NaN is detected rather than using the fallback conditions. I think falling back from 2024.07.0-hourly+56.pro14 to 2024.07.0.0 feels wrong and we should instead properly document the version number constraints for Windows packaging.

I think the path forward for apps should be to set the appVersion and buildVersion properties differently on Windows to abide by the format.

[gtritchie]

Thanks, I can look at tweaking the version info just on our Windows builds to conform to numeric x.x.x.x.

Builds produced with the earlier packager were falling back to .0.0 for the file version as you suggested, but the product version maintained the full string including the non-numeric goo.

Some recent builds produced with the earlier packager:

2024.04.0+735.pro3

2024.07.0-daily+89.pro1