Include version info in Installer by SinghRajenM · Pull Request #2409 · notepad-plus-plus/notepad-plus-plus

SinghRajenM · 2016-10-09T14:51:48Z

As of now, there is no version info available on the detail section of installer properties.
It is good to have version info.

justanotheranonymoususer · 2016-10-11T08:30:03Z

PowerEditor/installer/nsisInclude/gobalDef.nsh


+!define CompanyName		"Don HO don.h@free.fr"
+!define Description		"Notepad++ : a free (GNU) source code editor"
+!define Version 		"7.0.0.0"


This is yet another place to modify when a new version comes out. Why not use the macros above?

If you are talking about existing macro APPVERSION, it can't be used directly as VIProductVersion expect version string X.X.X.X. We have to write many conditions to utilize APPVERSION.
Reason is:
In V7 APPVERSION = 7
In V6.9.2 APPVERSION = 6.9.2
In V6.9 APPVERSION = 6.9

I think he ment using something like ...

!define Version VERSION_MAJOR "." VERSION_MINOR

instead of ...

!define Version "7.0.0.0"

However, I was thinking why can't we remove macro APPVERSION (VERSION_MAJOR and VERSION_MINOR as well). Instead of changing its value for every release, just read the version info from notepad++.exe and use it.

This requires an additional nsis file (get version of npp exe and write version info in normal text file in form of macro with required format.) and its output (exe) file.
Later include call this exe in beginning of nsi script to generate text file and use these defined macro from the text file.
We have used this approach in many project. This will reduce the manual effort of writing version info everytime.

@MAPJe71 Initially, I though to utilizing existing macros, but when I saw the history of these macros, I could not use these without writing conditions as the value. Consider V6.9.2, then major is 6, minor is 92. So break 92 as 9.2 and append .0 (6.9.2.0) while in V7 major is 7, minor is 0, so append .0.0 (so 7.0.0.0).

Reduce human effort by reading verion info from npp exe itself and use it

SinghRajenM · 2016-10-12T20:14:01Z

PowerEditor/installer/nsisInclude/gobalDef.nsh


-!define APPVERSION "7"
+!system "nsisInclude\GetVersion.exe"
+!include "VersionInfo.txt"


Now, version info can be read from npp.exe itself. No need to update this file for every release (at-least for version change).

MAPJe71

I assume the GetVersion.exe binary is generated from GetVersion.nsi during the build process so there's no need to add the binary to the repository.

MAPJe71 · 2016-10-12T23:04:17Z

PowerEditor/installer/nsisInclude/GetVersion.nsi

+	SetAutoClose true	; it simply writes version info in the text. So close immediatly after writing version info
+
+    DetailPrint "Getting version from ${EXEPATH}"
+    GetDllVersion "${EXEPATH}" $R0 $R1 	; Two values were read during compilation


The trailing comment doesn't make sense to me ...

which two values?

during compilation of what?

Two values $ R0 and $ R1 which will be extracted from npp.exe.

I get that the two $Rx variables/registers will contain values that are read from Notepad++ (i.e. ${EXEPATH}) but it's not clear what's the meaning of these values:

ProductVersion and FileVersion?

major and minor version?

...

I think below screenshot will answer your queries.

No still doesn't answer my question!

I can only presume that you mean that $R0 will contain the file version i.e. 6.9.2.0 and $R1 will contain the Product name i.e. Notepad++ as that are the values in the red box.
If that's the case I would have written the line of code as ...

GetDllVersion "${EXEPATH}" $R0 $R1 ; Get file version and product name respectively

No...no
Say npp.exe version 6.9.2.0, then
$R0 = 6.9 $R1=2.0

Below link you give you more insight
http://nsis.sourceforge.net/GetDllVersion_Command_Explained

From URL
About numbers GetDllVersion. First 16 bit of first number is major version. Second 16 bit of first number is minor version. First 16 bit of second number is release version. Second 16 bit of second number is build version.

GetDLLVersion "MyApp.exe" $R0 $R1
IntOp $R2 $R0 >> 16
IntOp $R2 $R2 & 0x0000FFFF ; $R2 now contains major version
IntOp $R3 $R0 & 0x0000FFFF ; $R3 now contains minor version
IntOp $R4 $R1 >> 16
IntOp $R4 $R4 & 0x0000FFFF ; $R4 now contains release
IntOp $R5 $R1 & 0x0000FFFF ; $R5 now contains build
StrCpy $0 "$R2.$R3.$R4.$R5" ; $0 now contains string like "1.2.0.192"

