travis: upgrade to bionic, clang-9, improve readability #5370

Lekensteyn · 2020-05-10T10:20:48Z

A major cleanup of the travis job matrix. Before doing so, a libssh fix was needed to fix the Ubuntu 18.04 build. As libssh was missing in CMake, I had to add it in order to test it locally.

Changes:

Upgrade Ubuntu and drop unnecessary third-party repos.
Properly clone apt config to ensure retries.
Upgrade to clang-9.

Non-functional cleanups:

Simplify test matrix, drop redundant os and compiler keys.
Deprecation fixes: remove sudo, rename matrix -> jobs.
Every job has an 'env' key, put this key first in a list item.

Use consistent indentation, automatically done with:

python3 -c 'from ruamel.yaml import YAML;y=YAML();d=y.load(open(".travis.yml"));y.dump(d,open(".travis.yml.new","w"))'
# Result edited to avoid reflowing very long lines.

Lekensteyn · 2020-05-10T14:41:27Z

This change was prompted by flaky tests due to failure of apt to download from the PPA. Removal of external dependencies should make CI more reliable.

The new Ubuntu version also uncovered a build failure with libssh (not libssh2), a workaround is prepended as well as the CMake config I used to test it:

vssh/libssh.c:342:3: error: ‘ssh_get_publickey’ is deprecated [-Werror=deprecated-declarations]
   rc = ssh_get_publickey(sshc->ssh_session, &pubkey);
   ^~
In file included from vssh/libssh.c:32:
/usr/include/libssh/libssh.h:489:31: note: declared here
 SSH_DEPRECATED LIBSSH_API int ssh_get_publickey(ssh_session session, ssh_key *key);
                               ^~~~~~~~~~~~~~~~~

lib/vssh/libssh.c

mback2k

In general this is looking good to me. I would just recommend in the future to do separate commits for whitespace and logic changes, so that it is easier to follow them separately.

mback2k · 2020-05-12T17:26:44Z

.travis.yml

-              apt:
-                  sources:
-                      - *common_sources
-                      - llvm-toolchain-xenial-7


I am just wondering why all the llvm-toolchain-* packages are no longer needed?

Ubuntu 14.04 (Trusty) shipped with clang 3.4 by default,
Ubuntu 16.04 (Xenial, Travis CI default) included clang 3.8 by default (with clang-6 as option), and
Ubuntu 18.04 (Bionic) included clang 6 by default with clang-9 as option.
In order to use a more recent version of Clang with better diagnostics, the old config therefore used the LLVM apt repo.

Clang 10 is the latest stable version upstream and an option in Ubuntu 20.04, but is not an option in Travis CI.

Lekensteyn · 2020-05-12T20:07:59Z

Regarding the whitespace changes, sorry for making it harder to review. Since I have to rebase this anyway, do you prefer me to make a new whitespace-only change before the other functional changes?

MarcelRaad · 2020-05-12T20:12:41Z

Yes please, that would be nice! I actually wanted to review this too, but haven't yet because it was hard because of the whitespace changes.

Lekensteyn · 2020-05-12T21:03:20Z

Rebased and separated the whitespace-only change, it should hopefully be easier to follow now. Some review guidance:

Track the new list item, the old jobs all started with - os: linux, the new ones with - env:.
The before_install commands for the boringssl/quiche builds were moved after the env, and simplified into one line.
The compiler: clang and arch: arm64 lines have also been moved after the env key.

Once again, apologies for making it harder to review before. Please let me know if I can make it even easier for review.

MarcelRaad · 2020-05-12T21:49:40Z

.travis.yml

    config:
      retries: true
-    sources: &common_sources
-    - ubuntu-toolchain-r-test


If we want to update to GCC 9, we need to add this again. So I'd maybe rather update to GCC 9 right now instead of removing this and adding it again later?

MarcelRaad

👍

dfandrich · 2020-05-13T06:17:02Z

Clang 10 is the latest stable version upstream and an option in Ubuntu 20.04, but is not an option in Travis CI.

Clang 10 is available in Travis, using: dist: focal compiler: clang addons: apt: sources: sourceline: - deb http://apt.llvm.org/focal/ llvm-toolchain-focal-10 main key_url: - https://apt.llvm.org/llvm-snapshot.gpg.key packages: - clang-10

Lekensteyn · 2020-05-13T10:40:05Z

