Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split Travis build into stages #62

Merged
merged 44 commits into from Sep 14, 2017
Merged

Conversation

hawkw
Copy link
Collaborator

@hawkw hawkw commented Sep 7, 2017

This PR rewrites our Travis config to use Travis' Build Stages feature. It also includes my PR with fixes to codecov.io coverage uploading (#61). I've rewritten the step for publishing RustDoc to GitHub Pages to use a Travis Deploy stage, removing the dependency on travis-cargo.

Closes #59
Closes #61

@hawkw hawkw requested a review from olix0r September 7, 2017 16:12
@hawkw hawkw self-assigned this Sep 7, 2017
@carllerche carllerche mentioned this pull request Sep 8, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 677c2f8 on hawkw:eliza/build-stages into ** on carllerche:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling a359e99 on hawkw:eliza/build-stages into ** on carllerche:master**.

@hawkw
Copy link
Collaborator Author

hawkw commented Sep 11, 2017

Okay, so I have code coverage working finally using tarpaulin, but it looks like running the hpack tests with coverage enabled takes so long that Travis kills the build. I suspect this would have occurred with kcov as well, if it didn't segfault. Will try and see if I can configure tarpaulin to skip those tests...

@hawkw
Copy link
Collaborator Author

hawkw commented Sep 11, 2017

see xd009642/tarpaulin#32

@hawkw
Copy link
Collaborator Author

hawkw commented Sep 12, 2017

thanks to xd009642/tarpaulin@8e3fe3a, coverage may actually work now (fingers crossed!)

@codecov-io
Copy link

codecov-io commented Sep 12, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@09744f3). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #62   +/-   ##
=========================================
  Coverage          ?   50.41%           
=========================================
  Files             ?       44           
  Lines             ?     4953           
  Branches          ?        0           
=========================================
  Hits              ?     2497           
  Misses            ?     2456           
  Partials          ?        0

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 09744f3...940afe9. Read the comment docs.

@coveralls
Copy link

coveralls commented Sep 12, 2017

Coverage Status

Changes Unknown when pulling 4a7a8de on hawkw:eliza/build-stages into ** on carllerche:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 940afe9 on hawkw:eliza/build-stages into ** on carllerche:master**.

@carllerche
Copy link
Collaborator

Looks like stuff is happening... What are the next steps?

@hawkw
Copy link
Collaborator Author

hawkw commented Sep 13, 2017

Okay @carllerche, I think this will be ready to merge after build #320 is done.

I've disabled the codecov.io bot comment (since it's configured in YAML), but I haven't disabled the coveralls bot, since that has to be done in the repo's settings on Coveralls (which I don't have access to).

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 521340a on hawkw:eliza/build-stages into ** on carllerche:master**.

@carllerche
Copy link
Collaborator

Looks good. We probably should stick w/ one option to limit noise. Could you remove coveralls?

Copy link
Collaborator

@olix0r olix0r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, few questions

.ci/coverage.sh Outdated
#!/bin/bash

# this is in a separate script from `.travis.yml` so we can set this option.
shopt -s extglob
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always set -eu unless it's really clear that this isn't necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this script is actually no longer used – I should rm it

.ci/coverage.sh Outdated
cmake .. &&
make &&
make install DESTDIR=../tmp &&
cd ../..
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you use set -e, don't need these long && chains

.ci/coverage.sh Outdated
cd ../..

if [ $TRAVIS_RUST_VERSION = nightly ]; then
./kcov-master/tmp/usr/local/bin/kcov --exclude-pattern=/.cargo --coveralls-id=$TRAVIS_JOB_ID --verify target/kcov target/debug/*-*[!.]? &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be careful to quote "$TRAVIS_JOB_ID" so it can't be used to inject nefarious things ;)

(PSA shellcheck is a great tool that will complain about this sort of thing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can I brew install shellcheck?

.ci/coverage.sh Outdated
shopt -s extglob

wget https://github.com/SimonKagstrom/kcov/archive/master.tar.gz &&
tar xzf master.tar.gz &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just so i have context...

will this all run on every commit? can we cache the install directory via travis so that we only need to build if the cache is gone? If so, this should probably be guarded by if [ -x tmp/kvoc ] ... or something?

@hawkw
Copy link
Collaborator Author

hawkw commented Sep 14, 2017

@carllerche / @olix0r, this should be ready to merge now?

@carllerche
Copy link
Collaborator

Yes! Thanks for putting in the work and getting this done.

I'll merge and any further tweaks can happen separately.

@carllerche
Copy link
Collaborator

I merged in master, I guess it has to go through CI again!

@hawkw
Copy link
Collaborator Author

hawkw commented Sep 14, 2017

It's really strange to me that 1c2b030 broke this but 3abc489 built fine?

@carllerche carllerche merged commit e4b8dde into hyperium:master Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants