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

Separate FreeBSD Travis build in 2 stages #635

Merged
merged 5 commits into from Dec 14, 2017

Conversation

@nurupo
Copy link
Member

@nurupo nurupo commented Dec 12, 2017

We need to pass different argument to the FreeBSD script for stage 1 and 2 so that it could do different things in each stage. The way you would usually do this is by adding a new variable to env, but we can't do that because in order for two jobs to share cache they must have identical env (among other things like compiler, os, etc.), so I call the scripts directly with an extra argument, as this avoid messing with env.


This change is Reviewable

@nurupo nurupo force-pushed the nurupo:travis-freebsd-stages branch from 67caf3c to 21cad79 Dec 12, 2017
@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

The last commit was made purely to see if FreeBSD build succeeds, it should be removed before PR is merged.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

@nurupo nurupo force-pushed the nurupo:travis-freebsd-stages branch from 42be77b to 21cad79 Dec 12, 2017
@nurupo nurupo added PR:ready and removed PR:in_progress labels Dec 12, 2017
@nurupo nurupo changed the title [WIP] Separate FreeBSD Travis build in 2 stages Separate FreeBSD Travis build in 2 stages Dec 12, 2017
@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Should be ready for review.

@sudden6
Copy link

@sudden6 sudden6 commented Dec 12, 2017

Do you think it makes sense to include a hash with the image like you did in the qTox repo? So that when the build scripts change, the image is automatically rebuilt.


Review status: 0 of 6 files reviewed at latest revision, all discussions resolved, some commit checks broke.


Comments from Reviewable

@sudden6
Copy link

@sudden6 sudden6 commented Dec 12, 2017

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


.travis.yml, line 24 at r1 (raw file):

    sudo: required
    install:       other/travis/phase $JOB $ENV install       stage1
    script:        other/travis/phase $JOB $ENV script        stage1

I think this can be removed, along with the script that does nothing?


Comments from Reviewable

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


.travis.yml, line 24 at r1 (raw file):

Previously, sudden6 wrote…

I think this can be removed, along with the script that does nothing?

No, neither of those can be removed. Without having stage1 as an argument, the phase script wouldn't run the freebsd-install-stage1 script. And the script with exit 0 does something -- it terminates the phase script early, preventing it from running toxcore-script.


Comments from Reviewable

@sudden6
Copy link

@sudden6 sudden6 commented Dec 12, 2017

Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


.travis.yml, line 24 at r1 (raw file):

Previously, nurupo wrote…

No, neither of those can be removed. Without having stage1 as an argument, the phase script wouldn't run the freebsd-install-stage1 script. And the script with exit 0 does something -- it terminates the phase script early, preventing it from running toxcore-script.

I'm talking specifically about the script line. If it's not there it shouldn't call anything right? Or is there something I'm missing?


Comments from Reviewable

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Do you think it makes sense to include a hash with the image like you did in the qTox repo? So that when the build scripts change, the image is automatically rebuilt.

Don't think it would be as useful as in the qTox case, but I can add it. We don't need to rebuild the image if any of freebsd scripts are changed though, we have to watch only for changes in freebsd-install-stage1, since it's the only script that creates the VM image, other scripts just use it.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Btw, in case you are confused with how the execution flow works

  • Stage 1
    • install
      • env.sh <-- defines common variables and functions for toxcore-script to work
      • env-freebsd.sh <-- defines FreeBSD-specific common variables and functions for toxcore-script to work
      • freebsd-install <-- defines common variables and functions for freebsd-install-stageN scripts
      • freebsd-install-stage1 <-- creates/updates FreeBSD VM image if needed
    • script
      • env.sh
      • env-freebsd.sh
      • freebsd-script-stage1 <-- terminates the phase script early so that toxcore-script doesn't run in Stage 1
      • (if not for the freebsd-script-stage1 above, toxcore-script would have ran here)
    • after_script
      • env.sh
      • env-freebsd.sh
  • Stage 2
    • install
      • env.sh
      • env-freebsd.sh
      • freebsd-install
      • freebsd-install-stage2 <-- copies toxcore's code inside FreeBSD VM so that toxcore-script could be run
    • script
      • env.sh
      • env-freebsd.sh
      • toxcore-script <-- builds toxcore inside FreeBSD
    • after_script
      • env.sh
      • env-freebsd.sh
@sudden6
Copy link

@sudden6 sudden6 commented Dec 12, 2017

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Technically it would be called by https://github.com/nurupo/InsertProjectNameHere/blob/21cad79a9c635a05098d4da33ffd5b4acd50a858/.travis.yml#L24 because the earlier script command overwrites the later ones. If you remove that script command I pointed to, then yes, it would be called by the one you have pointed out.

We don't want to call toxcore-script, i.e. build toxcore, in Stage 1 yet because then it will be no different than what Travis does right now and it will exceed the 50 minute limit. The whole idea is to split VM image creating and toxcore building in 2 parts so that we wouldn't be hitting time limits.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

I think if you looked into what other/travis/phase script does, it would make it a bit more clear to you.

Travis calls phase script 3 or 4 times for every job, 1st time for install, second time for script, the third time for after_script and the fourth time for after_failure but only if the build failed. Actually, I'm not sure if it will call after_script if the build fails, it might skip it and go straight to after_failure, but that doesn't really matter since this PR doesn't deal with any of them.