If we want to update to GCC 9, we need to add this again. So I'd maybe rather update to GCC 9 right now instead of removing this and adding it again later?

gcc-8 seems recent enough, removing the external PPA should reduce the number CI failures due to network issues. If a newer GCC version is desired (the current stable is 10), it could be added at the top-level.

Clang 10 is the latest stable version upstream and an option in Ubuntu 20.04,
but is not an option in Travis CI.

Clang 10 is available in Travis, using:

Ah ok, what I meant it that it is not available in the standard repos. The same reason as for GCC applies, I want to reduce build failures due to external effects.

If you prefer to have the latest and greatest compiler for both, I could add another change on top of it. Hopefully with the addons.apt.config.retries: true setting being applied everywhere, the risk of failures should already be reduced.

Lekensteyn · 2020-05-14T10:45:27Z

Dropped the libssh patches and try Ubuntu 20.04 focal instead:

--- a/.travis.yml
+++ b/.travis.yml
@@ -64,6 +64,8 @@ jobs:
   - env:
     - T=normal C=--with-libssh
     - OVERRIDE_CC="CC=gcc-8" OVERRIDE_CXX="CXX=g++-8"
+    # Avoid bionic, its pre-release libssh version triggers deprecation warnings.
+    dist: focal
     addons:
       apt:
         <<: *common_apt

The commit message has also been updated, I am not using focal everywhere yet because Travis CI does not document this as option (it is work in progress and probably in very alpha state). I have no idea whether this build will even succeed.

If focal becomes available, then compilers can be upgraded to gcc-10 and clang-10.

