Skip to contentSkip to conversation

travis-ci: add 2 mingw "build only configurations"

PATCH In review Ilya Shipitsin 4 participants 12 messages Mar 26, 2017 – Apr 10, 2017

lore
Share & tools
lore

Ilya Shipitsin

author
#0 lore raw

PATCH 1/1 travis-ci: add 2 mingw "build only configurations"

Inspired by https://www.mail-archive.com/[email protected]/msg13032.html
build options are taken from regular windows installer builds

Signed-off-by Ilya Shipitsin <[email protected]...>
---
 .travis.yml           | 30 +++++++++++++++++++++-------
 .travis/build-deps.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 73 insertions(+), 12 deletions(-)
.travis.yml +23 -7
index 3c0aa7d..9ab30a2 100644
@@ -12,6 +12,9 @@ env:
   global:
     - JOBS=3
     - PREFIX="${HOME}/opt"
+    - TAP_WINDOWS_VERSION=9.21.2
+    - LZO_VERSION=2.10
+    - PKCS11_HELPER_VERSION=1.11
     - MBEDTLS_VERSION="2.4.0"
     - MBEDTLS_CFLAGS="-I${PREFIX}/include"
     - MBEDTLS_LIBS="-L${PREFIX}/lib -lmbedtls -lmbedx509 -lmbedcrypto"

@@ -50,6 +53,12 @@ matrix:
       os: osx
       osx_image: xcode7.3
       compiler: clang
+    - env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32
+      os: linux
+      compiler: ": Win64 build only"
+    - env: SSLLIB="openssl" CHOST=i686-w64-mingw32
+      os: linux
+      compiler: ": Win32 build only"
   exclude:
     - compiler: gcc
 

@@ -60,6 +69,7 @@ addons:
       - libpam0g-dev
       - liblz4-dev
       - linux-libc-dev
+      - man2html
 
 cache:
   ccache: true

@@ -72,16 +82,22 @@ before_install:
   - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew install lzo; fi
 
 install:
+  - if [ ! -z "${CHOST+xxx}" ]; then unset CC; unset CXX; fi
   - .travis/build-deps.sh > build-deps.log 2>&1 || (cat build-deps.log && exit 1)
 
 script:
   - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH}"; fi
   - if [ "${TRAVIS_OS_NAME}" = "osx"   ]; then export DYLD_LIBRARY_PATH="${PREFIX}/lib:${DYLD_LIBRARY_PATH}"; fi
   - autoreconf -vi
-  - ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} || (cat config.log && exit 1)
-  - make -j$JOBS
-  - src/openvpn/openvpn --version || true
-  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn; fi
-  - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L src/openvpn/openvpn; fi
-  - make check
-  - $EXTRA_SCRIPT
+  - if [ -z "${CHOST+xxx}" ]; then
+      ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} || (cat config.log && exit 1);
+      make -j$JOBS;
+      src/openvpn/openvpn --version || true;
+      if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn; fi;
+      if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L src/openvpn/openvpn; fi;
+      make check;
+      $EXTRA_SCRIPT;
+    else
+      TAP_CFLAGS="-I${PWD}/tap-windows-${TAP_WINDOWS_VERSION}/include" LZO_CFLAGS="-I${PREFIX}/include" LZO_LIBS="-L${PREFIX}/lib -llzo2" PKCS11_HELPER_LIBS="-L${PREFIX}/lib -lpkcs11-helper" PKCS11_HELPER_CFLAGS="-I${PREFIX}/include" ./configure --host=${CHOST} --build=x86_64-pc-linux-gnu --enable-pkcs11 --disable-plugins || (cat config.log && exit 1);
+      make -j$JOBS;
+    fi
.travis/build-deps.sh +50 -5

90 lines not on this page · raw message

Signature · 1 line
2.5.5
author
#1 lore raw

I opened https://github.com/OpenVPN/openvpn/pull/85
for discussion

thoughts ?

2017-03-26 16:21 GMT+05:00 Ilya Shipitsin <[email protected]...>:

Quoted 208 lines

Inspired by https://www.mail-archive.com/[email protected]...
sourceforge.net/msg13032.html
build options are taken from regular windows installer builds

Signed-off-by Ilya Shipitsin <[email protected]...>
---
 .travis.yml           | 30 +++++++++++++++++++++-------
 .travis/build-deps.sh | 55 ++++++++++++++++++++++++++++++
++++++++++++++++-----
 2 files changed, 73 insertions(+), 12 deletions(-)
.travis.yml +11 -0
index 3c0aa7d..9ab30a2 100644
@@ -12,6 +12,9 @@ env:
   global:
     - JOBS=3
     - PREFIX="${HOME}/opt"
+    - TAP_WINDOWS_VERSION=9.21.2
+    - LZO_VERSION=2.10
+    - PKCS11_HELPER_VERSION=1.11
     - MBEDTLS_VERSION="2.4.0"
     - MBEDTLS_CFLAGS="-I${PREFIX}/include"
     - MBEDTLS_LIBS="-L${PREFIX}/lib -lmbedtls -lmbedx509 -lmbedcrypto"

@@ -50,6 +53,12 @@ matrix:
       os: osx
       osx_image: xcode7.3
       compiler: clang
+    - env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32
+      os: linux
+      compiler: ": Win64 build only"
+    - env: SSLLIB="openssl" CHOST=i686-w64-mingw32
+      os: linux
+      compiler: ": Win32 build only"
   exclude:
     - compiler: gcc
 

@@ -60,6 +69,7 @@ addons:
       - libpam0g-dev
       - liblz4-dev
       - linux-libc-dev
+      - man2html
 
 cache:
   ccache: true

@@ -72,16 +82,22 @@ before_install:
   - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew install lzo; fi
 
 install:
+  - if [ ! -z "${CHOST+xxx}" ]; then unset CC; unset CXX; fi
   - .travis/build-deps.sh > build-deps.log 2>&1 || (cat build-deps.log &&
 exit 1)
 
 script:
   - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export

LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH}"; fi
- if [ "${TRAVIS_OS_NAME}" = "osx" ]; then export
DYLD_LIBRARY_PATH="${PREFIX}/lib:${DYLD_LIBRARY_PATH}"; fi
- autoreconf -vi
- - ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} || (cat config.log && exit 1)

.travis.yml +0 -4
-  - make -j$JOBS
-  - src/openvpn/openvpn --version || true
-  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn; fi
-  - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L

src/openvpn/openvpn; fi

.travis.yml +2 -2
-  - make check
-  - $EXTRA_SCRIPT
+  - if [ -z "${CHOST+xxx}" ]; then
+      ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} ||

(cat config.log && exit 1);

.travis.yml +3 -0
+      make -j$JOBS;
+      src/openvpn/openvpn --version || true;
+      if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn;

fi;
+ if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L
src/openvpn/openvpn; fi;

.travis.yml +4 -0
+      make check;
+      $EXTRA_SCRIPT;
+    else
+      TAP_CFLAGS="-I${PWD}/tap-windows-${TAP_WINDOWS_VERSION}/include"

LZO_CFLAGS="-I${PREFIX}/include" LZO_LIBS="-L${PREFIX}/lib -llzo2" PKCS11_HELPER_LIBS="-L${PREFIX}/lib -lpkcs11-helper" PKCS11_HELPER_CFLAGS="-I${PREFIX}/include" ./configure --host=${CHOST} --build=x86_64-pc-linux-gnu --enable-pkcs11 --disable-plugins || (cat config.log && exit 1);

.travis.yml +2 -0
+      make -j$JOBS;
+    fi
.travis/build-deps.sh +5 -2
index 3ffba0b..18f40ec 100755
@@ -31,7 +31,6 @@ download_openssl () {
 }
 
 build_openssl_linux () {
-    tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
     (
         cd "openssl-${OPENSSL_VERSION}/"
         ./config shared --openssldir="${PREFIX}" -DPURIFY

@@ -40,7 +39,6 @@ build_openssl_linux () {
 }
 
 build_openssl_osx () {
-    tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
     (
         cd "openssl-${OPENSSL_VERSION}/"
         ./Configure darwin64-x86_64-cc shared \

@@ -49,9 +47,25 @@ build_openssl_osx () {
     )
 }
 
+build_openssl_mingw () {
+    (
+        cd "openssl-${OPENSSL_VERSION}/"
+
+        if [ "${CHOST}" = "i686-w64-mingw32" ]; then export target=mingw;

fi
+ if [ "${CHOST}" = "x86_64-w64-mingw32" ]; then export
target=mingw64; fi

.travis/build-deps.sh +7 -1
+
+        ./Configure --cross-compile-prefix=${CHOST}- shared \
+           $target no-multilib no-capieng --openssldir="${PREFIX}"
-static-libgcc
+        make install
+    )
+}
+
 build_openssl () {
     if [ "$(cat ${PREFIX}/.openssl-version)" != "${OPENSSL_VERSION}" ];

then

.travis/build-deps.sh +8 -3
-        if [ "${TRAVIS_OS_NAME}" = "osx" ]; then
+        tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
+        if [ ! -z ${CHOST+xxx} ]; then
+            build_openssl_mingw
+        elif [ "${TRAVIS_OS_NAME}" = "osx" ]; then
             build_openssl_osx
         elif [ "${TRAVIS_OS_NAME}" = "linux" ]; then
             build_openssl_linux

@@ -61,7 +75,7 @@ build_openssl () {
 }
 
 # Enable ccache
-if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
+if [ "${TRAVIS_OS_NAME}" != "osx" ] && [ -z "${CHOST+xxx}" ]; then
     # ccache not available on osx, see:
     # https://github.com/travis-ci/travis-ci/issues/5567
     mkdir -p "${HOME}/bin"

@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu xenial main universe" |

sudo tee -a /etc/apt/sources.list.d/xenial.list

.travis/build-deps.sh +4 -1
+      echo "deb http://archive.ubuntu.com/ubuntu xenial main" | sudo tee
-a /etc/apt/sources.list.d/xenial.list
+      sudo apt-get update
+      sudo apt-get -y install dpkg mingw-w64
+      if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip"

]; then
+ wget -P download-cache/ http://build.openvpn.net/
downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip

.travis/build-deps.sh +5 -0
+      fi
+      unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip
+
+      (
+      wget -P download-cache/ http://www.oberhumer.com/

opensource/lzo/download/lzo-${LZO_VERSION}.tar.gz

.travis/build-deps.sh +10 -1
+      tar zxf download-cache/lzo-${LZO_VERSION}.tar.gz
+      cd lzo-${LZO_VERSION}
+
+      ./configure --host=${CHOST} --program-prefix='' --enable-shared
--libdir=${PREFIX}/lib --prefix=${PREFIX} --build=x86_64-pc-linux-gnu
+      make
+      make install
+      )
+
+      (
+      wget -P download-cache/ http://downloads.sourceforge.

net/project/opensc/pkcs11-helper/pkcs11-helper-${PKCS11_ HELPER_VERSION}.tar.bz2
+ tar jxf download-cache/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.
bz2

.travis/build-deps.sh +8 -3
+      cd pkcs11-helper-${PKCS11_HELPER_VERSION}
+
+      ./configure --host=${CHOST} --program-prefix='' --enable-shared
--libdir=${PREFIX}/lib --prefix=${PREFIX} --build=x86_64-pc-linux-gnu
--disable-crypto-engine-gnutls --disable-crypto-engine-nss
+      make
+      make install
+      )
+
+fi
--

2.5.5

#3 lore raw
In reply to Ilya Shipitsin

Hi,

Quoted 2 lines · Ilya Shipitsin

Inspired by https://www.mail-archive.com/[email protected]/msg13032.html
build options are taken from regular windows installer builds

Feature-ACK. Adding cross-compile builds for Windows on travis is a good plan.

Initial feedback below.

Quoted 43 lines
Signed-off-by Ilya Shipitsin <[email protected]...>
---
 .travis.yml           | 30 +++++++++++++++++++++-------
 .travis/build-deps.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 73 insertions(+), 12 deletions(-)
.travis.yml +10 -0
index 3c0aa7d..9ab30a2 100644
@@ -12,6 +12,9 @@ env:
   global:
     - JOBS=3
     - PREFIX="${HOME}/opt"
+    - TAP_WINDOWS_VERSION=9.21.2
+    - LZO_VERSION=2.10
+    - PKCS11_HELPER_VERSION=1.11
     - MBEDTLS_VERSION="2.4.0"
     - MBEDTLS_CFLAGS="-I${PREFIX}/include"
     - MBEDTLS_LIBS="-L${PREFIX}/lib -lmbedtls -lmbedx509 -lmbedcrypto"

@@ -50,6 +53,12 @@ matrix:
       os: osx
       osx_image: xcode7.3
       compiler: clang
+    - env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32
+      os: linux
+      compiler: ": Win64 build only"
+    - env: SSLLIB="openssl" CHOST=i686-w64-mingw32
+      os: linux
+      compiler: ": Win32 build only"
   exclude:
     - compiler: gcc
 

@@ -60,6 +69,7 @@ addons:
       - libpam0g-dev
       - liblz4-dev
       - linux-libc-dev
+      - man2html
 
 cache:
   ccache: true

.travis.yml before_install: · line 85

Quoted 4 lines
.travis.yml +1 -0
@@ -72,16 +82,22 @@ before_install:
   - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew install lzo; fi
 
 install:
+  - if [ ! -z "${CHOST+xxx}" ]; then unset CC; unset CXX; fi

What is the function of this +xxx ? It seems to me that all you need is "if [ ! -z "${CHOST}" ]".

Quoted 13 lines
.travis.yml +0 -7
   - .travis/build-deps.sh > build-deps.log 2>&1 || (cat build-deps.log && exit 1)
 
 script:
   - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH}"; fi
   - if [ "${TRAVIS_OS_NAME}" = "osx"   ]; then export DYLD_LIBRARY_PATH="${PREFIX}/lib:${DYLD_LIBRARY_PATH}"; fi
   - autoreconf -vi
-  - ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} || (cat config.log && exit 1)
-  - make -j$JOBS
-  - src/openvpn/openvpn --version || true
-  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn; fi
-  - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L src/openvpn/openvpn; fi
-  - make check
-  - $EXTRA_SCRIPT

.travis.yml before_install: · line 101

Quoted 10 lines
.travis.yml +10 -0
+  - if [ -z "${CHOST+xxx}" ]; then
+      ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} || (cat config.log && exit 1);
+      make -j$JOBS;
+      src/openvpn/openvpn --version || true;
+      if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn; fi;
+      if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L src/openvpn/openvpn; fi;
+      make check;
+      $EXTRA_SCRIPT;
+    else
+      TAP_CFLAGS="-I${PWD}/tap-windows-${TAP_WINDOWS_VERSION}/include" LZO_CFLAGS="-I${PREFIX}/include" LZO_LIBS="-L${PREFIX}/lib -llzo2" PKCS11_HELPER_LIBS="-L${PREFIX}/lib -lpkcs11-helper" PKCS11_HELPER_CFLAGS="-I${PREFIX}/include" ./configure --host=${CHOST} --build=x86_64-pc-linux-gnu --enable-pkcs11 --disable-plugins || (cat config.log && exit 1);

