GNOME Bugzilla – Bug 776549
Add continuous integration to GJS
Last modified: 2017-02-15 04:07:28 UTC
It is possible to add CI to GJS [1] on GitHub using Travis (or other CI provider [2]). - I see no downside: all routines, files, procedures are OPTIONAL. - The maintainer do not have to enable CI. - The maintainer do not have to care about CI failures, messages, and warnings. - But, if CI shows failures, messages, or warnings, it is helping you improve the code quality (NEVER the opposite). - You possibly ask: suppose the CI patch got accept and the maintainer do not enable CI in the main/official GitHub repo. I'll answer: "the show goes on" as usual. But you still benefit from it if at least one fork has it enabled. Once the fork get updated/rebased/pushed, the build and tests will be executed. On the other hand: - You can automate the build and testing for any PR/commit. - You can have a preview of how your software behaves on gcc version X, or using an LTS distro, maybe clang, why not ARM (yes, it is possible). - If you see failures, messages, or warnings on CI, you've got a hint about your code. Remember: you are allowed to ignore it. - you can to stop using CI; you can disable CI; or move to another CI provider. Read more at: https://github.com/GNOME/gjs/pull/4 CI - continuous integration CD - continuous deployment [1] At least some GNOME modules can benefit from CI and/or CD (using flatpak). [2] Travis is a nice and fair CI for OSS. If needed, a move to another CI provider takes no more than a few hours.
As I mentioned in a comment on the pull request, I'm quite in favour of this and, as a GJS maintainer, would use it. (I don't know for sure if the GNOME github account can or would enable it, and anyway I don't have administrator access to that account; but I would maintain a copy of GJS on my own account on which to test new patches.) I've had a similar discussion on IRC a few times and I'll cc Carlos Soriano here who is one of the people I remember discussing this with. (Carlos, feel free to cc anyone else who you think might be interested.) One objection I've heard is that people shouldn't have to use non-free services to contribute to GNOME if they don't want to. My plan for using Travis CI would be something like this: - As a maintainer I would monitor the build status. - When reviewing patches, I would commit them to a branch so that Travis would build it. - Any failures on Travis I would try to debug myself and translate into review comments on the patches, rather than saying "this doesn't build on Travis." - I would not make it a requirement for someone's patch to build green on Travis before submitting it, though contributors would of course be free to do so if they were OK with it. As for free alternatives, I learned recently that we have a Jenkins instance on freedesktop.org [1]. It seems that GNOME's modules mostly use that for automating submissions to Coverity Scan, which I was also planning to do with GJS one of these days. (And, in fact, Coverity Scan is a non-free service, so there is a precedent to Travis.) It is my understanding that this Jenkins instance doesn't have enough capacity to support CI for all GNOME modules, though. I will do further review of the actual code at the pull request on Github that is already up. [1] https://jenkins.freedesktop.org/
As a side note, it is possible to enable the fancy passing/failing icon for the project. It is not necessary to enable CI on the main branch (but I guess it is meaningless if you don't). As a preview, please take a look at: - https://github.com/claudioandre/gjs/tree/readme#gnome-gjs-
Created attachment 343952 [details] Fancy code coverage image
Note 2: it is also possible to have fancy code coverage icon online (automating the code coverage tests).
Created attachment 344084 [details] [review] installed-tests: check if stack is defined In order to avoid to introduce new errors in the error report.
Created attachment 344085 [details] [review] CI: add travis continuous integration Build and test GJS using Travis CI build servers. The goal is to automatically run the tests with every commit or pull request, implementing a continuous processes of quality assessment and control. Although the usage of dockerfiles and saved images is a better solution, it'll probably impact the maintainer experience by making it harder, or, at least, demanding new knowledge. Therefore, in order to avoid nuisance, as a first choice, I think it is better to keep the proposed solution as simple as possible: caches, scripts and no new assignments or learning process.
Attachment 344084 [details] pushed as b8c7195 - installed-tests: check if stack is defined Attachment 344085 [details] pushed as d94eaaf - CI: add travis continuous integration
For future reference, I added the commits here that we discussed on [1] which I pushed to master. I'll also CC Emmanuele Bassi on this bug since we were talking about Travis CI last week! Claudio, thanks for all your hard work! I'm really glad to see this merged. I think we should now try and find out who has the rights to the GNOME Github organization so we can enable Travis on [2] and start using this! (In reply to Claudio André from comment #4) > Note 2: it is also possible to have fancy code coverage icon online > (automating the code coverage tests). How are you generating the code coverage metrics? :-) [1] https://github.com/GNOME/gjs/pull/4 [2] https://travis-ci.org/GNOME/gjs
(In reply to Philip Chimento from comment #8) > How are you generating the code coverage metrics? :-) The image attached is not from a real run. That said, for a real one, do all stuff necessary to run a coverage test: 1. install lcov; autogen --enable-it 2. make check-code-coverage; 3. etc (including register yourself in one onlien service); 4. submit the .info file to "the" online service as the last step of travis.yml; You can also publish the generated GJS html in GJS wiki, GNOME site, etc. But this is a little bit more complicated: keep user and password (obfuscated) in travis.yml, run a tool to upload the files and NO FANCY icon.
Created attachment 344147 [details] Fancy build status icon For future reference, I'm uploading the build passing/failing README possibility.
Created attachment 344239 [details] [review] Cut down the CI build time
Review of attachment 344239 [details] [review]: Oops, I didn't realize this wasn't already squashed in the CI branch. +1
There are other missing "improvements". 1. I already enabled (my guess) all tests. GJS was running 760/31 tests (next patch attached); 2. we can add some error detection, e.g. asan (Address Sanitizer), to CI; 3. also, why not test GJS using moz38 (using the migration path) inside a (new) build job; Well, I already tried to test it using asan. Lets say that after my attempt, I decided to read an article about "How to give bad news in a business message".
Created attachment 344793 [details] [review] Enable more CI tests The autogenargs is due to a future need of: if [ whatever ] autogenargs = "$autogenargs --enable-whatever" fi
Nice! Definitely mozjs38 on CI would be a good thing to do before merging the mozjs38 branch. After that is over I was planning to open a new bug to identify and fix any issues with asan, ubsan, valgrind, and coverity-scan. Once we have those passing locally, then it would indeed be a good idea to add them to the automatic tooling. If I understand correctly, this patch adds xvfb to the gnome-desktop-testing run? But how did those tests pass before, if not using xvfb?
(In reply to Philip Chimento from comment #15) > If I understand correctly, this patch adds xvfb to the gnome-desktop-testing > run? But how did those tests pass before, if not using xvfb? Typically, gnome-desktop-testing runs inside a VM with a full desktop session, to allow real-world validation.
I meant, how did the tests pass on Travis CI before adding this?
(In reply to Philip Chimento from comment #17) > I meant, how did the tests pass on Travis CI before adding this? Only the "added" tests fails without xvfb. All previous tests were passing because a "real X" was not necessary (make sense to me).
Review of attachment 344793 [details] [review]: +1, let's do it.
By the way, I've been asking from time to time on IRC about enabling Travis on the main repo, but I don't seem to catch av at the right time... will try again tomorrow.
I'm not sure who is the guy and how confident he will be. Sorry, if it sounds obvious or pedantic: - it takes approx 5 clicks, including the "Sign Up" one (it will use the users GitHub credentials); - it is pretty safe, I mean, you can't do bad stuff. [1] - after a log in, access https://travis-ci.org/profile/. It should show all organizations and projects the person can enable/disable. The most complicated is to understand the slice control to click on. It is the gray rectangle on the left side of the repository name. [1] The worst possible thing this is enable CI for the wrong repository. In this case, GitHub will show a failure CI build (small red X next to every new commit). Nothing else.
I don't think it'll be a problem ... I just need to catch him when he's online.
I discovered the proper way is not via IRC but by submitting a feature request in Bugzilla: bug 778469.
(In reply to Philip Chimento from comment #15) > Nice! Definitely mozjs38 on CI would be a good thing to do before merging > the mozjs38 branch. mozjs38 works. The main ideia is: - if Travis is building and the current branch is 'wip/ptomato/mozjs38' => So, build using mozjs38; - otherwise, build using mozjs31.
(In reply to Claudio André from comment #24) > (In reply to Philip Chimento from comment #15) > > Nice! Definitely mozjs38 on CI would be a good thing to do before merging > > the mozjs38 branch. > > mozjs38 works. The main ideia is: > - if Travis is building and the current branch is 'wip/ptomato/mozjs38' => > So, build using mozjs38; > - otherwise, build using mozjs31. Nice, thank you! Is it possible to add the mozjs38 change directly to the wip/ptomato/mozjs38 branch? That way, I can just merge it in at the same time the other changes are made. By the way, I am planning to merge that by Monday.
Created attachment 345577 [details] [review] add Mozilla SpiderMonkey 38 testing No behavior changes on a "regular" (non SpiderMonkey 38) branch. --- The same travis.yml handle all branches and behaves differently on *mozjs38
It looks OK to me, but I would suggest changing it unconditionally to 38, no decision making based on the branch name. That way, when we merge the mozjs38 branch to master, there does not have to be any mozjs31 code left in the CI script.
Created attachment 345596 [details] [review] add Mozilla SpiderMonkey 38 testing Note the "artifacts" in the patch.
Review of attachment 345596 [details] [review]: +1, I'll commit this to the mozjs38 branch now. Thank you! ::: test/travis-ci.sh @@ +100,3 @@ ENDPATCH + patch -p1 <<ENDPATCH This is the "artifact" you were referring to? So we just remove this when it's merged to master. @@ +165,3 @@ + + #TODO Fix this upstream + #STOP! /root/jhbuild/checkout/mozjs-38.0.0/js/src/configure.in has changed, and your configure is out of date. Hmm, I thought I had fixed this already in the patches that I committed to jhbuild... Every patch that touches configure.in also makes the corresponding change in configure. In any case, I think this workaround is fine.
(In reply to Philip Chimento from comment #29) > Review of attachment 345596 [details] [review] [review]: > + patch -p1 <<ENDPATCH > > This is the "artifact" you were referring to? So we just remove this when > it's merged to master. Yes.
Just to document it: I will open a new bug report, but up to this moment I failed to get the needed information: I've seen an intermittent error on CI. - It is always the same error in the same place. - It seems new (last week or so). But since it is intermittent, I'm not sure. - Kind of 1 fail in 10 (maybe 20) builds. - A rebuild fix it (what I mean by intermittent). - A 'good' run shows this: [...] PASS: gjs-tests 88 /rooting/maybe-owned/object-destroyed-after-notify PASS: gjs-tests 89 /rooting/maybe-owned/can-destroy-object-in-notify-callback ============================================================================ - The error is: PASS: gjs-tests 88 /rooting/maybe-owned/object-destroyed-after-notify ERROR: gjs-tests - too few tests run (expected 89, got 88) ERROR: gjs-tests - exited with status 133 (terminated by signal 5?) ============================================================================
Well spotted ... no need to open up a new bug report, I've already got a patch at bug 776966 and it was just waiting for review. This makes me wonder if there's a way to get CI to print out the failing log file if a test fails?
(In reply to Philip Chimento from comment #32) > This makes me wonder if there's a way to get CI to print out the failing log > file if a test fails? Yes, (I was experimenting with it). Unfortunately, Travis does not have one needed resource, but we can workaround it [1]. [1] Kudos to Travis, in general. In this case, even a Windows free (as in beer) CI has what I need.
If you mean the patch on your Github branch, then yes, that looks quite suitable :-)
Created attachment 345709 [details] [review] Print log on failure
Created attachment 345713 [details] [review] Bash "Compilation" failing because the variable is not set I've seen errors because the variable is not set. Submitting for discussion.
Review of attachment 345709 [details] [review]: +1
Review of attachment 345713 [details] [review]: ::: installed-tests/scripts/testCommandLine.sh @@ -1,3 @@ #!/bin/sh -if test $GJS_USE_UNINSTALLED_FILES -eq 1; then Is the problem the -eq comparison, or the lack of quotes around the variable expansion, or really both?
Both. Without quotes the expansion is a problem: [ NOTHING -eq 1 ] => [: -eq: unexpected operator And, -eq is for integer comparison. [ "" -eq 1 ] => [: Illegal number: ------------- Seems easier to use string comparison: [ "" = "1" ] OR [ "whatever" = "1" ]
Review of attachment 345713 [details] [review]: +1 then.