Finally the explanation I was looking for 👍
I suggest ...

GetDllVersion "${EXEPATH}" $R0 $R1 ; Get 32-bit values with major+minor and release+build respectively

MAPJe71 · 2016-10-12T23:14:19Z

PowerEditor/installer/nsisInclude/GetVersion.nsi

+    GetDllVersion "${EXEPATH}" $R0 $R1 	; Two values were read during compilation
+
+	;${GetFileVersion} "${EXEPATH}" $R0
+	;DetailPrint "Another way to get version: $R0"


Why another way?
Pick one and don't mention other way(s) or mention the other way(s) and comment/explain why it's not the preferred way. Don't create confusion!

I don't think commented code creates confusion. If it really bothers people I'll remove commented code.

Why did you add it?

I was trying different options and commented code was one of them. Instead removing, I just commented. Nothing more than that.

As I wrote before pick one method/solution to be included in the official repository through a PR.
Keep tries to your local development environment.

MAPJe71 · 2016-10-12T23:18:49Z

PowerEditor/installer/nsisInclude/GetVersion.nsi

+	; 	$1 = 7.0.0.0 (File Version)
+	;	$2 = 7.1.2 	 (Product Version)
+	;	$3 = 12		 (Minor Version)
+


I think it's wise to get confirmation about the meaning of the different parts of the version number first i.e. before issuing a PR.

There is no assumption. These all are facts from the previous releases

In V7 APPVERSION = 7 major=7 minor=0
In V6.9.2 APPVERSION = 6.9.2 major=6 minor=92
In V6.9 APPVERSION = 6.9 major=6 minor=9

Correct about APPVERSION but AFAIK FileVersion and ProductVersion are always the same for Notepad++.

No. They are not same. Refer the screenshot (attached above) of actual notepad++.exe. You will find the difference.
I think that make sense.

Product version and File version are the same (see Notepad++ resource file).
But there's a difference in notation for the VERSION_DIGITALVALUE and the VERSION_VALUE.
The property list of the Notepad++ v6.9.2.0 executable will show the digital value (i.e. comma-separated-digits) as dot-separated-digits (e.g. 6.9.2.0) and the textual value will be visible as defined in the resource file i.e. 6.92.

Maybe it's possible for the NSIS script to read the version directly from the Notepad++ resource.h file?

Need to investigate if it is possible to extract from resource.h.

URL: https://msdn.microsoft.com/en-us/library/windows/desktop/aa381058(v=vs.85).aspx might be helpful to understand versioning.

What Windows version is that?
Strange that it shows the digital/fixed value for File version and textual/variable for Product version.

Yep, I've used automatic generated resource info in projects based on the linked MS info.
Notepad++ version numbering is not the same as MS tends to do it.
Here are some different methods I've seen over the years:

- Windows uses : major.minor[.build[.revision]] - .NET uses : major.minor[.revision[.build]] - other used formats : major.minor[.maintenance[.build]] major.minor[.build[.((majorRevision && 0x00FF)<<8)||(minorRevision && 0x00FF)]] major.minor[.patch[.stage]]

More recent I've encountered projects mentioning they use Semantic Versioning.

MAPJe71 · 2016-10-12T23:24:26Z

PowerEditor/installer/nsisInclude/GetVersion.nsi

+		StrCpy $2 "$R2.$R3.$R4.$R5"
+		StrCpy $3 "$R3$R4$R5"
+	${Endif}
+


Would be nice if it's possible to give the variables explanatory names.

I use these register variables because
: No declaration needed (var blah).
: Less LOC
: No name conflict (not applicable here very less code)
: Bit faster than normal variable

I don't think it's a time critical part and self explanatory code is preferred (see Style Guide)

This is way most of the nsis scripts are written. I feel these were introduce to simplify the task, otherwise there was no need to introduce registers $0~$9 and $R0~$R9.

SinghRajenM · 2016-10-13T05:24:22Z

Yes. It is generated from GetVersion.nsi but not during the build process.
This is one time activity and no need to again n again unless there is a
bug or some changes are required.

On Oct 13, 2016 4:27 AM, "Menno Vogels" notifications@github.com wrote:

@MAPJe71 requested changes on this pull request.

