[WIP] Docker based builds/tests by blueyed · Pull Request #1907 · awesomeWM/awesome

blueyed · 2017-07-04T21:48:54Z

TODO:

code coverage
compare with previous .travis.yml!
- test previous commits
local caching: http://circleci.com/docs/1.0/docker/#caching-docker-layers

Docker Image Caching:
Saves ~6 minutes (13 min 16 sec instead of 19 min 14 sec) for all 4 builds.
Cache can be deleted manually on Travis, or should use e.g. the md5 of the Dockerfile in the image file name.

psychon · 2017-07-05T08:21:51Z

docker/Dockerfile

+
+# RUN apk add build-base
+# TODO: Why is g++ required?!  Why does cmake "Check for working CXX compiler"?
+RUN apk --no-cache add gcc g++ make musl-dev


Google found https://cmake.org/pipermail/cmake/2010-August/039245.html, but we already call project with just C as the languages in awesomeConfig.cmake...?

Some quick testing suggests that CMakeLists.txt must contain the call to project(). Even if I move it after include(awesomeConfig.cmake) does CMake stop checking for a C++ compiler. Magic?

Yup, CMake scans CMakeLists.txt for cmake_minimum_required and project early on. If no project is found, it synthesises a PROJECT(Project) command and adds it before the first line.
https://gitlab.kitware.com/cmake/cmake/blob/43c3afa74538fd7b78bcf534bfb3fa93c3c1f191/Source/cmMakefile.cxx#L1442-1458

Fixed by #1909.

CMake "fakes" a call to "project(Project C CXX)" if it does not see a project() call in CMakeLists.txt. Since we had this call in a different file, this default applied. This meant that CMake unnecessarily required a C++ compiler. Fix/work-around this by moving the call to project() into CMakeLists.txt Reference: awesomeWM#1907 (comment) Signed-off-by: Uli Schlachter <psychon@znc.in>

CMake "fakes" a call to "project(Project C CXX)" if it does not see a project() call in CMakeLists.txt. Since we had this call in a different file, this default applied. This meant that CMake unnecessarily required a C++ compiler. Fix/work-around this by moving the call to project() into CMakeLists.txt Reference: #1907 (comment) Signed-off-by: Uli Schlachter <psychon@znc.in>

codecov · 2017-07-16T23:25:40Z

Codecov Report

Merging #1907 into master will increase coverage by 0.17%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1907      +/-   ##
==========================================
+ Coverage   79.55%   79.72%   +0.17%     
==========================================
  Files         404      404              
  Lines       27652    27857     +205     
  Branches      991      999       +8     
==========================================
+ Hits        21998    22209     +211     
+ Misses       5218     5206      -12     
- Partials      436      442       +6

Flag	Coverage Δ
#c_code	`65.97% <ø> (+0.5%)`	⬆️
#functionaltests	`66.1% <ø> (+0.19%)`	⬆️
#samples	`67.71% <ø> (+0.3%)`	⬆️
#unittests	`66.09% <ø> (+0.32%)`	⬆️

Impacted Files	Coverage Δ
luaa.c	`56.36% <0%> (-3.9%)`	⬇️
objects/client.h	`59.09% <0%> (-3.41%)`	⬇️
common/lualib.c	`8.69% <0%> (-3.07%)`	⬇️
common/util.h	`66.66% <0%> (-2.3%)`	⬇️
mousegrabber.c	`66.66% <0%> (-1.2%)`	⬇️
keygrabber.c	`71.05% <0%> (-1.17%)`	⬇️
common/buffer.h	`60.6% <0%> (-1.16%)`	⬇️
common/buffer.c	`44.18% <0%> (-1.06%)`	⬇️
tests/test-gravity.c	`83.08% <0%> (-0.74%)`	⬇️
xkb.c	`50.87% <0%> (-0.59%)`	⬇️
... and 110 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ac6db5...7e8d528. Read the comment docs.

