Installer support #22

bpintea · 2018-09-12T10:11:24Z

This PR implements some changes needed by the MSI installer:

adds a resource file with various meta properties like version numbers, descriptions, company name, file icon etc.
replaces the .txt license with a .rtf, which can then be rendered as is by the installer.

It also contains some remotely related fixes:

WideCharToMultiByte() string conversion function requires a specific flag for conversion to UTF8, as per documentation. (Interestingly, failure to comply seems to get different treatment in different CRTs, varying from correct functioning to failure with error reporting, but also failure with no error reporting.)
the build.bat will service the setup argument before clean/proper, since these two need to make use of MSBuild.
the HOMEPAGE_URL cmake variable has been a late addition, so not supported by all our CI hosts (some running 3.10 versions vs. latest 3.12).

Various changes for the MSI installer: - resource def file added; - sample DSN no longer added by driver; this is done now by the installer. - driver file name changed to fit in 8 chars. - the license is renamed to .rtf extention.

'clean' uses MSBuild to invoke cleaning target, so it needs the environment set up.

- ..helpful with troubleshooting. - remove old and unused "copying" "function".

- conversion to UTF8 requires WC_ERR_INVALID_CHARS flag (as per documentation). On some CRT versions, failure to use it would result in function failure, but with no detailed error reporting (GetLastError() == 0).

droberts195 · 2018-09-12T10:40:50Z

(Interestingly, failure to comply seems to get different treatment in different CRTs, varying from correct functioning to failure with error reporting, but also failure with no error reporting.)

WideCharToMultiByte() is in kernel32.dll, not the CRT, so I guess its behaviour could vary between different versions of Windows. I would have thought the CRT makes no difference.

droberts195

Most of my comments are about the possibility of replacing magic numbers in the resource file with symbols defined in windows.h.

droberts195 · 2018-09-12T10:47:31Z

LICENSE.rtf

@@ -1,223 +1,228 @@
-ELASTIC LICENSE AGREEMENT


I think the repo should still contain the text version of the license as well as the RTF version. The text version doesn't need to be touched by the build system, but should exist for consistency with the other Elastic repos.

Added the .txt version back (chose to install it as well for readability convenience, hope that's not too redundant).

droberts195 · 2018-09-12T10:54:03Z

driver/driver.rc.cmake

+ PRODUCTVERSION @PROJECT_VERSION_MAJOR@,@PROJECT_VERSION_MINOR@,@PROJECT_VERSION_PATCH@,0
+ FILEFLAGSMASK 0x3fL
+#ifdef _DEBUG
+ FILEFLAGS 0x1L


You could use VS_FF_DEBUG instead of 0x1L

Sure, that's a good idea.
I've used VS's delegating editor, removed the unnecessary parts and modified the relevant ones. The editor uses the defines, but generated .rc uses the values directly (since it doesn't include windows.h originally).

droberts195 · 2018-09-12T10:55:13Z

driver/driver.rc.cmake

+#else
+ FILEFLAGS 0x0L
+#endif
+ FILEOS 0x40004L


Could use VOS_NT_WINDOWS32

droberts195 · 2018-09-12T10:56:54Z

driver/driver.rc.cmake

+ FILEFLAGS 0x0L
+#endif
+ FILEOS 0x40004L
+ FILETYPE 0x2L


Could use VFT_DLL

droberts195 · 2018-09-12T10:59:18Z

driver/driver.rc.cmake

+            VALUE "FileDescription", "ODBC @ENCODING_VERBOSE@ driver for Elasticsearch"
+            VALUE "FileVersion", "@DRV_VERSION@"
+            VALUE "InternalName", "@DRV_NAME@@CMAKE_SHARED_LIBRARY_SUFFIX@"
+            VALUE "LegalCopyright", "Copyright (C) perpetual, Elasticsearch B.V."


I'm not sure this should have the word "perpetual" in it. In other places we don't give a specific year we just say "Copyright (C) Elasticsearch B.V.".

Maybe check with @legalastic.

I've changed it to the suggested wording, I suppose that got their blessing.

droberts195 · 2018-09-12T11:00:40Z

driver/driver.rc.cmake

+VS_VERSION_INFO VERSIONINFO
+ FILEVERSION @PROJECT_VERSION_MAJOR@,@PROJECT_VERSION_MINOR@,@PROJECT_VERSION_PATCH@,0
+ PRODUCTVERSION @PROJECT_VERSION_MAJOR@,@PROJECT_VERSION_MINOR@,@PROJECT_VERSION_PATCH@,0
+ FILEFLAGSMASK 0x3fL


Can use VS_FFI_FILEFLAGSMASK

- also reference it in the README.md

bpintea · 2018-09-12T12:11:16Z

WideCharToMultiByte() is in kernel32.dll, not the CRT, so I guess its behaviour could vary between different versions of Windows. I would have thought the CRT makes no difference.

Indeed, you're correct.

droberts195

LGTM

bpintea · 2018-09-13T19:53:49Z

closes #21 .

bpintea added 8 commits September 11, 2018 18:01

add resource definition file

567bbb7

Various changes for the MSI installer: - resource def file added; - sample DSN no longer added by driver; this is done now by the installer. - driver file name changed to fit in 8 chars. - the license is renamed to .rtf extention.

swap 'setup' and 'clean' handling.

4bcd2e8

'clean' uses MSBuild to invoke cleaning target, so it needs the environment set up.

add functionality to dump lib dependencies

9019ad3

echo cmake version being used

50355bd

- ..helpful with troubleshooting. - remove old and unused "copying" "function".

b/f: tracing was using descriptor W for non-NTS str

e5f69b4

b/f: change flags for WideCharToMultiByte w/ UTF8

79b9b98

- conversion to UTF8 requires WC_ERR_INVALID_CHARS flag (as per documentation). On some CRT versions, failure to use it would result in function failure, but with no detailed error reporting (GetLastError() == 0).

replaced BMP with ICO icon

8eda737

replaced LICENSE.txt with .rtf version

4d09e34

bpintea requested review from droberts195 and edsavage September 12, 2018 10:11

bpintea mentioned this pull request Sep 12, 2018

TODOs #2

Closed

droberts195 reviewed Sep 12, 2018

View reviewed changes

bpintea added 3 commits September 12, 2018 13:53

add LICENSE.txt back to the repo and install it

e48eeca

- also reference it in the README.md

use symbols for properties defs

905c457

change the license prop wording to remove timing

16d1301

droberts195 approved these changes Sep 13, 2018

View reviewed changes

bpintea merged commit 87e782e into elastic:master Sep 13, 2018

bpintea deleted the feature/installer_support branch September 13, 2018 19:54

bpintea added >enhancement v6.5.0 labels May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Installer support #22

Installer support #22

bpintea commented Sep 12, 2018

droberts195 commented Sep 12, 2018

droberts195 left a comment

droberts195 Sep 12, 2018

bpintea Sep 12, 2018

droberts195 Sep 12, 2018

bpintea Sep 12, 2018

droberts195 Sep 12, 2018

bpintea Sep 12, 2018

droberts195 Sep 12, 2018

bpintea Sep 12, 2018

droberts195 Sep 12, 2018

bpintea Sep 12, 2018

droberts195 Sep 12, 2018

bpintea Sep 12, 2018

bpintea commented Sep 12, 2018

droberts195 left a comment

bpintea commented Sep 13, 2018

Installer support #22

Installer support #22

Conversation

bpintea commented Sep 12, 2018

droberts195 commented Sep 12, 2018

droberts195 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Sep 12, 2018

droberts195 left a comment

Choose a reason for hiding this comment

bpintea commented Sep 13, 2018