[Github-comments] [geany/geany] add gcc 7 and 8 build to travis tests (#2067)

[Colomban Wendling]

b4n requested changes on this pull request.

This sounds good, but I'm not so sure how important it is to check with recent compilers -- old ones for compat, new ones… well, it certainly doesn't hurt :)  However, we most probably don't need to run `distcheck` on all compilers, as it's mostly checking the build system itself rather than the compilation.  Only `all` (obviously) and `check` are really useful to run everywhere for a sensible test, and that might give a comfortable speed up by not building everything twice.  This can be changed at some later point tho, and can probably be fixed by e.g. setting `TARGETS="all check distcheck"` in the "normal" case and removing `distcheck` in the "compiler builds", and replacing the `make -j2 && make -j2 check && make -j2 distcheck` with just `make -j2 $TARGETS`.

Anyway, the change seems a little verbose, but there might not be a shorter syntax for this (I'm no YAML expert tho).

> +      dist: xenial
+      addons:
+        apt:
+          sources:
+            - ubuntu-toolchain-r-test
+          packages:
+            - g++-8
+      env:
+        - MATRIX_EVAL="CC=gcc-8 && CXX=g++-8"
+
+# clang        
+    - os: linux
+      addons:
+        apt:
+          sources:
+            - llvm-toolchain-trusty-5.0

shouldn't you specify `trusy` as the `dist` if you use that repo?

> +        apt:
+          sources:
+            - ubuntu-toolchain-r-test
+          packages:
+            - g++-8
+      env:
+        - MATRIX_EVAL="CC=gcc-8 && CXX=g++-8"
+
+# clang        
+    - os: linux
+      addons:
+        apt:
+          sources:
+            - llvm-toolchain-trusty-5.0
+          packages:
+            - clang-5.0

Ideally we'd test with clang 3.4 as it's supposed to be able to build Scintilla, which is our most bleeding edge.

>  before_install:
+  - eval "${MATRIX_EVAL}"

this should be in `before_script` below with the `CFLAGS` export, shouldn't it?

>  before_install:
+  - eval "${MATRIX_EVAL}"

Also I'd rather see this done using `export`, and set e.g. `MATRIX_EXPORT="CC=clang-5.0 CXX=clang++-5.0"`, which is effectively what we want done (and so it doesn't rely on `CC` and `CXX` being exported by something else).

> @@ -1,17 +1,70 @@
 # we use both C and C++, so advertize C++
 language: cpp
 cache: ccache
-dist: trusty
-compiler:
-  - gcc

If this is removed, which compiler is being used for the `env` part?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/2067#pullrequestreview-232169736
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20190501/dfd7cbce/attachment-0001.html>