installer: Fix version of installer and installed file #235

r1walz · 2019-03-01T16:00:24Z

As reported by @JJClements, the File version and Product version of the installer and installed binary were not the same. They are set by the installer/release.sh and by using sdk build (installer | git-and-installer) commands, which created this discrepancy.

Closes: git-for-windows/git#1797

dscho · 2019-03-01T16:34:35Z

installer/install.iss

@@ -74,7 +74,7 @@ PrivilegesRequired=none
 UninstallDisplayIcon={app}\{#MINGW_BITNESS}\share\git\git-for-windows.ico
 #ifndef COMPILE_FROM_IDE
 #if Pos('-',APP_VERSION)>0
-VersionInfoVersion={#Copy(APP_VERSION,1,Pos('-',APP_VERSION)-1)}
+VersionInfoVersion=GetFileVersion('{#MINGW_BITNESS}\bin\git.exe')


Are you sure that this is executed? I could imagine that the APP_VERSION constant is defined somehow without a dash...

Yes. But APP_VERSION is defined through installer/release.sh, right? Anyway, this was just to make you remember what you said (which I think is not the correct approach). According to your words that day, you are actually changing the version of the installer, according to what (binary) is provided with git-sdk, instead of giving it the version what user wants (through installer/release.sh).

Well, the wish was for the file version in the properties of the installer executable to match the file version in the properties of the included git.exe, and I kind of agree...

And yes, APP_VERSION is defined in config.iss that is written by installer/release.sh. But is it not also used for the installer's window title? IOW APP_VERSION is used, but we may not want to use it for the file version of the .exe.

If you change this so that VersionInfoVersion is defined independently from APP_VERSION, but using GetFileVersion() instead, does it work?

But is it not also used for the installer's window title?

Yes, ofc, it is used for window's title and other stuff (regedit). But won't it be a bad UX if user sees different versions, I mean, on title bar and installer/installed bin? What was the intended use of APP_VERSION again? Can't we change the version of file while packing?

If you change this so that VersionInfoVersion is defined independently from APP_VERSION, but using GetFileVersion() instead, does it work?

I can make GetFileVersion() work, but again, it will face the formentioned problem. Also how can we generate git-sdk using git-sdk? (basically mingw64\bin\git.exe)

The file version is limited to four numbers separated by dots, IIUC, the version displayed in the window title is free form. So it can be a lot more expressive.

@dscho my main concern here is UX, I mean if we are showing some different version on the title bar and installing some other version then this is bad(I know that we'll not be shipping such installer but yeah :P).

I'm making it to change the VersionInfoVersion to match the binary included and I guess we shall not be needing the conditional here because then we will get the same problematic installer when using sdk build (installer|git-and-installer) or installer/release.sh <version-string>?

build-extra/installer/install.iss

Lines 76 to 80 in bbe200d

#if Pos('-',APP_VERSION)>0

VersionInfoVersion={#Copy(APP_VERSION,1,Pos('-',APP_VERSION)-1)}

#else

VersionInfoVersion={#APP_VERSION}

#endif

Sounds good.

r1walz · 2019-03-04T10:23:58Z

@dscho can you please review this?

dscho

Could we simplify this further? Have you tested and verified that it works?

dscho · 2019-03-05T12:08:03Z

installer/install.iss

+#define FILE_VERSION GetFileVersion(SourcePath+"\..\..\..\..\" \
+			+MINGW_BITNESS+"\bin\git.exe")
+#define PROD_VERSION GetStringFileInfo(SourcePath+"\..\..\..\..\" \
+			+MINGW_BITNESS+"\bin\git.exe", "ProductVersion")


Why not use ExpandConstant('{#SourceDir}') (or something similar to make use of SourceDir) for both arms, making them identical, i.e. you can move the definition of FILE_VERSION and PROD_VERSION out of the conditional?

r1walz · 2019-03-05T13:35:17Z

Could we simplify this further? Have you tested and verified that it works?

I'll see if we can simplify it further ... Yes, I've verified and it works according to my understandings.

dscho · 2019-03-05T22:32:02Z

I'll see if we can simplify it further

To be clear: I think there is no need to put those #defines of FILE_VERSION and PROD_VERSION into that if ... else ... because I think they should be defined identically, using the SourceDir value, outside that if ... else ....

r1walz · 2019-03-06T09:47:26Z

@dscho I tried changing the code, but it gave error undeclared identifier ExpandConstant, any views?

dscho · 2019-03-06T10:00:53Z

I tried changing the code, but it gave error undeclared identifier ExpandConstant

As discussed on IRC, we'll try to use #define SOURCE_DIR instead, as it is apparently impossible to use an InnoSetup constant to define a pre-processor constant.

r1walz · 2019-03-06T12:47:28Z

Yes, I've tested it, and it works!

r1walz · 2019-03-06T12:56:02Z

Yes, I agree this is prettier.

@JJClements

As reported by @JJClements, the File version and Product version of the installer and installer binary were not the same. They are set by the `installer/release.sh` and by using `sdk build (installer | git-and-installer)` commands, which created this discrepancy. Create two macros, `FILE_VERSION` and `PROD_VERSION` to store File and Product version of the included binary and copy it to the installer file. Closes: git-for-windows/git#1797 Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

dscho · 2019-03-06T13:17:04Z

Excellent, thank you so much!

The file/product version stored in the installer's `.exe` file [now matches the version of the included `git.exe` file's](#235). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

see: git-for-windows/build-extra#235

dscho reviewed Mar 1, 2019

View reviewed changes

r1walz force-pushed the version-fix branch from 8eb8e36 to 5c9f81f Compare March 2, 2019 23:33

r1walz changed the title ~~[WIP] installer: Fix version of installer and installed file~~ installer: Fix version of installer and installed file Mar 2, 2019

dscho reviewed Mar 5, 2019

View reviewed changes

r1walz force-pushed the version-fix branch 2 times, most recently from fecd672 to ed65f19 Compare March 6, 2019 12:17

r1walz force-pushed the version-fix branch from ed65f19 to 08a7430 Compare March 6, 2019 12:57

dscho merged commit 2e879f7 into git-for-windows:master Mar 6, 2019

sendqueery mentioned this pull request Jun 18, 2019

Incorrect version number git-for-windows/git#2223

Closed

r1walz added a commit to r1walz/git-patches that referenced this pull request Jul 2, 2019

add installer fix from build-extra

7221938

see: git-for-windows/build-extra#235

r1walz added a commit to r1walz/git-patches that referenced this pull request Aug 20, 2019

add installer fix from build-extra

bc1e499

see: git-for-windows/build-extra#235

r1walz added a commit to r1walz/git-patches that referenced this pull request Aug 20, 2019

add installer fix from build-extra

1b020f3

see: git-for-windows/build-extra#235

r1walz added a commit to r1walz/git-patches that referenced this pull request Aug 22, 2019

add installer fix from build-extra

eaa57fe

see: git-for-windows/build-extra#235

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

installer: Fix version of installer and installed file #235

installer: Fix version of installer and installed file #235

r1walz commented Mar 1, 2019 •

edited

Loading

dscho Mar 1, 2019

r1walz Mar 1, 2019 •

edited

Loading

dscho Mar 1, 2019

r1walz Mar 1, 2019 •

edited

Loading

dscho Mar 2, 2019

r1walz Mar 2, 2019

dscho Mar 2, 2019

r1walz commented Mar 4, 2019

dscho left a comment

dscho Mar 5, 2019

r1walz commented Mar 5, 2019

dscho commented Mar 5, 2019

r1walz commented Mar 6, 2019

dscho commented Mar 6, 2019

r1walz commented Mar 6, 2019

r1walz commented Mar 6, 2019

dscho commented Mar 6, 2019

	#if Pos('-',APP_VERSION)>0
	VersionInfoVersion={#Copy(APP_VERSION,1,Pos('-',APP_VERSION)-1)}
	#else
	VersionInfoVersion={#APP_VERSION}
	#endif

installer: Fix version of installer and installed file #235

installer: Fix version of installer and installed file #235

Conversation

r1walz commented Mar 1, 2019 • edited Loading

dscho Mar 1, 2019

Choose a reason for hiding this comment

r1walz Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

dscho Mar 1, 2019

Choose a reason for hiding this comment

r1walz Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

dscho Mar 2, 2019

Choose a reason for hiding this comment

r1walz Mar 2, 2019

Choose a reason for hiding this comment

dscho Mar 2, 2019

Choose a reason for hiding this comment

r1walz commented Mar 4, 2019

dscho left a comment

Choose a reason for hiding this comment

dscho Mar 5, 2019

Choose a reason for hiding this comment

r1walz commented Mar 5, 2019

dscho commented Mar 5, 2019

r1walz commented Mar 6, 2019

dscho commented Mar 6, 2019

r1walz commented Mar 6, 2019

r1walz commented Mar 6, 2019

dscho commented Mar 6, 2019

r1walz commented Mar 1, 2019 •

edited

Loading

r1walz Mar 1, 2019 •

edited

Loading

r1walz Mar 1, 2019 •

edited

Loading