blueyed · 2017-07-16T23:28:12Z

Yay, first (partly) coverage upload succeeded.

Elv13 · 2017-07-17T21:31:48Z

docker/Dockerfile

@@ -0,0 +1,81 @@
+# Dockerfile for awesomeWM.


Could you move the Dockerfile into a subdirectory? At some point we might/should have many Dockerfiles for jobs like a PPA upload and older distribution support (executed on a timely base instead of each PR changes).

It is already in a subdir?!

Inititally I had multiple, e.g. .dep for the base dependencies - might come back to this..

Yes, but wouldn't another level welcome? I usually see Dockerfile being built with a directory path instead of the Dockerfile absolute path. This allows for image specific assets to be "encapsulated" into the sub directory. Something like:

$BASE_DIR/docker/docker-awesome_coverage/Dockerfile # The current Alpine base $BASE_DIR/docker/docker-awesome_ppa/Dockerfile # Ubuntu LTS with PPA upload (no build) $BASE_DIR/docker/docker-awesome_dotdeb/Dockerfile # "make package" for Debian stable and testing $BASE_DIR/docker/docker-awesome_coverity/Dockerfile # Static analysis $BASE_DIR/docker/docker-awesome_scanbuild/Dockerfile # Static and dynamic analysis $BASE_DIR/docker/docker-awesome_bleeding_edge/Dockerfile # Use all the AUR -git / luarocks dependencies to get early breakage warnings.

Note that those Dockerfiles can be written over time. I already have a base for the Coverity one (still need to ask for freebies from them. It is usually very expensive, but they it away as long as the code is Free).

Yes, makes sense, although I'd prefer shorter names.. ;)
I think for now we can have it at the top level though for our main one.

blueyed · 2017-07-17T23:51:08Z

What is the best way to pass arguments to check-integration / tests/run.sh?
I've used $TEST_RUN_ARGS for now - should that get added to CMakeLists.txt then, as (renamed) $ENV{TEST_INTEGRATION_ARGS}?

blueyed · 2017-07-17T23:52:06Z

As for caching: we could use the Docker Hub for this in general, i.e. having tagged base images maybe, that could be used by others then, too?
Currently I am trying to docker save/load it to Travis' cache.

blueyed · 2017-07-18T00:08:16Z

Still at 10mins with the cached Docker image (not optimized for size though): https://travis-ci.org/awesomeWM/awesome/builds/254663200

blueyed · 2017-07-18T17:17:15Z

Might be relevant for gcov: https://gcc.gnu.org/onlinedocs/gcc/Cross-profiling.html (currently running it in build appears to fix it).

Is there a reason why we run two builds with coverage?
We could just report to codecov and coveralls from the same build job?! (this would also make their numbers more comparable)

blueyed · 2017-07-18T20:32:47Z

btw: I've edited the coveralls settings (https://coveralls.io/github/awesomeWM/awesome/settings) to post comments again (which is only possible globally?!), and also to include branch coverage (what codecov is doing with this PR already).

psychon · 2017-07-19T07:00:12Z

Is there a reason why we run two builds with coverage?

Codecov supports tags ("this code was executed by functional tests"). For this to work, we have to delete the coverage files after uploading. However, for coveralls we need an "all in one" coverage file.

blueyed · 2017-07-19T08:13:20Z

@psychon
That should be handled by now.

psychon · 2017-07-19T09:10:55Z

Okay, good to know.

So, should some parts of this be done in separate PRs? For example, moving "all the mess in .travis.yml" into a shell script without changing anything else could be a PR. Or merging the two coverage builds into one. Those are just two random ideas from the top of my head.
I don't want to dictate how you work, I am just afraid that eventually the [WIP] here disappears and we end up with a huge PR. I really do not like huge PRs.

psychon · 2017-07-19T12:43:02Z

btw: I've edited the coveralls settings (https://coveralls.io/github/awesomeWM/awesome/settings) to post comments again (which is only possible globally?!),

