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

fix (windows) removed NSIS semver build version numeric only validation #12136

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

unknovvn
Copy link
Contributor

@unknovvn unknovvn commented Jan 2, 2025

Closes #8038

@unknovvn unknovvn requested a review from a team as a code owner January 2, 2025 19:38
@unknovvn unknovvn changed the title fix (windows) removed NSIS semver build version only numeric validation fix (windows) removed NSIS semver build version numeric only validation Jan 2, 2025
Copy link
Contributor

github-actions bot commented Jan 2, 2025

Package Changes Through 77ea045

There are 3 changes which include tauri-bundler with patch, tauri-cli with patch, @tauri-apps/cli with patch

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
tauri-bundler 2.2.0 2.2.1
@tauri-apps/cli 2.2.2 2.2.3
tauri-cli 2.2.2 2.2.3

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@FabianLars
Copy link
Member

your first change file was correct. the bot sends the link to create a new one unconditionally at the moment :/

@unknovvn
Copy link
Contributor Author

unknovvn commented Jan 2, 2025

@FabianLars haha, it got me. Gonna remove the bot generated file then. Thanks 🙇

@FabianLars
Copy link
Member

Okay so i just checked again and this won't work. VIProductVersion and VIFileVersion must be numeric only in a X.X.X.X format. This is a requirement by NSIS which if i got that right is because those values are used to set https://learn.microsoft.com/de-de/windows/win32/api/verrsrc/ns-verrsrc-vs_fixedfileinfo?redirectedfrom=MSDN which also requires numeric-only values.

I'm not sure what VS_FixedFileInfo is actually used for though because for example the versions in the file properties' Details tab are set via VIAddVersionKey.

We could modify the check to replace the build/pre data with .0 if it's non-numeric, keeping the "normal" version for VIAddVersionKey and to then wait and see if anything breaks.

@unknovvn
Copy link
Contributor Author

unknovvn commented Jan 3, 2025

I have additionally researched this topic a bit more and it looks like @FabianLars is correct - NSIS VIProductVersion requires all 4 parts to be numbers.
Personally I think that replacing build version with .0 if it's non-numeric is not correct as it's better to throw a build error and indicate that this format is not supported rather than introducing unexpected behavior.

One thing I noticed, that msi convert_version method has additional validation regarding correctness of each version part. Which might be added to nsis in order to unify this behavior.

@FabianLars
Copy link
Member

Personally I think that replacing build version with .0 if it's non-numeric is not correct as it's better to throw a build error and indicate that this format is not supported rather than introducing unexpected behavior.

Yeah, fair. I'm just thinking about how to expose VIAddVersionKey to devs, which supports non-numeric versions, without adding yet another version config.

@FabianLars FabianLars marked this pull request as draft January 3, 2025 23:34
@unknovvn
Copy link
Contributor Author

unknovvn commented Jan 4, 2025

If we would decide to expose build version with non-numeric values as a VIAddVersionKey to devs - it would require interpreting VIProductVersion in some way. Either to add .0 at the end (which would replicate default NSIS behavior) or to add stripped out version of non-numeric build version (remove non-numeric values from the build version). In both cases this would introduce unexpected behavior

@unknovvn
Copy link
Contributor Author

unknovvn commented Jan 4, 2025

Although after checking initial Bug it looks like author of that ticket was expecting that file version would have default .0 revision value instead of non-numeric build version value (probably expecting same behavior for VIProductVersion). In that case it looks like for non-numeric build numbers we could set VIProductVersion and FileVersion with values where non-numeric build version would be replaced by .0 and for product version it would return original build version

@unknovvn unknovvn marked this pull request as ready for review January 9, 2025 13:25
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 this pull request may close these issues.

[bug] NSIS bundler: semver version metadata is required to be numeric, but shouldn't be
2 participants