search

OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
793 stars 237 forks source link

Give stampinf correct DriverVer information #65

Closed jkunkee closed 5 years ago

jkunkee commented 6 years ago

Give stampinf correct DriverVer information

VS2017 has some nice INF-processing infrastructure, but it uses stampinf which insists on adding the DriverVer directive to the INF file itself. This overwrites whatever the INF template produces.

This commit plumbs the INF template values in through stampinf.

Note that the VS2017 GUI field "Driver Version Number" is stored in the project file field TimeStamp.

selvanair commented 6 years ago

@mattock Can we retest with this patch included? This will give a minor merge conflict in OemVista.inf.in. Easily resolved by just accepting "DriverVer=filled in by stampinf" to replace the line coming from PR 48.

jkunkee commented 6 years ago

Note that it doesn't matter what's on that line, so keeping it either way is technically valid (though one is more explanatory).

selvanair commented 6 years ago

Yes, what is added here should be preferred as it documents that date and version will get stamped by stampinf.

cron2 commented 6 years ago

Looks promising...! Thanks, Jon.

selvanair commented 6 years ago

Forgot to mention that I did a rebuild including this patch and checked that the inf processed by stampinf now gets the right date and version as

DriverVer = 07/26/2018,9.22.2.601

By the way, the new tapinstall.exe has acquired a dependency on VCRUNTIME140.dll which cannot be expected to be present.

jkunkee commented 6 years ago

Irritating. The CRT can be statically linked, but it may require changing the project. I’ll take a look.

Sent from my Windows 10 device [15254 - 16.0.10325.20094]

From: Selva Nair Sent: Tuesday, July 31, 2018 12:38 PM To: OpenVPN/tap-windows6 Cc: Jon Kunkee; Author Subject: Re: [OpenVPN/tap-windows6] Give stampinf correct DriverVerinformation (#65)

Forgot to mention that I did a rebuild including this patch and checked that the inf processed by stampinf now gets the right date and version as DriverVer = 07/26/2018,9.22.2.601 By the way, the new tapinstall.exe has acquired a dependency on VCRUNTIME140.dll which cannot be expected to be present. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

jkunkee commented 6 years ago

There are a few options:

  1. We can fork devcon.exe and then it's easy.
  2. We can suggest the change to static linking upstream. I don't know how that might go.
  3. We can attempt command line hackery to force static linking.

I'm working on 3 at the moment.

cron2 commented 6 years ago

Hi,

On Tue, Jul 31, 2018 at 08:50:30PM +0000, Jon Kunkee wrote:

There are a few options:

  1. We can fork devcon.exe and then it's easy.
  2. We can suggest the change to static linking upstream. I don't know how that might go.
  3. We can attempt command line hackery to force static linking.

I'm working on 3 at the moment.

Can we bundle this DLL?

gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany gert@greenie.muc.de

jkunkee commented 6 years ago

The SDK comes with the VC 2017 Redist intended for exactly that purpose, yes. I am fuzzy on the fine print here, but:

jkunkee commented 6 years ago

Option 3 does not look practical.

selvanair commented 6 years ago

Are we allowed to redistribute this runtime or does it have to be downloaded on the target host at install time? If we can't bundle it, we may have to revert back to the old prebuilt binaries until a good solution can be found.

jkunkee commented 6 years ago

Yes: https://docs.microsoft.com/en-us/cpp/ide/redistributing-visual-cpp-files

Sorry I’m not able to do more explication at the present moment.

Sent from my Windows 10 device [15254 - 16.0.10325.20094]

From: Selva Nair Sent: Tuesday, July 31, 2018 6:13 PM To: OpenVPN/tap-windows6 Cc: Jon Kunkee; Author Subject: Re: [OpenVPN/tap-windows6] Give stampinf correct DriverVerinformation (#65)

Are we allowed to redistribute this runtime or does it have to be downloaded on the target host at install time? If we can't bundle it, we may have to revert back to the old prebuilt binaries until a good solution can be found. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

rozmansi commented 6 years ago

AFAIK, you are not allowed to distribute Microsoft's run-time DLLs along your binaries. I remember reading the EULA a few years back, explaining you MUST use either vcredist_x86.exe or MSM merge modules to deploy the VC run-time libraries.

I suggest you stick to the old-reliable version of devcon.exe/tapinstall.exe for the time being. I have MSI packaging for TAP driver finished. MSI packaging for OpenVPN itself should follow soon. None will need devcon.exe.

selvanair commented 6 years ago

AFAIK, you are not allowed to distribute Microsoft's run-time DLLs along your binaries. I remember reading the EULA a few years back, explaining you MUST use either vcredist_x86.exe or MSM merge modules to deploy the VC run-time libraries.

This used to be my understanding too. But as Jon pointed out and based on deployment methods listed here copying the dlls to the applications's directory on target computer is a possible option though not recommended.

_Local deployment, in which you copy particular Visual C++ DLLs from your Visual Studio installation—typically in \Program Files (x86)\Microsoft Visual Studio version\VC\Redist\platform\library\—and install them on target computers in the same folder as the application executable._

Note that files in [VSFOLDER]\VC\Redist are expressly listed as redistributable (unmodified) except for 3 exceptions (files named with suffix _nonredist)

From an end user's perspective using vcredist_x86.exe or merge modules is better as those put the dlls in a central location, won't duplicate already existing ones, Windows update can manage them etc. But in the short run we can either use the old binary or include the runtime dll. Unless arm64 needs a new build, choose whichever is easier.

mattock commented 6 years ago

I agree with @rozmansi that we can cut some corners here and use the old tapinstall.exe/devcon.exe in the next installers. I'm almost certain it won't work on ARM64, but I think that is acceptable in the short-term while moving to MSI.

Am I correct that the next test installer should include everything I had in the previous one plus this PR (as-is) plus old, pre-compiled devcon.exe?

cron2 commented 6 years ago

Hi,

On Thu, Aug 02, 2018 at 08:19:26AM +0000, Samuli Seppänen wrote:

Am I correct that the next test installer should include everything I had in the previous one plus this PR (as-is) plus old, pre-compiled devcon.exe?

This is how i read it, yes.

gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany gert@greenie.muc.de

mattock commented 5 years ago

I assume the "driver version now looks reasonable in device properties" is a side-effect of this PR. If so, it works as it should.

mattock commented 5 years ago

Test installer and signed driver for for Windows 7/8/8.1/Server 2012r2 in case somebody wants to test: