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

1. Move toxcore travis build scripts out of .travis.yml. #29

Merged
merged 1 commit into from
Aug 11, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 11, 2016

This is in preparation for having multiple types of build. One of the future
builds will be a hstox build, another may be frama-c or some other static
analyser. It makes sense to split these up into multiple builds, because each of
them can take a while, and running them in parallel will speed things up. Also,
the hstox test coverage should be reported separately from the toxcore auto_test
coverage.


This change is Reviewable

@iphydf iphydf changed the title Move toxcore travis build scripts out of .travis.yml. 1. Move toxcore travis build scripts out of .travis.yml. Aug 11, 2016
@iphydf iphydf force-pushed the travis-scripts branch 2 times, most recently from edefb63 to 88ceb02 Compare August 11, 2016 17:25
@iphydf iphydf assigned tux3 and unassigned tux3 Aug 11, 2016
@GrayHatter
Copy link

Reviewed 2 of 4 files at r1.
Review status: 2 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


other/travis/toxcore-install, line 15 [r1] (raw file):


# Build apidsl.
git clone --depth=1 https://github.com/iphydf/apidsl

do we want to cache this as well, then we can git pull.

For this it's very low gain, but others will likely use it as an example when adding new sections


other/travis/toxcore-install, line 28 [r1] (raw file):

}

# Install libsodium (not in ubuntu-precise).

I'm surprised Travis doesn't have this available to install. Did you check?


other/travis/toxcore-script, line 18 [r1] (raw file):

  --enable-logging \
  --enable-ntox \
  CFLAGS="-O0 -Wall -Wextra -fprofile-arcs -ftest-coverage"

missing the TRAVIS_ENV define


other/travis/toxcore-script, line 21 [r1] (raw file):


make
make check || true

not anymore


Comments from Reviewable

This is in preparation for having multiple types of build. One of the future
builds will be a hstox build, another may be frama-c or some other static
analyser. It makes sense to split these up into multiple builds, because each of
them can take a while, and running them in parallel will speed things up. Also,
the hstox test coverage should be reported separately from the toxcore auto_test
coverage.
@iphydf
Copy link
Member Author

iphydf commented Aug 11, 2016

Review status: 1 of 4 files reviewed at latest revision, 4 unresolved discussions.


other/travis/toxcore-install, line 15 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

do we want to cache this as well, then we can git pull.

For this it's very low gain, but others will likely use it as an example when adding new sections

No, I don't think we should ever cache git checkouts. It will clutter the cache for no gain. We use --depth=1 on all git clones, so we always only get the most recent version. Cloning any git repo takes a few seconds. Caching it would add complexity that is not warranted by the potential tiny gain in some situations.

other/travis/toxcore-install, line 28 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

I'm surprised Travis doesn't have this available to install. Did you check?

It is in the debian-sid repo. I have never successfully installed anything from that repo, so I didn't try. Since we already build a bunch of things here, risking breakage due to debian-sid being unstable is not worth it.

other/travis/toxcore-script, line 18 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

missing the TRAVIS_ENV define

Done.

other/travis/toxcore-script, line 21 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

not anymore

Done.

Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Aug 11, 2016

Review status: 1 of 4 files reviewed at latest revision, 4 unresolved discussions.


other/travis/toxcore-install, line 15 [r1] (raw file):

Previously, iphydf wrote…

No, I don't think we should ever cache git checkouts. It will clutter the cache for no gain. We use --depth=1 on all git clones, so we always only get the most recent version. Cloning any git repo takes a few seconds. Caching it would add complexity that is not warranted by the potential tiny gain in some situations.

Ah, what would in fact help is caching apidsl itself. Either way, this PR is mostly about moving script from one place to another. We can think about that later.

Comments from Reviewable

@iphydf iphydf assigned GrayHatter and unassigned JFreegman Aug 11, 2016
@GrayHatter
Copy link

:lgtm:


Reviewed 1 of 4 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@iphydf iphydf closed this Aug 11, 2016
@iphydf iphydf deleted the travis-scripts branch August 11, 2016 23:21
@iphydf iphydf merged commit 5092107 into TokTok:master Aug 11, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
zugz pushed a commit to zugz/c-toxcore that referenced this pull request Jan 2, 2018
This pull request was closed.
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.

4 participants