Uhm... why? I like that codecov edits its existing comment instead of posting new ones (well, mostly).

blueyed · 2017-07-19T21:36:33Z

I've edited the coveralls settings

Uhm... why?

For debugging/feedback.

IIRC I've seen that you could configure it through yml/branch eventually, so I might change that.

So, should some parts of this be done in separate PRs?

Yes, and a lot of the recent build changes originated from here already.

moving "all the mess in .travis.yml" into a shell script without changing anything else could be a PR

Might be hard, since it is refactored / based on the Docker build already.
I think regarding that it is not worth backporting that first, but I will keep it in mind.

Or merging the two coverage builds into one.

Still WIP and also not trivial to "backport" probably.

I really do not like huge PRs.

👍

blueyed · 2017-07-20T23:04:55Z

The coverage changes (especially for the C code) are coming from covering luajit (Lua 5.1) now only, while we covered Lua 5.2 and 5.3 before.
Should we re-add it for one or both even?

Regarding coveralls: the coverage of C code is new there, so this decreased overall.

CMake "fakes" a call to "project(Project C CXX)" if it does not see a project() call in CMakeLists.txt. Since we had this call in a different file, this default applied. This meant that CMake unnecessarily required a C++ compiler. Fix/work-around this by moving the call to project() into CMakeLists.txt Reference: awesomeWM#1907 (comment) Signed-off-by: Uli Schlachter <psychon@znc.in>

Conflicts: .travis.yml Makefile

blueyed force-pushed the docker branch from 3bfc012 to 7a66663 Compare July 4, 2017 21:57

psychon reviewed Jul 5, 2017

View reviewed changes

blueyed self-assigned this Jul 6, 2017

psychon mentioned this pull request Jul 9, 2017

Move project() call to CMakeLists.txt #1909

Merged

blueyed force-pushed the docker branch 2 times, most recently from 53103d0 to 225ffc4 Compare July 16, 2017 23:16

blueyed force-pushed the docker branch 3 times, most recently from 73e98f5 to fb36f60 Compare July 17, 2017 21:21

Elv13 reviewed Jul 17, 2017

View reviewed changes

blueyed force-pushed the docker branch from abe8208 to 30b7ac3 Compare July 17, 2017 23:48

blueyed force-pushed the docker branch 2 times, most recently from e8fabb9 to d427de4 Compare July 17, 2017 23:53

blueyed force-pushed the docker branch 2 times, most recently from 147e7b8 to 1dc6cac Compare July 18, 2017 16:53

fixup! WIP: coverage

13a26ad

blueyed force-pushed the docker branch from 80c4fab to 13a26ad Compare September 8, 2017 20:17

awesomeWM deleted a comment from coveralls Sep 8, 2017

Merge branch 'master' into docker

7e8d528

Conflicts: .travis.yml Makefile

awesomeWM deleted a comment from coveralls Oct 23, 2017

Conversation

blueyed commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psychon Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

psychon Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

blueyed Jul 9, 2017

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueyed commented Jul 16, 2017

Uh oh!

Elv13 Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

blueyed Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

Elv13 Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

blueyed Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

blueyed commented Jul 17, 2017

Uh oh!

blueyed commented Jul 17, 2017

Uh oh!

blueyed commented Jul 18, 2017

Uh oh!

blueyed commented Jul 18, 2017

Uh oh!

blueyed commented Jul 18, 2017

Uh oh!

psychon commented Jul 19, 2017

Uh oh!

blueyed commented Jul 19, 2017

Uh oh!

psychon commented Jul 19, 2017

Uh oh!

psychon commented Jul 19, 2017

Uh oh!

blueyed commented Jul 19, 2017

Uh oh!

blueyed commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

blueyed commented Jul 4, 2017 •

edited

Loading

codecov bot commented Jul 16, 2017 •

edited

Loading

blueyed commented Jul 20, 2017 •

edited

Loading