Additionally, I pushed a patch on top of it that simplifies the quiche build instructions (from #5359).

Lekensteyn · 2020-05-14T12:19:47Z

The focal build failed due to python-impacket being removed, leaving python3-impacket (since 20.04) as only option. For simplicity I've just skipped installation of this now. When moving to focal for all future builds, we have to make sure that python is Python 3 or else python3-impacket will not work.

I also had to drop the quiche improvement, it uncovered a bug in configure.ac related to pthreads detection... argh. Will work on it later.

dfandrich · 2020-05-14T12:23:47Z

Or, switch from "python" to "python3" when invoking it.

Lekensteyn · 2020-05-14T18:59:40Z

Or, switch from "python" to "python3" when invoking it.

That is the long-term plan, but it will require installing impacket through pip or upgrading to focal since bionic does not have python3-impacket for SMB tests. python2 is EOL, so it would be reasonable to force python3 in a future change.

A few CI jobs failed due to test flakiness (or a setup failure in case of FreeBSD).

@dfandrich are you also happy with the current patch or would you like me to make further changes before merging?

.travis.yml

Automatically apply a consistent indentation with: python3 -c 'from ruamel.yaml import YAML;y=YAML();d=y.load(open(".travis.yml"));y.width=500;y.dump(d,open(".travis.yml.new","w"))' followed by manually re-indenting three comments.

Changes, partially to reduce build failures from external dependencies: - Upgrade Ubuntu and drop unnecessary third-party repos. - Properly clone apt config to ensure retries. - Upgrade to clang-9 from the standard repos. - Use Ubuntu 20.04 focal for the libssh build, use of ssh_get_publickey fails on -Werror=deprecated-declarations in Ubuntu 18.04. Do not use focal everywhere yet since Travis CI has not documented this option. In focal, python-impacket (Py2.7) has been removed, leaving only python3-impacket. Since it is only needed for SMB tests and not SSH, skip it for the libssh job since it might need more work. - apt: Remove gcc-8 and libstdc++-8-dev, already installed via g++-8. Non-functional cleanups: - Simplify test matrix, drop redundant os and compiler keys. - Deprecation fixes: remove sudo, rename matrix -> jobs. - Every job has an 'env' key, put this key first in a list item.

Lekensteyn · 2020-05-15T19:45:59Z

Removed CC/CXX override for focal and used YAML anchors to simplify future clang version upgrades. The GCC override can be changed in a similar way in the future when the version is bumped.

I like this diffstat (it can be cut down further if libpsl-dev/libbrotli-dev are moved to common_packages - is that a good idea?):

$ git show --oneline --stat HEAD^{/clang-9}
7534bb43c (origin/travis-cleanup) travis: upgrade to bionic, clang-9, improve readability
 .travis.yml | 322 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------------------------------------------------
 1 file changed, 101 insertions(+), 221 deletions(-)

diff since previous change

diff --git a/.travis.yml b/.travis.yml
index eb66a2c9d..6f7e67bfb 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -61,7 +61,6 @@ jobs:
         - libssh2-1-dev
   - env:
     - T=normal C=--with-libssh
-    - OVERRIDE_CC="CC=gcc-8" OVERRIDE_CXX="CXX=g++-8"
     # Avoid bionic, its pre-release libssh version triggers deprecation warnings.
     dist: focal
     addons:
@@ -162,64 +161,59 @@ jobs:
         - libbrotli-dev
   - env:
     - T=debug
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - &clang OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - &clang_packages [*common_packages, clang-9]
         - libpsl-dev
         - libbrotli-dev
   - env:
     - T=debug C="--enable-alt-svc"
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libpsl-dev
         - libbrotli-dev
   - env:
     - T=debug C="--with-mbedtls --without-ssl"
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libpsl-dev
         - libbrotli-dev
         - libmbedtls-dev
   - env:
     - T=debug C="--with-gnutls --without-ssl"
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libgnutls28-dev
         - libpsl-dev
         - libbrotli-dev
   - env:
     - T=debug C="--with-nss --without-ssl" NOTESTS=1 CPPFLAGS="-isystem /usr/include/nss"
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libnss3-dev
         - libpsl-dev
         - libbrotli-dev
@@ -241,15 +235,14 @@ jobs:
         - libbrotli-dev
   - env:
     - T=cmake NGTCP2=yes C="-DUSE_NGTCP2=ON"
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     - PKG_CONFIG_PATH="$HOME/ngbuild/lib/pkgconfig"
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libpsl-dev
         - libbrotli-dev
   - env:
@@ -276,51 +269,47 @@ jobs:
         - libbrotli-dev
   - env:
     - T=fuzzer
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libpsl-dev
         - libbrotli-dev
   - env:
     - T=tidy
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - clang-tidy-9
         - libpsl-dev
         - libbrotli-dev
   - env:
     - T=scan-build
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libpsl-dev
         - libbrotli-dev
   - env:
     - T=debug CFLAGS="-fsanitize=address,undefined,signed-integer-overflow -fno-sanitize-recover=undefined,integer -Wformat -Werror=format-security -Werror=array-bounds -g" LDFLAGS="-fsanitize=address,undefined -fno-sanitize-recover=undefined,integer" LIBS="-ldl -lubsan"
-    - OVERRIDE_CC="CC=clang-9" OVERRIDE_CXX="CXX=clang++-9"
+    - *clang
     compiler: clang
     addons:
       apt:
         <<: *common_apt
         packages:
-        - *common_packages
-        - clang-9
+        - *clang_packages
         - libpsl-dev
         - libbrotli-dev
   - env:

MarcelRaad · 2020-05-15T22:11:46Z

Looks good to me! 👍

Unfortunately, g++-8 (and gcc-8) are hard to get rid of as they're set inside before_script.sh for building nghttp2, otherwise they could also be removed on focal. focal ships nghttp2 1.40.0 though, so it doesn't even need to be built there. Probably something for another PR.

bagder · 2020-05-15T22:24:12Z

And we're not particularly married to 1.40.0 either... if a version is provide by the distro, I think that's a better and easier route!

MarcelRaad · 2020-05-16T05:43:04Z

In that case, we don't need to build it at all - Bionic has 1.30.0. Then we only need a C++ compiler for the BoringSSL build.

Changes, partially to reduce build failures from external dependencies: - Upgrade Ubuntu and drop unnecessary third-party repos. - Properly clone apt config to ensure retries. - Upgrade to clang-9 from the standard repos. - Use Ubuntu 20.04 focal for the libssh build, use of ssh_get_publickey fails on -Werror=deprecated-declarations in Ubuntu 18.04. Do not use focal everywhere yet since Travis CI has not documented this option. In focal, python-impacket (Py2.7) has been removed, leaving only python3-impacket. Since it is only needed for SMB tests and not SSH, skip it for the libssh job since it might need more work. - apt: Remove gcc-8 and libstdc++-8-dev, already installed via g++-8. Non-functional cleanups: - Simplify test matrix, drop redundant os and compiler keys. - Deprecation fixes: remove sudo, rename matrix -> jobs. - Every job has an 'env' key, put this key first in a list item. Closes #5370

Lekensteyn force-pushed the travis-cleanup branch from 7457d79 to 04a4467 Compare May 10, 2020 14:34

Lekensteyn force-pushed the travis-cleanup branch 2 times, most recently from 289d98e to f31ca1b Compare May 10, 2020 20:16

bagder reviewed May 10, 2020

View reviewed changes

lib/vssh/libssh.c Outdated Show resolved Hide resolved

This was referenced May 11, 2020

HTTP/3 (ngtcp2 and quiche) and alt-svc support in CMake #5359

Closed

Fix build with pre-release libssh in Ubuntu 18.04, add libssh to CMake #5372

Closed

bagder requested a review from mback2k May 12, 2020 06:49

mback2k approved these changes May 12, 2020

View reviewed changes

Lekensteyn force-pushed the travis-cleanup branch from f31ca1b to 26187bb Compare May 12, 2020 20:55

MarcelRaad reviewed May 12, 2020

View reviewed changes

MarcelRaad approved these changes May 12, 2020

View reviewed changes

Lekensteyn force-pushed the travis-cleanup branch from 26187bb to 734c4cb Compare May 14, 2020 10:38

Lekensteyn force-pushed the travis-cleanup branch from 734c4cb to 30c6192 Compare May 14, 2020 12:15

MarcelRaad reviewed May 15, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

Lekensteyn added 2 commits May 15, 2020 21:18

travis: whitespace-only changes for consistency

35ed00f

Automatically apply a consistent indentation with: python3 -c 'from ruamel.yaml import YAML;y=YAML();d=y.load(open(".travis.yml"));y.width=500;y.dump(d,open(".travis.yml.new","w"))' followed by manually re-indenting three comments.

Lekensteyn force-pushed the travis-cleanup branch from 30c6192 to 7534bb4 Compare May 15, 2020 19:35

Lekensteyn closed this in 1753e41 May 16, 2020

Lekensteyn deleted the travis-cleanup branch May 20, 2020 21:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

travis: upgrade to bionic, clang-9, improve readability #5370

travis: upgrade to bionic, clang-9, improve readability #5370

Lekensteyn commented May 10, 2020 •

edited

Lekensteyn commented May 10, 2020

mback2k left a comment

mback2k May 12, 2020

Lekensteyn May 12, 2020 •

edited

Lekensteyn commented May 12, 2020

MarcelRaad commented May 12, 2020

Lekensteyn commented May 12, 2020 •

edited

MarcelRaad May 12, 2020

MarcelRaad left a comment

dfandrich commented May 13, 2020 via email

Lekensteyn commented May 13, 2020

Lekensteyn commented May 14, 2020

Lekensteyn commented May 14, 2020

dfandrich commented May 14, 2020 via email

Lekensteyn commented May 14, 2020

Lekensteyn commented May 15, 2020

MarcelRaad commented May 15, 2020

bagder commented May 15, 2020

MarcelRaad commented May 16, 2020

travis: upgrade to bionic, clang-9, improve readability #5370

travis: upgrade to bionic, clang-9, improve readability #5370

Conversation

Lekensteyn commented May 10, 2020 • edited

Lekensteyn commented May 10, 2020

mback2k left a comment

Choose a reason for hiding this comment

mback2k May 12, 2020

Choose a reason for hiding this comment

Lekensteyn May 12, 2020 • edited

Choose a reason for hiding this comment

Lekensteyn commented May 12, 2020

MarcelRaad commented May 12, 2020

Lekensteyn commented May 12, 2020 • edited

MarcelRaad May 12, 2020

Choose a reason for hiding this comment

MarcelRaad left a comment

Choose a reason for hiding this comment

dfandrich commented May 13, 2020 via email

Lekensteyn commented May 13, 2020

Lekensteyn commented May 14, 2020

Lekensteyn commented May 14, 2020

dfandrich commented May 14, 2020 via email

Lekensteyn commented May 14, 2020

Lekensteyn commented May 15, 2020

MarcelRaad commented May 15, 2020

bagder commented May 15, 2020

MarcelRaad commented May 16, 2020

Lekensteyn commented May 10, 2020 •

edited

Lekensteyn May 12, 2020 •

edited

Lekensteyn commented May 12, 2020 •

edited