This works but I'm not sure yet how much the code can be reduced by using a [build matrix](https://docs.travis-ci.com/user/customizing-the-build/#build-matrix). Comments welcome. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2067
-- Commit Summary --
* add gcc 7 and 8 build to travis test
-- File Changes --
M .travis.yml (24)
-- Patch Links --
https://github.com/geany/geany/pull/2067.patch https://github.com/geany/geany/pull/2067.diff
@andy5995 pushed 1 commit.
e4b21b460357a080623058ddde63468be5b9a62e add comment, shorten condition
@andy5995 pushed 1 commit.
d9587e9b715906f0ac47c7775646fa89529b3751 for gcc 8, specify xenial and the correct version
@andy5995 pushed 1 commit.
5204016d9f6cf1294a520288f5cb52c3a018fcab try globalizing the GTK3 env directives
@andy5995 pushed 1 commit.
496bc758a21e2ca5afe3cadc23975441ea343ce5 try indenting the matrix to get global to work
@andy5995 pushed 1 commit.
b0fdb0848905becd24b2e54779fe3d70c06c0055 revert a little
@andy5995 pushed 1 commit.
06bff1e39c825974a1f2570e4f95d7d312228a45 fix indentation
To record conversation from IRC, I don't think there is any need to matrix all the envs with the compiler types.
Travis for Geany is essentially a compile check (there are no real Geany tests to run, just a few ctags ones from upstream). So all we need is each of the env options to get compiled, and to look out for any compile issues with variations of compilers (and newly defaulted stupid warnings :)
Understood. All the checks have passed and this is ready for reviewed.
cc @b4n
Clang could also be added, but it does have a "-pthread option not used" warning on the link (on clang 6 which is my default here) which googling seems to imply is harmless. Googling also implies that libtool automatically adds this, so it might not be easy to fix.
Clang could also be added
May be worth a short discussion that if an osx (xcode) test were added, it would use clang. So the question would be, should the osx test be added? should it be added on a separate PR? And if so, would that render a clang test on Ubuntu as not needed?
cc @techee
I'm pretty sure if it's decided that adding osx is ultimately desired, it should be done in a separate PR, because most of the stuff in the 2nd half of .travis.yml would have to be put in conditional statements (if linux (run this) if osx (run this), and some brew lines would be needed to install packages if the test was for osx.
Well, OSX stuff is in a separate repository [geany/geany-osx](https://github.com/geany/geany-osx), so not sure if the code here compiles on OSX by itself.
@andy5995 pushed 1 commit.
93c7e5ef6e60da0de82aa6f4602005547aa84da1 add clang 5 test (closes #2139)
Well, OSX stuff is in a separate repository [geany/geany-osx](https://github.com/geany/geany-osx), so not sure if the code here compiles on OSX by itself.
Ok, so in travis.yml there'd need to be instructions to clone that repo or cache it.
Well, OSX stuff is in a separate repository geany/geany-osx, so not sure if the code here compiles on OSX by itself.
Not really, geany-osx contains just the bundling stuff but no code so it would make sense to have an osx test. I just think the osx builders tend to be overloaded on travis and take too much time to build. However, the command-line part of osx behaves like freebsd so if there's some BSD builder on travis, we could use that one.
No BSDs I'm afraid, maybe OSX should be left to another PR, lets see what @b4n says about this one for now.
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?
@b4n
Anyway, the change seems a little verbose, but there might not be a shorter syntax for this (I'm no YAML expert tho).
The syntax is copied from the Github help page link I posted on #2139 so blame it on Github. This applies to some of your comments above too, so unless you know better than Github maybe it should stay as is.
@andy5995 pushed 1 commit.
40d3f3dd5800df8bacf6745e3981ceb370b8a8cb only run `make distcheck` for a single job
andy5995 commented on this pull request.
@@ -1,17 +1,70 @@
# we use both C and C++, so advertize C++ language: cpp cache: ccache -dist: trusty -compiler: - - gcc
Looks like it defaults to gcc https://travis-ci.org/geany/geany/jobs/526317392#L442
andy5995 commented on this pull request.
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
replace 5.0 with 3.4, or add 3.4?
andy5995 commented on this pull request.
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
Travis uses trusty as the environment by default https://travis-ci.org/geany/geany/jobs/527122310#L9
andy5995 commented on this pull request.
before_install:
+ - eval "${MATRIX_EVAL}"
As @elextr noted, what I've used is suggested by the travis docs. It doesn't appear from the logs that CC and CXX are getting exported by anything else later. But you're concern is that it's a possibility? But definitely let me know if you're sure you'd like me to change that anyway.
elextr commented on this pull request.
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
Sadly Geany C89 is the trailing edge, although I think a few C99s have crept in under the radar :)
But Clang 3.4 isn't officially C++11 either.
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`.
I pushed a commit that addresses that. But I made the change before I reviewed your suggested approach. If you'd like it changed, please let me know.
Anyway, the change seems a little verbose, but there might not be a shorter syntax for this (I'm no YAML expert tho).
That's another copy/paste from the travis docs. afaik the only way to reduce the lines is to use a script that manually imports the toolchain repo and keys, and installs the appropriate gcc package.
However, using the the "addons/source" travis directive as it is now may be easier in the long-run, to maintain. What do you think?
@andy5995 pushed 1 commit.
ae64a5eaf85506cce7f8acc4344dd10a33c72402 specify dist and compiler explicity
@andy5995 pushed 1 commit.
5fd0f45d8d6d1495e2e6c06cbebdbc53ac595065 remove unneeded hyphenated lines
@andy5995 pushed 1 commit.
9752c4abc5bee069171ed1881ad87696134ce3ca add DISTCHECK_ONLY, don't hide it under gcc-5
@andy5995 pushed 1 commit.
fbb18e283429f5a013be47b70c10fda385b37c38 use xenial instead of trusty
@andy5995 pushed 1 commit.
f7141d51fda221a03534e4dc8cef5c8bf9a8b92c reduce clang to 3.5
@andy5995 pushed 1 commit.
002d1bcdb8cc80e7d17f73f696673baf880a625b try default clang on xenial
@andy5995 pushed 1 commit.
d0793e4174b4a233f800a5df272e3a2314f00e59 try default clang on trusty
@andy5995 pushed 1 commit.
da10acd71cafecfacc05560226d37254cd02caa4 clang:revert to using 5.0 on trusty
andy5995 commented on this pull request.
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
I don't see that the 3.4 toolchain is available anymore https://www.ubuntuupdates.org/package/xorg-edgers/trusty/main/base/llvm-tool...
3.5 fails on trusty and xenial 5 passes on trusty 7 passes on xenial
Oh.. a lot of errors when building the documentation on xenial
https://travis-ci.org/geany/geany/jobs/528635477#L1635
compared to trusty
https://travis-ci.org/geany/geany/jobs/527884388#L1667
Should a build fail when that happens?
@andy5995 pushed 1 commit.
3ba478bb28f7ae4c3fad6be21471604adcd47414 allow clang to fail
We shouldn't be allowing anything to fail, the problem with clang needs to be fixed.
Not sure why, but it seems to have the wrong library version, you can see its using ".../c++/4.8/..." with clang 5. Thats likely the problem.
@andy5995 pushed 1 commit.
2bc15141f82d7ab07e4a5d0b5096ec79df758ba9 remove cache directive, try clang 3.5 again; also...
@andy5995 pushed 1 commit.
8eaa3bfab25868e504df8be559dbfeb4b2196af4 specifying 3.5 on trusty
@andy5995 pushed 1 commit.
baebe829e8f381d8f3a3c0f5aa4d039cdff0588e install libc++
@andy5995 pushed 1 commit.
b89920961ac0b0bc0b5c9038becd7bb88fed8e7e clang:explicity enable C++11 support
@andy5995 pushed 1 commit.
009ab6e0f9ccb0877a35cc5583da30a4507c9a5b make:temporarily add -n and VERBOSE=1
Note: clang 5 doesn't work here (even with -std=c++11) but clang 6 does. But removing the "noexcept"s from LineMarker (`scintilla/src/linemaker.h`) makes it work.
@b4n does Mitchell actually check with the compilers he claims to support?
Got some VERBOSE output from the latest Travis build https://travis-ci.org/geany/geany/jobs/528700712#L1734
Got some VERBOSE output from the latest Travis build https://travis-ci.org/geany/geany/jobs/528700712#L1734
I see now C++11 support for clang < 6 was already being added by the -std=gnu++11 flag
@andy5995 don't ever use the gnu++ flags, they can contain things that actually conflict with the standard, so its not portable.
@andy5995 don't ever use the gnu++ flags, they can contain things that actually conflict with the standard, so its not portable.
Ok, but I didn't put that there xD I don't know *where* it's coming from...
@b4n does Mitchell actually check with the compilers he claims to support?
I'm not sure. And if it's due to the linemarker thing and this PR got rebased on master, it makes sense it breaks, and it's a good thing we know it.
This said, for me clang 3.8 builds master.
Ok, but I didn't put that there xD I don't know *where* it's coming from...
It's coming from `AX_CXX_COMPILE_STDCXX_11` which figured it's a way to get that compiler to support C++11 -- or at least a part of it that is checked for.
b4n commented on this pull request.
@@ -25,10 +95,15 @@ install:
- sudo apt-get install -y python-lxml before_script: - export CFLAGS="-g -O2 -Werror=pointer-arith -Werror=implicit-function-declaration" + - if [ "$TRAVIS_COMPILER" = "clang" ]; then + CFLAGS="$CFLAGS -std=c++11"; + fi
don't do that, `configure` is responsible for this kind of stuff, and it should be doing it alright. And if it ain't, it might be worth fixing it.
b4n commented on this pull request.
@@ -25,10 +95,15 @@ install:
- sudo apt-get install -y python-lxml before_script: - export CFLAGS="-g -O2 -Werror=pointer-arith -Werror=implicit-function-declaration" + - if [ "$TRAVIS_COMPILER" = "clang" ]; then + CFLAGS="$CFLAGS -std=c++11"; + fi
BTW: `CFLAGS` is for *C* flags, not C++ ones. `CXXFLAGS` is for C++ flags. So it makes no sense to set `CFLAGS=-std=c++11`, as it's basically saying "use C++11 when compiling C code".
b4n commented on this pull request.
@@ -37,7 +112,10 @@ script:
mkdir _build && cd _build && { ../configure $CONFIGURE_FLAGS || { cat config.log; exit 1; } ; } && - make -j2 && - make -j2 check && - make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + if [ -z "$DISTCHECK_ONLY" ]; then + make -j2 VERBOSE=1 -n && + make -j2 check; + else + make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + fi;
I don't like that, because what you do is build one more time, and only that one do the distcheck. And given how distcheck works (make the tarball and try to build it), it could possibly be affected by the configuration options selecting buildig or not some code (even though it should not if we didn't mess up, but CI is to catch case where we did, isn't it :smile: ). So it is a good thing it's checked for all variations of the environment.
Only the same environment with different compilers is not useful, because building with another compiler shouldn't affect *what* we're building.
andy5995 commented on this pull request.
@@ -37,7 +112,10 @@ script:
mkdir _build && cd _build && { ../configure $CONFIGURE_FLAGS || { cat config.log; exit 1; } ; } && - make -j2 && - make -j2 check && - make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + if [ -z "$DISTCHECK_ONLY" ]; then + make -j2 VERBOSE=1 -n && + make -j2 check; + else + make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + fi;
I didn't completely understand this but let me know if the change I made is what you want.
@b4n does Mitchell actually check with the compilers he claims to support?
I'm not sure. And if it's due to the linemarker thing and this PR got rebased on master, it makes sense it breaks, and it's a good thing we know it.
This said, for me clang 3.8 builds master.
I tried with 3.8. fail.
This said, for me clang 3.8 builds master.
I tried with 3.8. fail.
Here clang 3.9 works, clang 5.0 fails. Note `make clean` needed to make it fail. Also I'm building GTK-3 all the time, but GTK-2 works with 3.9 as well.
Now its failed on another `noexcept`, @andy5995 please don't trigger another build, its not possible to see old build logs, wait for others to see it.
andy5995 commented on this pull request.
@@ -37,7 +112,10 @@ script:
mkdir _build && cd _build && { ../configure $CONFIGURE_FLAGS || { cat config.log; exit 1; } ; } && - make -j2 && - make -j2 check && - make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + if [ -z "$DISTCHECK_ONLY" ]; then + make -j2 VERBOSE=1 -n && + make -j2 check; + else + make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + fi;
Now its failed on another `noexcept`, @andy5995 please don't trigger another build, its not possible to see old build logs, wait for others to see it.
Sure, I'll wait. But I do believe you can see old build logs at https://travis-ci.org/geany/geany/pull_requests
@b4n the comment above by @andy5995 about library versions was an observation I made that the LineMarker failure happened in an STL file in ".../c++/n.m/..." where the `n.m` version did not match the compiler version.
@andy5995 installing different libraries isn't the solution, it needs to use whats on peoples systems, the point of travis is to find problems, not for us to force it to work by creating a non-standard configuration :)
Here clang 3.9 works, clang 5.0 fails. Note `make clean` needed to make it fail.
Do you build a clean clone each time? Needing `make clean` doesn't suggest so, and I personally wouldn't try rebuilding half the project with another compiler and expect things to go smoothly :) Also, just to me sure: you need to re-run configure with the new values of `CC` and `CXX`, not merely run `make CC=... CXX=...` -- the later *could* work, but it also could fail.
https://travis-ci.org/geany/geany/jobs/528683476, build 1959.12:
$ export MATRIX_EVAL="CC=clang-3.5 && CXX=clang++-3.5" $ export TRAVIS_COMPILER=gcc $ export CXX=g++ $ export CXX_FOR_BUILD=g++ $ export CC=gcc $ export CC_FOR_BUILD=gcc
That's already suspicious. And the job summary suggest Travis thinks it's gcc, but we set clang-3.5.
/usr/bin/../lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/unique_ptr.h:166:18:
*That* gets me worried: it looks like it's using GCC 4.8 include paths with Clang…
Similarly in build 1959.13 which is supposed to be Clang 5.0 :
$ export MATRIX_EVAL="CC=clang-5.0 && CXX=clang++-5.0" $ export TRAVIS_COMPILER=clang $ export CXX=clang++ $ export CXX_FOR_BUILD=clang++ $ export CC=clang $ export CC_FOR_BUILD=clang $ clang --version clang version 7.0.0 (tags/RELEASE_700/final) […] /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/unique_ptr.h:217:18:
Maybe it's nothing, but it'd get me looking.
b4n commented on this pull request.
@@ -37,7 +112,10 @@ script:
mkdir _build && cd _build && { ../configure $CONFIGURE_FLAGS || { cat config.log; exit 1; } ; } && - make -j2 && - make -j2 check && - make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + if [ -z "$DISTCHECK_ONLY" ]; then + make -j2 VERBOSE=1 -n && + make -j2 check; + else + make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + fi;
I didn't completely understand this but let me know if the change I made is what you want.
now `DISTCHECK_ONLY` is never set, is it? but stop worrying about this part and we'll see once the build stuff is dealt with
Needing make clean doesn't suggest so, and I personally wouldn't try rebuilding half the project with another compiler and expect things to go smoothly :)
Yep, totally agree, thats why I did make clean in between (although with meson it wouldn't be needed `</troll>`) just that I forgot one time and noted it "worked", and despite the mixed compiles it ran too :)
Also, just to me sure: you need to re-run configure with the new values of CC and CXX, not merely run make CC=... CXX=... -- the later could work, but it also could fail.
Yes, I'm running configure each time.
That gets me worried: it looks like it's using GCC 4.8 include paths with Clang…
I think (but no expert on this level of details) that for the STL clang uses the same as gcc so the compiled products can be linked together, the clang package here is dependent on libstdc++ and libgcc not any clang specific version. On that basis I wonder if the version difference isn't an issue since one is a gcc version and the other is a clang version?
(although with meson it wouldn't be needed </troll>)
Also not needed with https://github.com/kugel-/kmake :-)
b4n commented on this pull request.
@@ -37,7 +112,10 @@ script:
mkdir _build && cd _build && { ../configure $CONFIGURE_FLAGS || { cat config.log; exit 1; } ; } && - make -j2 && - make -j2 check && - make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + if [ -z "$DISTCHECK_ONLY" ]; then + make -j2 VERBOSE=1 -n && + make -j2 check; + else + make -j2 distcheck DISTCHECK_CONFIGURE_FLAGS="$CONFIGURE_FLAGS"; + fi;
my bad I missed the one in the matrix
@andy5995 what about the latest state on #2149? it's still verbose but indeed I don't know how to improve it, and has a solution I like better for running only some targets. Also I dropper clang builds failing because we have code they don't like, see that PR for details on why.
https://travis-ci.org/geany/geany/jobs/528683476, build 1959.12:
$ export MATRIX_EVAL="CC=clang-3.5 && CXX=clang++-3.5" $ export TRAVIS_COMPILER=gcc $ export CXX=g++ $ export CXX_FOR_BUILD=g++ $ export CC=gcc $ export CC_FOR_BUILD=gccThat's already suspicious. And the job summary suggest Travis thinks it's gcc, but we set clang-3.5.
Oh, looks like I forgot to set the "compiler" in that section. I'll push a new commit
@andy5995 pushed 1 commit.
7509dfefc5f3c320035a612462a15e93086dba4b use clang 3.5, add missing "compiler" line
https://travis-ci.org/geany/geany/jobs/528683476, build 1959.12:
$ export MATRIX_EVAL="CC=clang-3.5 && CXX=clang++-3.5" $ export TRAVIS_COMPILER=gcc $ export CXX=g++ $ export CXX_FOR_BUILD=g++ $ export CC=gcc $ export CC_FOR_BUILD=gccThat's already suspicious. And the job summary suggest Travis thinks it's gcc, but we set clang-3.5.
Oh, looks like I forgot to set the "compiler" in that section. I'll push a new commit
I see now you're trying stuff in #2149 so I'll hold off on any more changes.
btw, anyone with "maintainer" access can just edit the files in this PR.
Similarly in build 1959.13 which is supposed to be Clang 5.0 :
$ export MATRIX_EVAL="CC=clang-5.0 && CXX=clang++-5.0" $ export TRAVIS_COMPILER=clang $ export CXX=clang++ $ export CXX_FOR_BUILD=clang++ $ export CC=clang $ export CC_FOR_BUILD=clang $ clang --version clang version 7.0.0 (tags/RELEASE_700/final) […] /usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/unique_ptr.h:217:18:Maybe it's nothing, but it'd get me looking.
afaik, it's showing that near the start because 7.0 is the default. Later it installs the correct packages and sets the other vars appropriately.
But for the other quote (not included ) where it shows gcc vars being exported, I'll say again it's because I forgot to add the compiler line to that section.
@andy5995 what about the latest state on #2149? it's still verbose but indeed I don't know how to improve it, and has a solution I like better for running only some targets. Also I dropper clang builds failing because we have code they don't like, see that PR for details on why.
@b4n LGTM feel free to do any fine-tuning and finish it up
Closed #2067.
Closed as replaced by #2149, @andy5995 thanks for starting the process.
You're welcome
github-comments@lists.geany.org