-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 checkout behavior and FileVersion #3337
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3337 +/- ##
=======================================
Coverage 57.99% 57.99%
=======================================
Files 302 302
Lines 21764 21764
=======================================
Hits 12621 12621
Misses 8219 8219
Partials 924 924 |
Actually wondering now; shouldn't it show the commit? Because the commit it was built from is not an exact match for a tag (see the ProductVersion) |
You mean for Edit: Yes should be like |
d8271a1
to
41446ab
Compare
@thaJeztah Fixed |
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
41446ab
to
dbac826
Compare
"FileVersion": "${VERSION_QUAD}", | ||
"FileVersion": "${VERSION}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this previously used VERSION_QUAD
, is this field expected to be SemVer(ish)? If so, this will break if our CalVer version has a leading 0
(19.03.9
, e.g.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from https://docs.microsoft.com/en-us/windows/win32/menurc/versioninfo-resource
FILEVERSION
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).
PRODUCTVERSION
Binary version number for the product with which the file is distributed. The version parameter is two 32-bit integers, defined by four 16-bit integers. For more information about version, see the FILEVERSION description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further down, the description is different (sigh)
FileVersion
Version number of the file for example, "3.10" or "5.00.RC2". This string is required.
ProductVersion
Version of the product with which the file is distributed?for example, "3.10" or "5.00.RC2". This string is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further down, the description is different (sigh)
Yeah can be misleading, VERSIONINFO.FILEVERSION
!= StringFileInfo.FileVersion
. In our case:
VERSIONINFO.FILEVERSION
= goversioninfo FixedFileInfo.FileVersion
"FixedFileInfo":
{
"FileVersion": {
"Major": $(echo "$VERSION_QUAD" | cut -d. -f1),
"Minor": $(echo "$VERSION_QUAD" | cut -d. -f2),
"Patch": $(echo "$VERSION_QUAD" | cut -d. -f3),
"Build": $(echo "$VERSION_QUAD" | cut -d. -f4)
},
}
StringFileInfo.FileVersion
= goversioninfo StringFileInfo.FileVersion
"StringFileInfo":
{
"FileVersion": "${VERSION}",
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh.. yes, that's confusing, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Looks like
cli/scripts/build/.variables
Line 7 in 6d2820b
Seems related to GitHub Actions checkout behavior. We need to fetch all tags otherwise it fails:
cc @thaJeztah
Signed-off-by: CrazyMax crazy-max@users.noreply.github.com