I assume the GetVersion.exe binary is generated from GetVersion.nsi
during the build process so there's no need to add the binary to the
repository.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2409 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AOGzJXtW_aeu-gCt9QZ9x1rtSrwveRXEks5qzWXcgaJpZM4KSBM0
.

MAPJe71 · 2016-10-13T12:56:26Z

IMO binaries in a repository are security liability but sometimes you can't get around them e.g. 3rd party libraries/tools. In this case the source is part of the project so I'd say build it during the build process but I'll leave that decision to @donho.

SinghRajenM · 2016-10-13T16:17:46Z

In this case the source is part of the project so I'd say build it during the build process

I feel it is not that much easy, moreover I don't how appveyor is configured. But if I take an example from Jenkins, it requires two jenkin jobs.

Complie GetVersion.nsi which creates GetVersion.exe.
Actuall npp build job

In this case, job 1 will be slave job of job 2. So first job 2 should trigger job 1. Once job 1 is completed, move the artifact (GetVersion.exe) from job 1 to job2 and execute it which will write version info in form of macros.

MAPJe71 · 2016-10-13T16:46:42Z

Notepad++ build already has two dependencies, the Boost and Scintilla libraries, so it shouldn't be a problem for the AppVeyor build environment.
Furthermore, I don't see a procedural difference in building an executable from NSIS script source or C++ source.

SinghRajenM · 2016-10-13T16:55:24Z

Notepad++ build already has two dependencies, the Boost and Scintilla libraries, so it shouldn't be a problem for the AppVeyor build environment.

If so, then GetVersion.nsi should be built in different job and artifact can be moved to "PowerEditor\installer\nsisInclude" before starting npp setup creation build.

Sorry, I don't how to create jobs and dependencies in AppVeyor as we have dedicated build engineers.

cmeriaux · 2016-10-13T19:38:04Z

About AppVeyor, the configuration is here : notepad-plus-plus/appveyor.yml link
There is one job ( see build_script section) that is executed with a matrix of parameters ( platform and configuration )
The job execute visual studio with this command :

msbuild notepadPlus.vcxproj /p:configuration="%configuration%" /p:platform="%platform%"

You can execute before this command the compliation of GetVersion.nsi then use your getVersion program to do what you want to do.

However, appVeyor is not used to build the notepad++ installer. It can't due to code certification.
appVeyor doesn't compile with boost support.

I hope it helps you

cmeriaux · 2016-10-13T19:45:16Z

I'd like also to have the git version to identify each version available (in case of people uses a nigthly build)
Git offer a command to do that : git describe

[21:42:41] christophe@Christophe-PC /cygdrive/d/dev/notepad-plus-plus  master
: git describe --tags --long
v7-36-g4fec265

=> My repository HEAD is on version 7, 36 commits since tags, sha1 = 4fec265

donho · 2016-11-12T10:23:04Z

It's a small "Nice to have" feature. However, it's not worth to me to integrate all the codes for adding this small feature.

SinghRajenM · 2016-11-12T14:18:29Z

No problem!!!

justanotheranonymoususer reviewed Oct 11, 2016

View reviewed changes

Writing file version information in the installer

42f342b

Reduce human effort by reading verion info from npp exe itself and use it

SinghRajenM force-pushed the NPP_SETUP_VER_INFO branch from c5bb9bd to 42f342b Compare October 12, 2016 19:37

SinghRajenM commented Oct 12, 2016

View reviewed changes

MAPJe71 suggested changes Oct 12, 2016

View reviewed changes

donho closed this Nov 12, 2016

SinghRajenM deleted the NPP_SETUP_VER_INFO branch November 12, 2016 14:18

SinghRajenM mentioned this pull request Dec 5, 2016

Launch N++ with same privileges as explorer #2643

Closed

Conversation

SinghRajenM commented Oct 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SinghRajenM Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SinghRajenM Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MAPJe71 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SinghRajenM Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MAPJe71 Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MAPJe71 Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MAPJe71 Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SinghRajenM commented Oct 13, 2016

Uh oh!

SinghRajenM Oct 11, 2016 •

edited

Loading

SinghRajenM Oct 12, 2016 •

edited

Loading

SinghRajenM Oct 13, 2016 •

edited

Loading

MAPJe71 Oct 13, 2016 •

edited

Loading

MAPJe71 Oct 13, 2016 •

edited

Loading

MAPJe71 Oct 13, 2016 •

edited

Loading