Add quotes for makensis args. Fix troubles with spaces

OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)

Other

788 stars 237 forks source link

Add quotes for makensis args. Fix troubles with spaces #36

Closed InHavk closed 5 years ago

InHavk commented 7 years ago

Include all available variables

mattock commented 7 years ago

This makes sense. I will run some smoke tests to ensure that this works as intended.

@chipitsine any plans on adding AppVeyor for tap-windows6 :smile:? That way we would not need to manually test that basic build process is not broken.

chipitsine commented 7 years ago

well, currently

1) installer cannot be built without signing certificates

2) those bugs appear only when building package

we should add AppVeyor of course, but it is more complicated

mattock commented 7 years ago

We could use self-signed test certificates. But yes, build testing will be more complex than in openvpn, openvpn-build or openvpn-gui.

chipitsine commented 7 years ago

it looks like a bug. when I want to build an installer without code signing, I mean that, installer without code signing.

mattock commented 7 years ago

Right now the same signing settings are used for the catalog files as well as the package. I think you can build and sign, and then, as a separate step, package without signing. I think it is possible to build the driver with code-signing turned off complete - it just won't work in non-test-mode Windows.

chipitsine commented 7 years ago

yes, it is possible to build driver without signing I could not figure out how to build an installer without signing

jkunkee commented 6 years ago

This change looks like a much smarter version of #51 (which I authored).

selvanair commented 6 years ago

Indeed, replacing system() by subprocess is the way to go..

cron2 commented 5 years ago

What is the status of this? Is this still relevant, is it fine to be merged?

I admit I lost track.

jkunkee commented 5 years ago

This is still relevant and should be fairly straightforward to merge.

Here's how I read it:

1) Switch from os.system() to subprocess.call() when running other programs

simplifies output capture and error handling if desired
I recall something about the arguments being more intuitive for one than the other 2) Instead of explicitly picking values out of kv to pass to makensis using -D, pass all of them
avoids editing buildtap.py if a new value is needed
makes passed values less explicit 3) Add quotes to kv-derived -D flags
prevents values with spaces from breaking the makensis invocation

In #51, I manually injected quotes for PRODUCT_PUBLISHER because it had spaces in it, just like entry 3 here. I feel this PR is a more elegant and general solution to that and the other problems, but it is less obvious at first glance exactly which variables are important.

cron2 commented 5 years ago

Hi,

On Sat, Apr 13, 2019 at 11:02:16PM -0700, Jon Kunkee wrote:

This is still relevant and should be fairly straightforward to merge.

Thanks. It does not merge without conflicts, so I'm leaving this to @mattock to merge - your explanation makes sense, and the code looks good, but since I do not speak python and have no build system to test, I can't verify that the result actually does what I assume it does.

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

This was merged in https://github.com/OpenVPN/tap-windows6/pull/73 -> closing.