Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1873133003:
Add travis CI config (Closed)
|
Created:
4 years, 8 months ago by Ted Mielczarek Modified:
4 years, 8 months ago CC:
google-breakpad-dev_googlegroups.com Base URL:
https://chromium.googlesource.com/breakpad/breakpad.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd travis CI config
This will let us setup travis-ci on the Breakpad GitHub mirror.
R=vapier@chromium.org, mark@chromium.org
BUG=
Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0203b0cbddeffff2f408338f10a5f775c0d2f5a7
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fix review comments #
Total comments: 12
Patch Set 3 : More review fixup #
Messages
Total messages: 11 (2 generated)
I enabled Travis on my own GitHub fork and after some tweaking got it to build and run tests: https://travis-ci.org/luser/breakpad/builds/121711686 Someone with access to the Google Breakpad GitHub mirror will have to enable Travis there, obviously, but it's just a few clicks to make that happen. I have this building against GCC 4.8 right now. We could obviously set up a matrix of more compilers (like clang), but I didn't want to fiddle with that right now: https://docs.travis-ci.com/user/customizing-the-build/#Build-Matrix I tried to put all the actual logic into the scripts to keep .travis.yml fairly simple. travis-checkout.sh just deals with fetching the gclient deps, since Travis does a straight git clone of the repository from GitHub. travis-build.sh is mostly just "configure && make check", but we do have to explicitly set CC/CXX because Travis is weird and sets them to the system version of GCC, which is GCC 4.6. It would be nice if they had better baked-in support for using different toolchain versions, but what you see here seems to be the state-of-the-art. If this proves useful we could look into getting them to enable OS X support for the repository, and we could also setup a Windows build on AppVeyor.
vapier@chromium.org changed reviewers: + vapier@chromium.org
i've enabled travis for the main google repo now in travis https://codereview.chromium.org/1873133003/diff/1/.travis.yml File .travis.yml (right): https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode1 .travis.yml:1: language: cpp at the top of this file, could you put: # Travis build integration. # https://docs.travis-ci.com/ https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode2 .travis.yml:2: compiler: gcc add a TODO about adding clang ? you might as well write this now as: compiler: - gcc https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode11 .travis.yml:11: - USE_CC=gcc-4.8 USE_CXX=g++-4.8 there should be a comment here explaining why you need to do this https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode15 .travis.yml:15: - linux add a comment about OS X. maybe just a TODO. https://codereview.chromium.org/1873133003/diff/1/scripts/travis-build.sh File scripts/travis-build.sh (right): https://codereview.chromium.org/1873133003/diff/1/scripts/travis-build.sh#new... scripts/travis-build.sh:4: i would restructure this code a bit up front: setup_env() { ... mess with cc/cxx ... } build() { ./configure make .... } main() { setup_env build } main "$@" https://codereview.chromium.org/1873133003/diff/1/scripts/travis-build.sh#new... scripts/travis-build.sh:12: # Use -j4 for faster builds. Travis build machines under Docker i've never run into this problem. i do: ncpus=$(getconf _NPROCESSORS_ONLN) make -j${ncpus} we should at least calculate the value dynamically rather than hardcode 4 https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh File scripts/travis-checkout.sh (right): https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:3: same comment here about using a main func so it's easier to manage the code https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:9: srcdir=`basename $TRAVIS_BUILD_DIR` please write instead: srcdir=$(basename "${TRAVIS_BUILD_DIR}") https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:10: cd ${TRAVIS_BUILD_DIR}/.. please quote it https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:11: mv $srcdir src quote & use braces mv "${srcdir}" src
Here's a build with these changes: https://travis-ci.org/luser/breakpad/builds/122291607 (note it's failing because I rebased this patch off of the change in https://codereview.chromium.org/1870733005/ ) https://codereview.chromium.org/1873133003/diff/1/.travis.yml File .travis.yml (right): https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode1 .travis.yml:1: language: cpp On 2016/04/11 15:09:17, vapier wrote: > at the top of this file, could you put: > > # Travis build integration. > # https://docs.travis-ci.com/ Done. https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode2 .travis.yml:2: compiler: gcc On 2016/04/11 15:09:17, vapier wrote: > add a TODO about adding clang ? you might as well write this now as: > > compiler: > - gcc Done. https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode11 .travis.yml:11: - USE_CC=gcc-4.8 USE_CXX=g++-4.8 On 2016/04/11 15:09:17, vapier wrote: > there should be a comment here explaining why you need to do this Done. https://codereview.chromium.org/1873133003/diff/1/.travis.yml#newcode15 .travis.yml:15: - linux On 2016/04/11 15:09:17, vapier wrote: > add a comment about OS X. maybe just a TODO. Done. https://codereview.chromium.org/1873133003/diff/1/scripts/travis-build.sh File scripts/travis-build.sh (right): https://codereview.chromium.org/1873133003/diff/1/scripts/travis-build.sh#new... scripts/travis-build.sh:4: On 2016/04/11 15:09:17, vapier wrote: > i would restructure this code a bit up front: Okay. I'm not a big fan of overly-complex shell scripts (I generally move to Python for anything that winds up non-trivial), but I don't care too strongly about it in this case. https://codereview.chromium.org/1873133003/diff/1/scripts/travis-build.sh#new... scripts/travis-build.sh:12: # Use -j4 for faster builds. Travis build machines under Docker On 2016/04/11 15:09:17, vapier wrote: > i've never run into this problem. i do: > > ncpus=$(getconf _NPROCESSORS_ONLN) > make -j${ncpus} I tried something very similar: https://github.com/luser/breakpad/commit/ab5619c5250cb5f0f3a785c107ecf841afff... which made the build use `make -j32`, and it resulted in my GCC processes being killed (presumably by OOMkiller): https://travis-ci.org/luser/breakpad/builds/120983550 > we should at least calculate the value dynamically rather than hardcode 4 This is definitely a hack, but given that it's written to run only in the Travis environment it doesn't bother me overmuch. If I made it `min($ncpus,4)` would that be better? https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh File scripts/travis-checkout.sh (right): https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:3: On 2016/04/11 15:09:18, vapier wrote: > same comment here about using a main func so it's easier to manage the code Done. https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:9: srcdir=`basename $TRAVIS_BUILD_DIR` On 2016/04/11 15:09:18, vapier wrote: > please write instead: > > srcdir=$(basename "${TRAVIS_BUILD_DIR}") Done. https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:10: cd ${TRAVIS_BUILD_DIR}/.. On 2016/04/11 15:09:18, vapier wrote: > please quote it Done. https://codereview.chromium.org/1873133003/diff/1/scripts/travis-checkout.sh#... scripts/travis-checkout.sh:11: mv $srcdir src On 2016/04/11 15:09:18, vapier wrote: > quote & use braces > > mv "${srcdir}" src Done.
https://codereview.chromium.org/1873133003/diff/20001/.travis.yml File .travis.yml (right): https://codereview.chromium.org/1873133003/diff/20001/.travis.yml#newcode23 .travis.yml:23: # TODO: add mac support, needs coordination from Travis. why does it need coordination w/Travis ? i've set up builds before w/OS X and didn't need to talk to anyone. https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh File scripts/travis-build.sh (right): https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:8: if test -n "$USE_CC"; then more standard imo is to use [ instead of test if [ -n "${USE_CC}" ]; then https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:11: if test -n "$USE_CXX"; then same here https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:20: # if we try to use them all, so use at most 4. you should reference this report: https://github.com/travis-ci/travis-ci/issues/1972 it's not a core utilization issue, but more the compiler uses a lot of memory, and when you allow a lot of cores like that, the kernel starts OOM-ing. i would move this section up to setup_env and have it export like NCPUS and JOBS. that way we can allow a higher # depending on the toolchain (sounds like clang doesn't use as much memory). https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:22: jobs=$(($ncpus<4?$ncpus:4)) there's no need to omit spaces ... that just makes it harder to read. jobs=$(( ncpus < 4 ? ncpus : 4 ))
https://codereview.chromium.org/1873133003/diff/20001/.travis.yml File .travis.yml (right): https://codereview.chromium.org/1873133003/diff/20001/.travis.yml#newcode23 .travis.yml:23: # TODO: add mac support, needs coordination from Travis. On 2016/04/11 17:13:35, vapier wrote: > why does it need coordination w/Travis ? i've set up builds before w/OS X and > didn't need to talk to anyone. Oh, I just have outdated information. For the longest time this was in beta and you had to explicitly ask them to enable it for you. I'll fix that comment. https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh File scripts/travis-build.sh (right): https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:20: # if we try to use them all, so use at most 4. On 2016/04/11 17:13:35, vapier wrote: > you should reference this report: > https://github.com/travis-ci/travis-ci/issues/1972 > > it's not a core utilization issue, but more the compiler uses a lot of memory, > and when you allow a lot of cores like that, the kernel starts OOM-ing. Yeah, I understood the root cause, I may have just not phrased my explanation clearly.
https://codereview.chromium.org/1873133003/diff/20001/.travis.yml File .travis.yml (right): https://codereview.chromium.org/1873133003/diff/20001/.travis.yml#newcode23 .travis.yml:23: # TODO: add mac support, needs coordination from Travis. On 2016/04/11 17:30:16, Ted Mielczarek wrote: > On 2016/04/11 17:13:35, vapier wrote: > > why does it need coordination w/Travis ? i've set up builds before w/OS X and > > didn't need to talk to anyone. > > Oh, I just have outdated information. For the longest time this was in beta and > you had to explicitly ask them to enable it for you. I'll fix that comment. Done. https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh File scripts/travis-build.sh (right): https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:8: if test -n "$USE_CC"; then On 2016/04/11 17:13:35, vapier wrote: > more standard imo is to use [ instead of test > > if [ -n "${USE_CC}" ]; then Done. https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:11: if test -n "$USE_CXX"; then On 2016/04/11 17:13:35, vapier wrote: > same here Done. https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:20: # if we try to use them all, so use at most 4. On 2016/04/11 17:13:35, vapier wrote: > you should reference this report: > https://github.com/travis-ci/travis-ci/issues/1972 > > it's not a core utilization issue, but more the compiler uses a lot of memory, > and when you allow a lot of cores like that, the kernel starts OOM-ing. > > i would move this section up to setup_env and have it export like NCPUS and > JOBS. that way we can allow a higher # depending on the toolchain (sounds like > clang doesn't use as much memory). Done. https://codereview.chromium.org/1873133003/diff/20001/scripts/travis-build.sh... scripts/travis-build.sh:22: jobs=$(($ncpus<4?$ncpus:4)) On 2016/04/11 17:13:35, vapier wrote: > there's no need to omit spaces ... that just makes it harder to read. > > jobs=$(( ncpus < 4 ? ncpus : 4 )) Done.
lgtm
Description was changed from ========== Add travis CI config This will let us setup travis-ci on the Breakpad GitHub mirror. R=mark@chromium.org BUG= ========== to ========== Add travis CI config This will let us setup travis-ci on the Breakpad GitHub mirror. R=vapier@chromium.org, mark@chromium.org BUG= Committed: https://chromium.googlesource.com/breakpad/breakpad/+/0203b0cbddeffff2f408338... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 0203b0cbddeffff2f408338f10a5f775c0d2f5a7 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