@sudden6
Copy link

@sudden6 sudden6 commented Dec 12, 2017

because the earlier script command overwrites the later ones.

Didn't know that, now it's clear. Thx for the explanation :)

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.


.travis.yml, line 24 at r1 (raw file):

Previously, sudden6 wrote…

I'm talking specifically about the script line. If it's not there it shouldn't call anything right? Or is there something I'm missing?

If you remove this script line the one that is defined later in the script will be used and it will call toxcore-script trying to build toxcore.


Comments from Reviewable

@sudden6
Copy link

@sudden6 sudden6 commented Dec 12, 2017

:lgtm_strong: from my side


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


Comments from Reviewable

robinlinden and others added 2 commits Dec 9, 2017
@robinlinden
Copy link
Member

@robinlinden robinlinden commented Dec 12, 2017

:lgtm_strong:


Reviewed 5 of 6 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 12, 2017

Want me to squash the commits?

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Well, I will squash them anyway.

@nurupo nurupo force-pushed the nurupo:travis-freebsd-stages branch from 224c663 to ada0ec2 Dec 13, 2017
@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

There.

@nurupo nurupo force-pushed the nurupo:travis-freebsd-stages branch from ada0ec2 to 92c4108 Dec 13, 2017
@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Rebased on top of @Diadlo's fix_travis_test2 branch (#630) by @robinlinden's request on IRC.

@sudden6
Copy link

@sudden6 sudden6 commented Dec 13, 2017

uh, wtf happens at the windows builds?

@nurupo nurupo closed this Dec 13, 2017
@nurupo nurupo reopened this Dec 13, 2017
@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Oops, accidentally clicked on the wrong button while the page was still loading and layout was moving, didn't mean to close the PR.

Looks like Windows tests are crashing with stack traces and don't complete fast enough triggering Travis time limit. Did this happen before?

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Windows tests are also failing in #630, but on master they succeed.

@robinlinden
Copy link
Member

@robinlinden robinlinden commented Dec 13, 2017

That's really weird. They weren't failing when I LGTM'd it: https://travis-ci.org/TokTok/c-toxcore/builds/313983843

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Actually, it's not that they succeed on master, the tests also fail there, it's just that the jobs don't exceed 50 minutes on master. The test failure is ignored in Windows builds. Those stacktraces might also be Wine bugs, the Wine on Travis must be ancient.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

I don't really understand how Windows jobs that were getting completed in 10 minutes now take over 50. The tests do fail in both cases, so the number of times tests get re-run should be the same. Also, tests get killed after certain amount of time, so there shouldn't be any infinite loop keeping them running. I'm somewhat at loss. It would help if Travis didn't truncate the logs at 4mb.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

There is this thing Travis says

This job is running on our Trusty environment, which has been updated on Tuesday, December 12th, 2017. Read all about this on our blog and take note that you can add group: deprecated-2017Q4 in your .travis.yml file to use the previous image version.

but Windows jobs run in their own environment in Docker, which supposedly should be the same, so I don't think this would have affected them.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Interesting, it looks like the job gets terminated at around 15 minutes of runtime because it outputs too much (see below), but even though Travis says that it's terminated, for some reason it keeps showing the job as running until it gets to 50 minutes.

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated

So it looks like the reason is not in exceeding 50 minute limit but in outputting too much.

Travis has upadted their VM image and by default the latest is being used.
This has caused our Windows builds to fail due to them exceeding Travis's log
limit of 4mb. It appears to be due to the new Docker version the new Trusty
image has, it somehow changed network behaviour in a way that makes toxcore
network module output a lot more of log messages than it used to.
@nurupo nurupo force-pushed the nurupo:travis-freebsd-stages branch from bd54e23 to e84aefe Dec 13, 2017
@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 13, 2017

Looks like the issue was cased by a newer Docker version on the updated Travis Trusty image. Newer Docker changed networking behaviour in a way that causes toxcore's network module spamming more log messages than it used to. For now I have just made it use older Trusty image on Travis, but the proper fix would be to somehow limit the output of the tests so that total job log doesn't exceed 4mb.

@sudden6
Copy link

@sudden6 sudden6 commented Dec 13, 2017

Restarted one failing job, because it was a strange apt error.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Dec 14, 2017

Yay, Travis passed.

@robinlinden
Copy link
Member

@robinlinden robinlinden commented Dec 14, 2017

Reviewed 2 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@robinlinden robinlinden merged commit e84aefe into TokTok:master Dec 14, 2017
3 of 6 checks passed
3 of 6 checks passed
ci/circleci CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
license/cla Contributor License Agreement is not signed yet.
Details
code-review/reviewable 2/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@robinlinden robinlinden added this to the v0.1.11 milestone Dec 14, 2017
@robinlinden
Copy link
Member

@robinlinden robinlinden commented Dec 14, 2017

Merged together with #630, #631, and #632 as Travis required all of them before passing and I'm unable to merge anything without Travis passing.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Apr 16, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

robinlinden
Diadlo
nurupo
You have signed the CLA already but the status is still pending? Let us recheck it.

@nurupo
Copy link
Member Author

@nurupo nurupo commented Apr 16, 2020

@CLAassistant is such a necromancer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.