Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

@Lekensteyn
Copy link
Member

@Lekensteyn Lekensteyn commented May 10, 2020

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 Lekensteyn force-pushed the Lekensteyn:travis-cleanup branch from 7457d79 to 04a4467 May 10, 2020
@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 10, 2020

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);
                               ^~~~~~~~~~~~~~~~~
@Lekensteyn Lekensteyn force-pushed the Lekensteyn:travis-cleanup branch 2 times, most recently from 289d98e to f31ca1b May 10, 2020
lib/vssh/libssh.c Outdated Show resolved Hide resolved
Copy link
Member

@mback2k mback2k left a comment

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.

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

This comment has been minimized.

@mback2k

mback2k May 12, 2020
Member

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

This comment has been minimized.

@Lekensteyn

Lekensteyn May 12, 2020
Author Member

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
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 12, 2020

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
Copy link
Member

@MarcelRaad MarcelRaad commented May 12, 2020

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 Lekensteyn force-pushed the Lekensteyn:travis-cleanup branch from f31ca1b to 26187bb May 12, 2020
@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 12, 2020

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.

.travis.yml Outdated
config:
retries: true
sources: &common_sources
- ubuntu-toolchain-r-test

This comment has been minimized.

@MarcelRaad

MarcelRaad May 12, 2020
Member

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?

Copy link
Member

@MarcelRaad MarcelRaad left a comment

👍

@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented May 13, 2020

@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 13, 2020

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 Lekensteyn force-pushed the Lekensteyn:travis-cleanup branch from 26187bb to 734c4cb May 14, 2020
@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 14, 2020

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 Lekensteyn force-pushed the Lekensteyn:travis-cleanup branch from 734c4cb to 30c6192 May 14, 2020
@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 14, 2020

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
Copy link
Collaborator

@dfandrich dfandrich commented May 14, 2020

@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 14, 2020

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 Outdated Show resolved Hide resolved
Lekensteyn added 2 commits May 12, 2020
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 Lekensteyn force-pushed the Lekensteyn:travis-cleanup branch from 30c6192 to 7534bb4 May 15, 2020
@Lekensteyn
Copy link
Member Author

@Lekensteyn Lekensteyn commented May 15, 2020

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
Copy link
Member

@MarcelRaad MarcelRaad commented May 15, 2020

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
Copy link
Member

@bagder bagder commented May 15, 2020

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
Copy link
Member

@MarcelRaad MarcelRaad commented May 16, 2020

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.

Lekensteyn added a commit that referenced this pull request May 16, 2020
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 Lekensteyn deleted the Lekensteyn:travis-cleanup branch May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.