All these _CFLAGS and _LIBS should go into the env: section above.

Any reason to --enable-pkcs11 for the windows builds, while that's not done for the non-windows builds?

Quoted 25 lines
.travis.yml +2 -0
+      make -j$JOBS;
+    fi
.travis/build-deps.sh +0 -2
index 3ffba0b..18f40ec 100755
@@ -31,7 +31,6 @@ download_openssl () {
 }
 
 build_openssl_linux () {
-    tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
     (
         cd "openssl-${OPENSSL_VERSION}/"
         ./config shared --openssldir="${PREFIX}" -DPURIFY

@@ -40,7 +39,6 @@ build_openssl_linux () {
 }
 
 build_openssl_osx () {
-    tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
     (
         cd "openssl-${OPENSSL_VERSION}/"
         ./Configure darwin64-x86_64-cc shared \

@@ -49,9 +47,25 @@ build_openssl_osx () {
     )
 }

.travis/build-deps.sh build_openssl_osx () { · line 58

Quoted 10 lines
.travis/build-deps.sh +9 -0
 
+build_openssl_mingw () {
+    (
+        cd "openssl-${OPENSSL_VERSION}/"
+
+        if [ "${CHOST}" = "i686-w64-mingw32" ]; then export target=mingw; fi
+        if [ "${CHOST}" = "x86_64-w64-mingw32" ]; then export target=mingw64; fi
+
+        ./Configure --cross-compile-prefix=${CHOST}- shared \
+           $target no-multilib no-capieng --openssldir="${PREFIX}" -static-libgcc

Can you try to keep style consistent? I.e. use all-caps variable names (TARGET instead of target) and use quoting ("${TARGET}" instead of $target).

Quoted 15 lines
.travis/build-deps.sh +8 -1
+        make install  
+    )
+}
+
 build_openssl () {
     if [ "$(cat ${PREFIX}/.openssl-version)" != "${OPENSSL_VERSION}" ]; then
-        if [ "${TRAVIS_OS_NAME}" = "osx" ]; then
+        tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
+        if [ ! -z ${CHOST+xxx} ]; then
+            build_openssl_mingw
+        elif [ "${TRAVIS_OS_NAME}" = "osx" ]; then
             build_openssl_osx
         elif [ "${TRAVIS_OS_NAME}" = "linux" ]; then
             build_openssl_linux

.travis/build-deps.sh build_openssl () { · line 80

Quoted 7 lines
.travis/build-deps.sh +1 -1
@@ -61,7 +75,7 @@ build_openssl () {
 }
 
 # Enable ccache
-if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
+if [ "${TRAVIS_OS_NAME}" != "osx" ] && [ -z "${CHOST+xxx}" ]; then
     # ccache not available on osx, see:
     # https://github.com/travis-ci/travis-ci/issues/5567

Is ccache not available for mingw? In that case, please update the comment, preferably with a reference to a travis issue so that we can enable it if travis adds support later on.

Quoted 12 lines
.travis/build-deps.sh +0 -1
     mkdir -p "${HOME}/bin"

@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1

.travis/build-deps.sh else · line 105

Quoted 10 lines
.travis/build-deps.sh +9 -0
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu xenial main universe" | sudo tee -a /etc/apt/sources.list.d/xenial.list
+      echo "deb http://archive.ubuntu.com/ubuntu xenial main" | sudo tee -a /etc/apt/sources.list.d/xenial.list
+      sudo apt-get update
+      sudo apt-get -y install dpkg mingw-w64
+      if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip" ]; then
+         wget -P download-cache/ http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
+      fi

This should be in a download_tap_windows() function.

.travis/build-deps.sh else · line 110

Quoted 5 lines
.travis/build-deps.sh +5 -0
+      unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip
+
+      (
+      wget -P download-cache/ http://www.oberhumer.com/opensource/lzo/download/lzo-${LZO_VERSION}.tar.gz
+      tar zxf download-cache/lzo-${LZO_VERSION}.tar.gz

This should be in a download_lzo() function.

.travis/build-deps.sh else · line 115

Quoted 5 lines
.travis/build-deps.sh +5 -0
+      cd lzo-${LZO_VERSION}
+
+      ./configure --host=${CHOST} --program-prefix='' --enable-shared --libdir=${PREFIX}/lib --prefix=${PREFIX} --build=x86_64-pc-linux-gnu
+      make
+      make install

This should bin in a build_lzo() function.

.travis/build-deps.sh else · line 119

Quoted 4 lines
.travis/build-deps.sh +4 -0
+      )
+
+      (
+      wget -P download-cache/ http://downloads.sourceforge.net/project/opensc/pkcs11-helper/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2

This should be in a download_pkcs11helper() function.

.travis/build-deps.sh else · line 125

Quoted 6 lines
.travis/build-deps.sh +6 -0
+      tar jxf download-cache/pkcs11-helper-${PKCS11_HELPER_VERSION}.tar.bz2
+      cd pkcs11-helper-${PKCS11_HELPER_VERSION}
+
+      ./configure --host=${CHOST} --program-prefix='' --enable-shared --libdir=${PREFIX}/lib --prefix=${PREFIX} --build=x86_64-pc-linux-gnu --disable-crypto-engine-gnutls --disable-crypto-engine-nss
+      make
+      make install

This should be in a build_pkcs11helper() function.

.travis/build-deps.sh else · line 128

Quoted 4 lines
.travis/build-deps.sh +3 -0
+      )
+
+fi

-Steffan

author
#4 lore raw

2017-04-09 13:44 GMT+05:00 Steffan Karger <[email protected]...>:

Quoted 97 lines

Hi,

Quoted 1 line · Ilya Shipitsin

Inspired by https://www.mail-archive.com/[email protected]...

sourceforge.net/msg13032.html

Quoted 1 line

build options are taken from regular windows installer builds

Feature-ACK. Adding cross-compile builds for Windows on travis is a good plan.

Initial feedback below.

Quoted 5 lines
Signed-off-by Ilya Shipitsin <[email protected]...>
---
 .travis.yml           | 30 +++++++++++++++++++++-------
 .travis/build-deps.sh | 55 ++++++++++++++++++++++++++++++

++++++++++++++++-----

Quoted 38 lines
 2 files changed, 73 insertions(+), 12 deletions(-)
.travis.yml +10 -0
index 3c0aa7d..9ab30a2 100644
@@ -12,6 +12,9 @@ env:
   global:
     - JOBS=3
     - PREFIX="${HOME}/opt"
+    - TAP_WINDOWS_VERSION=9.21.2
+    - LZO_VERSION=2.10
+    - PKCS11_HELPER_VERSION=1.11
     - MBEDTLS_VERSION="2.4.0"
     - MBEDTLS_CFLAGS="-I${PREFIX}/include"
     - MBEDTLS_LIBS="-L${PREFIX}/lib -lmbedtls -lmbedx509 -lmbedcrypto"

@@ -50,6 +53,12 @@ matrix:
       os: osx
       osx_image: xcode7.3
       compiler: clang
+    - env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32
+      os: linux
+      compiler: ": Win64 build only"
+    - env: SSLLIB="openssl" CHOST=i686-w64-mingw32
+      os: linux
+      compiler: ": Win32 build only"
   exclude:
     - compiler: gcc
 

@@ -60,6 +69,7 @@ addons:
       - libpam0g-dev
       - liblz4-dev
       - linux-libc-dev
+      - man2html
 
 cache:
   ccache: true

.travis.yml before_install: · line 85

Quoted 4 lines
.travis.yml +1 -0
@@ -72,16 +82,22 @@ before_install:
   - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then brew install lzo; fi
 
 install:
+  - if [ ! -z "${CHOST+xxx}" ]; then unset CC; unset CXX; fi

What is the function of this +xxx ? It seems to me that all you need is "if [ ! -z "${CHOST}" ]".

.travis.yml before_install: · line 86

Quoted 1 line
.travis.yml +0 -0
   - .travis/build-deps.sh > build-deps.log 2>&1 || (cat build-deps.log

&& exit 1)

.travis.yml before_install: · line 88

Quoted 3 lines
.travis.yml +0 -0
 script:
   - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then export

LD_LIBRARY_PATH="${PREFIX}/lib:${LD_LIBRARY_PATH}"; fi

.travis.yml before_install: · line 89

Quoted 1 line
.travis.yml +0 -0
   - if [ "${TRAVIS_OS_NAME}" = "osx"   ]; then export

DYLD_LIBRARY_PATH="${PREFIX}/lib:${DYLD_LIBRARY_PATH}"; fi

.travis.yml before_install: · line 80

Quoted 2 lines
.travis.yml +0 -1
   - autoreconf -vi
-  - ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} ||

(cat config.log && exit 1)

.travis.yml before_install:

Quoted 3 lines
.travis.yml +0 -3
-  - make -j$JOBS
-  - src/openvpn/openvpn --version || true
-  - if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd src/openvpn/openvpn;

fi

.travis.yml before_install: · line 84

Quoted 1 line
.travis.yml +0 -1
-  - if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L

src/openvpn/openvpn; fi

.travis.yml before_install:

Quoted 4 lines
.travis.yml +2 -2
-  - make check
-  - $EXTRA_SCRIPT
+  - if [ -z "${CHOST+xxx}" ]; then
+      ./configure --with-crypto-library="${SSLLIB}" ${EXTRA_CONFIG} ||

(cat config.log && exit 1);

.travis.yml before_install:

Quoted 3 lines
.travis.yml +3 -0
+      make -j$JOBS;
+      src/openvpn/openvpn --version || true;
+      if [ "${TRAVIS_OS_NAME}" = "linux" ]; then ldd

src/openvpn/openvpn; fi;

.travis.yml before_install: · line 96

Quoted 1 line
.travis.yml +1 -0
+      if [ "${TRAVIS_OS_NAME}" = "osx" ]; then otool -L

src/openvpn/openvpn; fi;

.travis.yml before_install:

Quoted 4 lines
.travis.yml +4 -0
+      make check;
+      $EXTRA_SCRIPT;
+    else
+      TAP_CFLAGS="-I${PWD}/tap-windows-${TAP_WINDOWS_VERSION}/include"
Quoted 8 lines

LZO_CFLAGS="-I${PREFIX}/include" LZO_LIBS="-L${PREFIX}/lib -llzo2" PKCS11_HELPER_LIBS="-L${PREFIX}/lib -lpkcs11-helper" PKCS11_HELPER_CFLAGS="-I${PREFIX}/include" ./configure --host=${CHOST} --build=x86_64-pc-linux-gnu --enable-pkcs11 --disable-plugins || (cat config.log && exit 1);

All these _CFLAGS and _LIBS should go into the env: section above.

there are 3 "env" sections: global and 2 "mingw".
I'm afraid, defining those variables in global section might have some side effect, while specifying variables 2 times in both mingw sections also do not make much sense.

Quoted 4 lines

Any reason to --enable-pkcs11 for the windows builds, while that's not done for the non-windows builds?

regular windows installer is built with pkcs11. that's why I added that option.
build matrix for non-windows builds was discussed and nobody asked me to add pkcs11 there.

Quoted 35 lines
Quoted 24 lines
.travis.yml +2 -0
+      make -j$JOBS;
+    fi
.travis/build-deps.sh +0 -2
index 3ffba0b..18f40ec 100755
@@ -31,7 +31,6 @@ download_openssl () {
 }
 
 build_openssl_linux () {
-    tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
     (
         cd "openssl-${OPENSSL_VERSION}/"
         ./config shared --openssldir="${PREFIX}" -DPURIFY

@@ -40,7 +39,6 @@ build_openssl_linux () {
 }
 
 build_openssl_osx () {
-    tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
     (
         cd "openssl-${OPENSSL_VERSION}/"
         ./Configure darwin64-x86_64-cc shared \

.travis/build-deps.sh build_openssl_osx () { · line 54

Quoted 8 lines
.travis/build-deps.sh +5 -0
@@ -49,9 +47,25 @@ build_openssl_osx () {
     )
 }
 
+build_openssl_mingw () {
+    (
+        cd "openssl-${OPENSSL_VERSION}/"
+
+        if [ "${CHOST}" = "i686-w64-mingw32" ]; then export

target=mingw; fi

.travis/build-deps.sh build_openssl_osx () { · line 55

Quoted 1 line
.travis/build-deps.sh +1 -0
+        if [ "${CHOST}" = "x86_64-w64-mingw32" ]; then export

target=mingw64; fi

Quoted 9 lines

.travis/build-deps.sh build_openssl_osx () {

Quoted 3 lines
.travis/build-deps.sh +3 -0
+
+        ./Configure --cross-compile-prefix=${CHOST}- shared \
+           $target no-multilib no-capieng --openssldir="${PREFIX}"

-static-libgcc

Can you try to keep style consistent? I.e. use all-caps variable names (TARGET instead of target) and use quoting ("${TARGET}" instead of $target).

yes, I can

Quoted 19 lines

.travis/build-deps.sh build_openssl_osx () {

Quoted 7 lines
.travis/build-deps.sh +4 -0
+        make install
+    )
+}
+
 build_openssl () {
     if [ "$(cat ${PREFIX}/.openssl-version)" != "${OPENSSL_VERSION}" ];

then

Quoted 11 lines
.travis/build-deps.sh +4 -1
-        if [ "${TRAVIS_OS_NAME}" = "osx" ]; then
+        tar zxf "download-cache/openssl-${OPENSSL_VERSION}.tar.gz"
+        if [ ! -z ${CHOST+xxx} ]; then
+            build_openssl_mingw
+        elif [ "${TRAVIS_OS_NAME}" = "osx" ]; then
             build_openssl_osx
         elif [ "${TRAVIS_OS_NAME}" = "linux" ]; then
             build_openssl_linux

@@ -61,7 +75,7 @@ build_openssl () {
 }
Quoted 10 lines

.travis/build-deps.sh build_openssl () { · line 80

Quoted 5 lines
.travis/build-deps.sh +1 -1
 # Enable ccache
-if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
+if [ "${TRAVIS_OS_NAME}" != "osx" ] && [ -z "${CHOST+xxx}" ]; then
     # ccache not available on osx, see:
     # https://github.com/travis-ci/travis-ci/issues/5567

Is ccache not available for mingw? In that case, please update the comment, preferably with a reference to a travis issue so that we can enable it if travis adds support later on.

I could not get ccache working with mingw, but there's no issue, I'm not sure it is related to travis either.

Quoted 23 lines
Quoted 11 lines
.travis/build-deps.sh +0 -1
     mkdir -p "${HOME}/bin"

@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

.travis/build-deps.sh else · line 99

Quoted 6 lines
.travis/build-deps.sh +3 -0
@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu xenial main universe"

| sudo tee -a /etc/apt/sources.list.d/xenial.list

.travis/build-deps.sh else · line 100

Quoted 1 line
.travis/build-deps.sh +1 -0
+      echo "deb http://archive.ubuntu.com/ubuntu xenial main" | sudo

tee -a /etc/apt/sources.list.d/xenial.list

.travis/build-deps.sh else

Quoted 3 lines
.travis/build-deps.sh +3 -0
+      sudo apt-get update
+      sudo apt-get -y install dpkg mingw-w64
+      if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip"
Quoted 7 lines

]; then

.travis/build-deps.sh else · line 104

Quoted 1 line
.travis/build-deps.sh +1 -0
+         wget -P download-cache/ http://build.openvpn.net/

downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip

.travis/build-deps.sh else · line 105

Quoted 1 line
.travis/build-deps.sh +1 -0
+      fi

This should be in a download_tap_windows() function.

can you be more particular on "should be".
it does not mean "must be".
if so, we can leave it, right ?

Quoted 56 lines

.travis/build-deps.sh else

Quoted 5 lines
.travis/build-deps.sh +4 -0
+      unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip
+
+      (
+      wget -P download-cache/ http://www.oberhumer.com/

opensource/lzo/download/lzo-${LZO_VERSION}.tar.gz

.travis/build-deps.sh else · line 110

Quoted 1 line
.travis/build-deps.sh +1 -0
+      tar zxf download-cache/lzo-${LZO_VERSION}.tar.gz

This should be in a download_lzo() function.

.travis/build-deps.sh else

Quoted 3 lines
.travis/build-deps.sh +3 -0
+      cd lzo-${LZO_VERSION}
+
+      ./configure --host=${CHOST} --program-prefix='' --enable-shared

--libdir=${PREFIX}/lib --prefix=${PREFIX} --build=x86_64-pc-linux-gnu

.travis/build-deps.sh else · line 115

Quoted 2 lines
.travis/build-deps.sh +2 -0
+      make
+      make install

This should bin in a build_lzo() function.

.travis/build-deps.sh else

Quoted 4 lines
.travis/build-deps.sh +4 -0
+      )
+
+      (
+      wget -P download-cache/ http://downloads.sourceforge.

net/project/opensc/pkcs11-helper/pkcs11-helper-${PKCS11_ HELPER_VERSION}.tar.bz2

This should be in a download_pkcs11helper() function.

.travis/build-deps.sh else · line 120

Quoted 1 line
.travis/build-deps.sh +1 -0
+      tar jxf download-cache/pkcs11-helper-$

{PKCS11_HELPER_VERSION}.tar.bz2

.travis/build-deps.sh else

Quoted 3 lines
.travis/build-deps.sh +3 -0
+      cd pkcs11-helper-${PKCS11_HELPER_VERSION}
+
+      ./configure --host=${CHOST} --program-prefix='' --enable-shared

--libdir=${PREFIX}/lib --prefix=${PREFIX} --build=x86_64-pc-linux-gnu --disable-crypto-engine-gnutls --disable-crypto-engine-nss

.travis/build-deps.sh else · line 125

Quoted 2 lines
.travis/build-deps.sh +2 -0
+      make
+      make install

This should be in a build_pkcs11helper() function.

.travis/build-deps.sh else · line 128

Quoted 4 lines
.travis/build-deps.sh +3 -0
+      )
+
+fi

-Steffan

------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

#5 lore raw

Hi,

Quoted 15 lines · Илья Шипицин

2017-04-09 13:44 GMT+05:00 Steffan Karger <[email protected]... <mailto:[email protected]...>>:

    On 26-03-17 13:21, Ilya Shipitsin wrote:
    > +      TAP_CFLAGS="-I${PWD}/tap-windows-${TAP_WINDOWS_VERSION}/include"
    LZO_CFLAGS="-I${PREFIX}/include" LZO_LIBS="-L${PREFIX}/lib -llzo2"
    PKCS11_HELPER_LIBS="-L${PREFIX}/lib -lpkcs11-helper"
    PKCS11_HELPER_CFLAGS="-I${PREFIX}/include" ./configure
    --host=${CHOST} --build=x86_64-pc-linux-gnu --enable-pkcs11
    --disable-plugins || (cat config.log && exit 1);

    All these _CFLAGS and _LIBS should go into the env: section above.
Quoted 4 lines

there are 3 "env" sections: global and 2 "mingw".
I'm afraid, defining those variables in global section might have some side effect, while specifying variables 2 times in both mingw sections also do not make much sense.

Good point, my suggestion will not work. I'm still not quite happy with this very long line though. Maybe put them in something like .travis/win_build_vars and source that file from .travis.yml ? Other suggestions are welcome too.

Quoted 6 lines

Any reason to --enable-pkcs11 for the windows builds, while that's not done for the non-windows builds?

regular windows installer is built with pkcs11. that's why I added that option.

Ok. Makes sense to verify that at least our own Windows builds work.

Quoted 2 lines

build matrix for non-windows builds was discussed and nobody asked me to add pkcs11 there.

I just wondered why the difference, didn't mean to suggest that we need to add pkcs11 to the other builds.

Quoted 7 lines

.travis/build-deps.sh build_openssl () { · line 80

Quoted 5 lines

# Enable ccache

.travis/build-deps.sh +1 -1
-if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
+if [ "${TRAVIS_OS_NAME}" != "osx" ] && [ -z "${CHOST+xxx}" ]; then
     # ccache not available on osx, see:
     # https://github.com/travis-ci/travis-ci/issues/5567

<https://github.com/travis-ci/travis-ci/issues/5567>

Quoted 7 lines

Is ccache not available for mingw? In that case, please update the comment, preferably with a reference to a travis issue so that we can enable it if travis adds support later on.

I could not get ccache working with mingw, but there's no issue, I'm not sure it is related to travis either.

Ok. Then just make sure to update the comment so it becomes clear that && [ -z "${CHOST+xxx}" ] disables ccache for windows cross compiles.

Quoted 25 lines
Quoted 9 lines
.travis/build-deps.sh +0 -1
@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

.travis/build-deps.sh else · line 99

Quoted 6 lines
.travis/build-deps.sh +3 -0
@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>

xenial main universe" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> + echo "deb http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>
xenial main" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> + sudo apt-get update
> + sudo apt-get -y install dpkg mingw-w64
> + if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip" ]; then
> + wget -P download-cache/ http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>
> + fi

Quoted 6 lines

This should be in a download_tap_windows() function.

can you be more particular on "should be".
it does not mean "must be".
if so, we can leave it, right ?

I'll be more clear (and less polite..): writing long lists of commands is bad. That's not only true for code, it goes for scripts as well. Writing functions (with good names!) allows the reader to not have to parse the commands to understand what the author wanted to do.

If we put this in functions, the pkcs11 section will read

download_pkcs11helper
build_pkcs11helper

Which makes it immediately clear what the intent of that code is.

For tap-windows we only have to download and extract, to that would only read

download_tap_windows

which makes this immediately clear too.

This really is basic software engineering, and the lack of quality in quite some of your contributions is an important reason why your contributions aren't picked up quickly. Your ideas and intentions are good, but we need to put in far too much effort to make the quality of your contribution acceptable. Time that we can also spend on better-quality contributions. It would help us all (including you) if you would try to improve on that.

-Steffan

author
#6 lore raw

2017-04-09 17:03 GMT+05:00 Steffan Karger <[email protected]...>:

Quoted 120 lines

Hi,

Quoted 6 lines · Илья Шипицин

2017-04-09 13:44 GMT+05:00 Steffan Karger <[email protected]... <mailto:[email protected]...>>:

    On 26-03-17 13:21, Ilya Shipitsin wrote:
    > +      TAP_CFLAGS="-I${PWD}/tap-windows-${TAP_WINDOWS_VERSION}

/include"

Quoted 9 lines
    LZO_CFLAGS="-I${PREFIX}/include" LZO_LIBS="-L${PREFIX}/lib -llzo2"
    PKCS11_HELPER_LIBS="-L${PREFIX}/lib -lpkcs11-helper"
    PKCS11_HELPER_CFLAGS="-I${PREFIX}/include" ./configure
    --host=${CHOST} --build=x86_64-pc-linux-gnu --enable-pkcs11
    --disable-plugins || (cat config.log && exit 1);

    All these _CFLAGS and _LIBS should go into the env: section above.
Quoted 4 lines

there are 3 "env" sections: global and 2 "mingw".
I'm afraid, defining those variables in global section might have some side effect, while specifying variables 2 times in both mingw sections also do not make much sense.

Good point, my suggestion will not work. I'm still not quite happy with this very long line though. Maybe put them in something like .travis/win_build_vars and source that file from .travis.yml ? Other suggestions are welcome too.

Quoted 1 line

Any reason to --enable-pkcs11 for the windows builds, while that's

not

Quoted 5 lines

done for the non-windows builds?

regular windows installer is built with pkcs11. that's why I added that option.

Ok. Makes sense to verify that at least our own Windows builds work.

Quoted 2 lines

build matrix for non-windows builds was discussed and nobody asked me to add pkcs11 there.

I just wondered why the difference, didn't mean to suggest that we need to add pkcs11 to the other builds.

Quoted 7 lines

.travis/build-deps.sh build_openssl () { · line 80

Quoted 5 lines

# Enable ccache

.travis/build-deps.sh +1 -1
-if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
+if [ "${TRAVIS_OS_NAME}" != "osx" ] && [ -z "${CHOST+xxx}" ]; then
     # ccache not available on osx, see:
     # https://github.com/travis-ci/travis-ci/issues/5567

<https://github.com/travis-ci/travis-ci/issues/5567>

Quoted 7 lines

Is ccache not available for mingw? In that case, please update the comment, preferably with a reference to a travis issue so that we can enable it if travis adds support later on.

I could not get ccache working with mingw, but there's no issue, I'm not sure it is related to travis either.

Ok. Then just make sure to update the comment so it becomes clear that && [ -z "${CHOST+xxx}" ] disables ccache for windows cross compiles.

Quoted 15 lines
Quoted 9 lines
.travis/build-deps.sh +0 -1
@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

.travis/build-deps.sh else · line 99

Quoted 6 lines
.travis/build-deps.sh +3 -0
@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu <

http://archive.ubuntu.com/ubuntu>

Quoted 1 line

xenial main universe" | sudo tee -a /etc/apt/sources.list.d/

xenial.list

Quoted 1 line

Review else · line 100

Quoted 1 line
(quoted diff) +1 -0
+      echo "deb http://archive.ubuntu.com/ubuntu <

http://archive.ubuntu.com/ubuntu>

Quoted 4 lines

xenial main" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> + sudo apt-get update
> + sudo apt-get -y install dpkg mingw-w64
> + if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip"

]; then

Quoted 1 line

Review else · line 101

Quoted 1 line
(quoted diff) +1 -0
+         wget -P download-cache/ http://build.openvpn.net/

downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip

Quoted 1 line

<http://build.openvpn.net/downloads/releases/tap-

windows-${TAP_WINDOWS_VERSION}.zip>

Quoted 8 lines

Review else · line 102

Quoted 1 line
(quoted diff) +1 -0
+      fi

This should be in a download_tap_windows() function.

can you be more particular on "should be".
it does not mean "must be".
if so, we can leave it, right ?

I'll be more clear (and less polite..): writing long lists of commands is bad. That's not only true for code, it goes for scripts as well. Writing functions (with good names!) allows the reader to not have to parse the commands to understand what the author wanted to do.

If we put this in functions, the pkcs11 section will read

download_pkcs11helper
build_pkcs11helper

Which makes it immediately clear what the intent of that code is.

For tap-windows we only have to download and extract, to that would only read

download_tap_windows

which makes this immediately clear too.

Quoted 8 lines

This really is basic software engineering, and the lack of quality in quite some of your contributions is an important reason why your contributions aren't picked up quickly. Your ideas and intentions are good, but we need to put in far too much effort to make the quality of your contribution acceptable. Time that we can also spend on better-quality contributions. It would help us all (including you) if you would try to improve on that.

those code lines are not reusable, so not splitting it into functions does not mean poor quality (for me)
the rule of thumb is "to put into function piece of code, which is called many times"

otherwise, it might be either function or not.

how did you came to conclusion that it means poor code quality? is it written somewhere on the https://openvpn.net on how to write bash scripts ?

Quoted 3 lines

-Steffan

#7 lore raw

Hi,

Quoted 67 lines · Илья Шипицин
Quoted 15 lines
Quoted 9 lines
.travis/build-deps.sh +0 -1
@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

.travis/build-deps.sh else · line 99

Quoted 6 lines
.travis/build-deps.sh +3 -0
@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu

<http://archive.ubuntu.com/ubuntu> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>
> xenial main universe" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> > + echo "deb http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>
> xenial main" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> > + sudo apt-get update
> > + sudo apt-get -y install dpkg mingw-w64
> > + if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip" ]; then
> > + wget -P download-cache/ http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>
> <http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>>
> > + fi
>
> This should be in a download_tap_windows() function.
>
>
> can you be more particular on "should be".
> it does not mean "must be".
> if so, we can leave it, right ?

I'll be more clear (and less polite..): writing long lists of commands is bad. That's not only true for code, it goes for scripts as well. Writing functions (with good names!) allows the reader to not have to parse the commands to understand what the author wanted to do.

If we put this in functions, the pkcs11 section will read

download_pkcs11helper
build_pkcs11helper

Which makes it immediately clear what the intent of that code is.

For tap-windows we only have to download and extract, to that would only read

download_tap_windows

which makes this immediately clear too.

This really is basic software engineering, and the lack of quality in quite some of your contributions is an important reason why your contributions aren't picked up quickly. Your ideas and intentions are good, but we need to put in far too much effort to make the quality of your contribution acceptable. Time that we can also spend on better-quality contributions. It would help us all (including you) if you would try to improve on that.

Quoted 6 lines

those code lines are not reusable, so not splitting it into functions does not mean poor quality (for me)
the rule of thumb is "to put into function piece of code, which is called many times"

otherwise, it might be either function or not.

Functions are not only to prevent code duplication. They are also for structuring your code. Which is exactly what I'm suggesting here. Code does not only need to do it's job, it also needs to be maintainable.

Quoted 3 lines

how did you came to conclusion that it means poor code quality? is it written somewhere on the https://openvpn.net on how to write bash scripts ?

We're not here to give software engineering classes. Nor is the openvpn wiki meant to be a book on software engineering.

-Steffan

author
#8 lore raw

2017-04-09 21:58 GMT+05:00 Steffan Karger <[email protected]...>:

Quoted 92 lines

Hi,

Quoted 15 lines · Илья Шипицин
Quoted 15 lines
Quoted 9 lines
.travis/build-deps.sh +0 -1
@@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
 fi
 
 # Download and build crypto lib
-mkdir -p download-cache
 if [ "${SSLLIB}" = "openssl" ]; then
     download_openssl
     build_openssl

.travis/build-deps.sh else · line 99

Quoted 6 lines
.travis/build-deps.sh +3 -0
@@ -81,3 +94,35 @@ else
     echo "Invalid crypto lib: ${SSLLIB}"
     exit 1
 fi
+
+if [ ! -z ${CHOST+xxx} ]; then
+      echo "deb http://archive.ubuntu.com/ubuntu
Quoted 3 lines

<http://archive.ubuntu.com/ubuntu> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>
> xenial main universe" | sudo tee -a /etc/apt/sources.list.d/

xenial.list

Quoted 7 lines
Quoted 1 line

Review else · line 100

Quoted 1 line
(quoted diff) +1 -0
+      echo "deb http://archive.ubuntu.com/ubuntu

<http://archive.ubuntu.com/ubuntu> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>
> xenial main" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> > + sudo apt-get update
> > + sudo apt-get -y install dpkg mingw-w64
> > + if [ ! -f "download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip"

]; then

Quoted 1 line
Quoted 1 line

Review else · line 101

Quoted 1 line
(quoted diff) +1 -0
+         wget -P download-cache/ http://build.openvpn.net/

downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip

Quoted 1 line

<http://build.openvpn.net/downloads/releases/tap-

windows-${TAP_WINDOWS_VERSION}.zip>

Quoted 1 line
Quoted 1 line

<http://build.openvpn.net/downloads/releases/tap-

windows-${TAP_WINDOWS_VERSION}.zip

Quoted 1 line

<http://build.openvpn.net/downloads/releases/tap-

windows-${TAP_WINDOWS_VERSION}.zip>>

Quoted 10 lines
Quoted 8 lines

Review else · line 102

Quoted 1 line
(quoted diff) +1 -0
+      fi

This should be in a download_tap_windows() function.

can you be more particular on "should be".
it does not mean "must be".
if so, we can leave it, right ?

I'll be more clear (and less polite..): writing long lists of

commands

Quoted 4 lines

is bad. That's not only true for code, it goes for scripts as well. Writing functions (with good names!) allows the reader to not have to parse the commands to understand what the author wanted to do.

Quoted 8 lines

If we put this in functions, the pkcs11 section will read

download_pkcs11helper
build_pkcs11helper

Which makes it immediately clear what the intent of that code is.

For tap-windows we only have to download and extract, to that would

only

Quoted 9 lines

read

download_tap_windows

which makes this immediately clear too.

This really is basic software engineering, and the lack of quality in quite some of your contributions is an important reason why your contributions aren't picked up quickly. Your ideas and intentions

are

Quoted 1 line

good, but we need to put in far too much effort to make the quality

of

Quoted 2 lines

your contribution acceptable. Time that we can also spend on better-quality contributions. It would help us all (including you)

if

Quoted 10 lines

you would try to improve on that.

those code lines are not reusable, so not splitting it into functions does not mean poor quality (for me)
the rule of thumb is "to put into function piece of code, which is called many times"

otherwise, it might be either function or not.

Functions are not only to prevent code duplication. They are also for structuring your code. Which is exactly what I'm suggesting here. Code does not only need to do it's job, it also needs to be maintainable.

Quoted 7 lines
Quoted 3 lines

how did you came to conclusion that it means poor code quality? is it written somewhere on the https://openvpn.net on how to write bash scripts ?

We're not here to give software engineering classes. Nor is the openvpn wiki meant to be a book on software engineering.

well, I did not ask for software engineering classes.
what I still do not understand is your "ultimate" statement about "poor code quality".
bash allows either use or not use functions. openvpn does not propose any standards on that.

so, what I did - I wrote very small piece of code, which fits single screen, also it is
pretty self documented (wget --> tar --> make). I still think it is good enough.

yes, we can use functions if people on list would agree on that, but I think there were not enough reasons for calling that "poor code quality".

Quoted 3 lines

-Steffan

#9 lore raw

Hi,

[This will be my last on-list reply about this.]

Quoted 123 lines · Илья Шипицин

2017-04-09 21:58 GMT+05:00 Steffan Karger <[email protected]... <mailto:[email protected]...>>:

Hi,

On 09-04-17 15:31, Илья Шипицин wrote:
> > > @@ -70,7 +84,6 @@ if [ "${TRAVIS_OS_NAME}" != "osx" ]; then
> > > fi
> > >
> > > # Download and build crypto lib
> > > -mkdir -p download-cache
> > > if [ "${SSLLIB}" = "openssl" ]; then
> > > download_openssl
> > > build_openssl
> > > @@ -81,3 +94,35 @@ else
> > > echo "Invalid crypto lib: ${SSLLIB}"
> > > exit 1
> > > fi
> > > +
> > > +if [ ! -z ${CHOST+xxx} ]; then
> > > + echo "deb http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>
> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>
<http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>
> <http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>>>
> > xenial main universe" | sudo tee -a /etc/apt/sources.list.d/xenial.list
> > > + echo "deb http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>
> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>
<http://archive.ubuntu.com/ubuntu <http://archive.ubuntu.com/ubuntu>
> <http://archive.ubuntu.com/ubuntu
<http://archive.ubuntu.com/ubuntu>>>
> > xenial main" | sudo tee -a
/etc/apt/sources.list.d/xenial.list
> > > + sudo apt-get update
> > > + sudo apt-get -y install dpkg mingw-w64
> > > + if [ ! -f
"download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip" ]; then
> > > + wget -P download-cache/
http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>
>
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>>
> >
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>
>
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip
<http://build.openvpn.net/downloads/releases/tap-windows-${TAP_WINDOWS_VERSION}.zip>>>
> > > + fi
> >
> > This should be in a download_tap_windows() function.
> >
> >
> > can you be more particular on "should be".
> > it does not mean "must be".
> > if so, we can leave it, right ?
>
> I'll be more clear (and less polite..): writing long lists of
commands
> is bad. That's not only true for code, it goes for scripts as
well.
> Writing functions (with good names!) allows the reader to not
have to
> parse the commands to understand what the author wanted to do.
>
> If we put this in functions, the pkcs11 section will read
>
> download_pkcs11helper
> build_pkcs11helper
>
> Which makes it immediately clear what the intent of that code is.
>
> For tap-windows we only have to download and extract, to that
would only
> read
>
> download_tap_windows
>
> which makes this immediately clear too.
>
> This really is basic software engineering, and the lack of
quality in
> quite some of your contributions is an important reason why your
> contributions aren't picked up quickly. Your ideas and
intentions are
> good, but we need to put in far too much effort to make the
quality of
> your contribution acceptable. Time that we can also spend on
> better-quality contributions. It would help us all (including
you) if
> you would try to improve on that.
>
>
>
> those code lines are not reusable, so not splitting it into functions > does not mean poor quality (for me)
> the rule of thumb is "to put into function piece of code, which is > called many times"
>
> otherwise, it might be either function or not.

Functions are not only to prevent code duplication. They are also for structuring your code. Which is exactly what I'm suggesting here. Code does not only need to do it's job, it also needs to be maintainable.

> how did you came to conclusion that it means poor code quality? > is it written somewhere on the https://openvpn.net on how to write bash > scripts ?

We're not here to give software engineering classes. Nor is the openvpn wiki meant to be a book on software engineering.

well, I did not ask for software engineering classes.
what I still do not understand is your "ultimate" statement about "poor code quality".
bash allows either use or not use functions. openvpn does not propose any standards on that.

Quoted 7 lines

so, what I did - I wrote very small piece of code, which fits single screen, also it is
pretty self documented (wget --> tar --> make). I still think it is good enough.

yes, we can use functions if people on list would agree on that, but I think there were not enough reasons for calling that "poor code quality".

Just look at the rest of the file. There's a pattern: download_xyz() and build_xyz() functions that are called from a compact main function. How hard is it to follow the pattern?

The patch that added travis support also needed a lot of rework. Not just style-wise, but also because there were a number of bugs hiding behind the bad style. I invested in refactoring that patch instead of just writing my own, because I think your ideas and enthusiasm are welcome and hoped I you would learn from it.

This is just one example. If this was the only one I would not have been this hard on you. Just scroll back through the mailing list and read the discussions about your patches. You should notice a trend there.

Anyway, feel free to find someone else to review and ACK this code. I feel that an ACK is partly taking responsibility for the code in question, and I'm not taking responsibility for this.

-Steffan

#10 lore raw
In reply to Илья Шипицин
Quoted 4 lines · Илья Шипицин

how did you came to conclusion that it means poor code quality? is it written somewhere on the https://openvpn.net on how to write bash scripts ?

Steffan have given a good reasoning, and I completely agree with him.

Further, it is impossible to write a documentation which is 100% perfect on coding style, good coding practice and related subjects. If you insist on that, I recommend you to visit http://shop.oreilly.com/ or Amazon and find some good books about it (the review ratings on those sites are usually fairly good in this regard). So there are no reasons for us to try to duplicate the work of other writers.

I would even dare to claim that programming isn't just a science with only one way to do things properly. But it is a science which requires you to think thoroughly of what you try to solve, how to solve it and how to make it easy and understandable for others. In fact, the real science part of programming (to solve a real and concrete issue) is just one little part of the whole picture. And this makes me think of Reuven Lerner and his weekly newsletter. He pulls up a lot of these things there too, perhaps consider to sign up for that too can be a good idea?
<http://lerner.co.il/newsletter/>

Signature · 4 lines
kind regards,

David Sommerseth
OpenVPN Technologies, Inc
#11 lore raw
Quoted 7 lines · David Sommerseth
Quoted 4 lines · Илья Шипицин

how did you came to conclusion that it means poor code quality? is it written somewhere on the https://openvpn.net on how to write bash scripts ?

Steffan have given a good reasoning, and I completely agree with him.

Searching for "why split code into functions?" in DuckDuckGo gave this as the first hit:

<http://stackoverflow.com/questions/3107400/what-is-the-golden-rule-for-when-to-split-code-up-into-functions#3107416>

Two good reasons from that page:

1) Code reuse (=avoid repeating yourself)
2) Readability

Your patch adds a big chunk of code under

   if [ ! -z ${CHOST+xxx} ]; then

It is pretty clear _what_ the lines in the code block are doing, but it is not at all obvious _why_ the code block is there. One has to search it for hints like

# Must have something to do with mingw-w64...
sudo apt-get -y install dpkg mingw-w64

and

   # This must be building for Windows...
   unzip download-cache/tap-windows-${TAP_WINDOWS_VERSION}.zip

One could also search for the commit that added that block, and notice the commit message: "travis-ci: add 2 mingw "build only configurations"".

Nobody should have to spend more than a few seconds trying to figure out why the code block is there. A simple comment before the block would have improved readability _a lot_, e.g.

   # Build on mingw-w64
   if [ ! -z ${CHOST+xxx} ]; then

That said, please do what Steffan suggested and split the block into human-readable logical functions and call those functions as needed. Then we can review the patch again and hopefully merge it as-is.

Signature · 5 lines
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock
Quoted 17 lines

Further, it is impossible to write a documentation which is 100% perfect on coding style, good coding practice and related subjects. If you insist on that, I recommend you to visit http://shop.oreilly.com/ or Amazon and find some good books about it (the review ratings on those sites are usually fairly good in this regard). So there are no reasons for us to try to duplicate the work of other writers.

I would even dare to claim that programming isn't just a science with only one way to do things properly. But it is a science which requires you to think thoroughly of what you try to solve, how to solve it and how to make it easy and understandable for others. In fact, the real science part of programming (to solve a real and concrete issue) is just one little part of the whole picture. And this makes me think of Reuven Lerner and his weekly newsletter. He pulls up a lot of these things there too, perhaps consider to sign up for that too can be a good idea?
<http://lerner.co.il/newsletter/>