8000 Circle CI by SkySkimmer · Pull Request #6400 · rocq-prover/rocq · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Circle CI #6400

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

Merged
merged 11 commits into from
Dec 19, 2017
Merged

Circle CI #6400

merged 11 commits into from
Dec 19, 2017

Conversation

SkySkimmer
Copy link
Contributor
@SkySkimmer SkySkimmer commented Dec 12, 2017

I squashed all the @aspiwack commits into one then added mine separately.

VS gitlab:
+ fiat-crypto (Circle has 4GB RAM, gitlab 2GB)
- publishing artefacts (TODO)
* tests with -local, not installed

VS travis:
+ reusing build products
- flambda validate job (TODO?)
- OSX jobs (TODO at least check if free OSX is possible)
- linter (TODO?)

Since we only get 4 jobs on circle we probably want to keep some stuff on travis.
Example run here https://circleci.com/workflow-run/f41442ca-c172-4067-8511-623ded2c768b (needs login, reload the link after login if it redirects to 404) (commits got squashed without change since that)

See #6347.

aspiwack and others added 2 commits December 11, 2017 21:09
Revert "CI: poc Circleci configuration"

Committed on master by mistake. Clearly I'm too clumsy to be trusted
with push rights.

This reverts commit d606a85.

CI: poc Circleci configuration

Fixup

Try minimising build for faster testing

Various fixes

Fixup: yaml identation

Do not -j2: native compiler seems to take too much memory

Revert "Do not -j2: native compiler seems to take too much memory"

This reverts commit 4886151.

Deactivate native compiler

Fixup (how did this happen?)

Do not call time (not install on docker images, will fix later)

Fixup

Fixup
VS gitlab:
+ fiat-crypto (Circle has 4GB RAM, gitlab 2GB)
- caching opam (TODO)
- publishing artefacts (TODO)
* tests with -local, not installed

VS travis:
+ reusing build products
- flambda validate job (TODO?)
- OSX jobs (TODO at least check if free OSX is possible)
- linter (TODO?)
@SkySkimmer SkySkimmer added kind: feature New user-facing feature request or implementation. kind: infrastructure CI, build tools, development tools. needs: review labels Dec 12, 2017
@Zimmi48
Copy link
Member
Zimmi48 commented Dec 12, 2017

That's cool, thank you @SkySkimmer for doing this. The output looks better than on Travis. It is a shame that we have to log in to see it though.

We should do more than just share the build of Coq. The builds of Bignums, Corn, Math-Classes and SSReflect should also be shared (although in the case of SSReflect I wonder what is the dependency today, given that the plugin was merged into Coq).
Currently, there is an inconsistency with Corn not having its own Travis job but Bignums, Math-Classes and Formal-Topology having their own (even though the build of Formal-Topology requires all the previous ones). With CircleCI it is possible to fix this by having a distinct job per development but having each of these jobs depend on the previous one.

For MacOS testing, CircleCI proposes the seed plan free for OSS project (provided we write to them) but it is limited to 500 max minutes/month. So that wouldn't fit with testing every PRs. That's too bad because I'm not happy with Travis for MacOS testing.

Finally, using CircleCI for testing external developments is supposed to help reduce the CI backlog but I don't quite get why you want to test the 32-bits target or the OCaml 4.06 target over there as well.

@Zimmi48
Copy link
Member
Zimmi48 commented Dec 12, 2017

With CircleCI it is possible to fix this by having a distinct job per development but having each of these jobs depend on the previous one.

But it will require rewriting part of the CI logic, e.g. by moving the dependency declaration from the build script to Makefile.ci.

@SkySkimmer
Copy link
Contributor Author

Finally, using CircleCI for testing external developments is supposed to help reduce the CI backlog but I don't quite get why you want to test the 32-bits target or the OCaml 4.06 target over there as well.

Because it's a port of the gitlab stuff, everything that was on gitlab came with.

@maximedenes
Copy link
Member

@SkySkimmer Looks wonderful, thanks! Do you think you'd need to address the remaining TODOs, or we could start using that right away, and remove some of these jobs from Travis?

COQIDE_PACKAGES: &coqide-packages "libgtk2.0-dev libgtksourceview2.0-dev"
#COQIDE_PACKAGES_32BIT: "libgtk2.0-dev:i386 libgtksourceview2.0-dev:i386"
COQIDE_OPAM: &coqide-opam "lablgtk-extras"
COQIDE_OPAM_BE: &coqide-opam-be "num lablgtk.2.18.6 lablgtk-extras.1.6"
Copy link
Member

Choose a reason for hiding this comment

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

In travis we also bound the range of lablgtk to {min..max} supported versions. You may not want to do that here thou.

opam switch ${COMPILER}
eval $(opam config env)
opam config list
opam install -j ${NJOBS} -y camlp5.${CAMLP5_VER} ocamlfind ${EXTRA_OPAM}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto for OCAMLFIND_VER

source ~/.profile

echo 'start:coq.config'
./configure -local -native-compiler no ${EXTRA_CONF}
Copy link
Member

Choose a reason for hiding this comment

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

We need to enable the native-compiler by default, it is a very important component for testing.

Copy link
Member
@ejgallego ejgallego left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me, I need to get more familiar with CircleCI but I don't see a reason this cannot go in now.

@Zimmi48
Copy link
Member
Zimmi48 commented Dec 13, 2017

@maximedenes Even if we merged this right now, it would probably be better to wait a few days / weeks to see if we are happy with it before removing the corresponding jobs in Travis.

@SkySkimmer How complex do you think it would be to also support sharing of the build of package dependencies, such as Bignums? Do you agree this would require stating dependencies in Makefile?

@SkySkimmer
Copy link
Contributor Author

@SkySkimmer How complex do you think it would be to also support sharing of the build of package dependencies, such as Bignums? Do you agree this would require stating dependencies in Makefile?

I don't think it would be too hard. I don't think we need to put the dependencies in the makefile, although it may be cleaner than keeping them in the scripts. Mostly we just need the script for foo to skip itself if foo is already present, because the makefile rules are .PHONY so if I do (with dependencies in Makefile)

Job 1: make ci-bignum
(bignum.sh is run, bignum built and installed)

Job 2: get bignum installed files, make ci-foo
(foo depends on bignum so bignum.sh is run but we don't want to rebuild it, then foo.sh is run etc)

Currently with dependencies in scripts it should go like

Job 1: make ci-bignum
(bignum.sh is run, bignum built and installed)

Job 2: get bignum installed files, make ci-foo
(foo.sh is run, itself running bignum.sh which needs to skip ahead)

I don't know how easily we can tell whether something is installed, it may need to be done custom per development.

Or maybe have dummy files _build_ci/.ci-foo.done? (then Makefile.ci is like

CI_TARGETS = ...

.PHONY: $(CI_TARGETS)

$(CI_TARGETS): ci-%: _build_ci/.ci-%.done $(DEPS_%)

_build_ci/.ci-%.done:
    dev/ci/ci-%.sh

)

@Zimmi48
Copy link
Member
Zimmi48 commented Dec 13, 2017

I like the .done trick 😀

@SkySkimmer
Copy link
Contributor Author

I seem to only have 3 parallel jobs now. I think the 4th was the free trial.

@SkySkimmer
Copy link
Contributor Author

What to do about install_ssreflect? Make those jobs depend on full mathcomp or have a small ci-ssreflect.sh?

@SkySkimmer
Copy link
Contributor Author

Now I have 4 jobs again. Maybe they had a bug.

@SkySkimmer
Copy link
Contributor Author

Now with longer dependency chain https://circleci.com/workflow-run/4348c189-7999-47ba-93d5-f14886566f8b

@ejgallego
Copy link
Member

I think that while the .done trick may seem attractive, it may however lead to a less dynamic workflow. I dunno.

The current "system" [to call it something] is however intended to work in a different way. If you already have bignums installed and up to date, the make ci-bignums should be extremely cheap as it would do nothing.

This also has problems.

@SkySkimmer SkySkimmer force-pushed the circle-ci branch 6 times, most recently from 5e47be7 to f83e437 Compare December 13, 2017 17:05
Copy link
Member
@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

I haven't read the bulk of the PR, but I think the change to test-suite/Makefile is on the wrong line. Also, can we add a badge to the README?

@@ -176,7 +176,7 @@ summary.log:
# local build, and downloadable on GitLab)
report: summary.log
$(HIDE)bash save-logs.sh
$(HIDE)if [ -n "${TRAVIS}" ]; then find logs/ -name '*.log' -not -name 'summary.log' -exec 'bash' '-c' 'echo "travis_fold:start:coq.logs.$$(echo '{}' | sed s,/,.,g)"' ';' -exec cat '{}' ';' -exec 'bash' '-c' 'echo "travis_fold:end:coq.logs.$$(echo '{}' | sed s,/,.,g)"' ';'; fi
$(HIDE)if [ "(" -n "${TRAVIS}" ")" -o "(" -n "${CIRCLECI}" ")" ]; then find logs/ -name '*.log' -not -name 'summary.log' -exec 'bash' '-c' 'echo "travis_fold:start:coq.logs.$$(echo '{}' | sed s,/,.,g)"' ';' -exec cat '{}' ';' -exec 'bash' '-c' 'echo "travis_fold:end:coq.logs.$$(echo '{}' | sed s,/,.,g)"' ';'; fi
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to add this to the APPVEYOR one, or make a separate CIRCLECI one, unless CIRCLECI also uses travis_fold: (which I'm suspicious of).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I forgot the reason they're separate.

@SkySkimmer
Copy link
Contributor Author

I tried adding a badge, currently it shows as failing, let's wait for the latest build to finish.

F438

@Zimmi48
Copy link
Member
Zimmi48 commented Dec 14, 2017

I'm not fond of the bullet points for the badges. Everything on the same line would look better and is more usual.

Copy link
Member
@maximedenes maximedenes left a comment

Choose a reason for hiding this comment

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

This looks very good!

@maximedenes
Copy link
Member

Improving the CI situation is a priority, so I'll merge this and activate Circle CI. Refining which jobs stay on Travis will be done in a second step.

@maximedenes maximedenes merged commit 653df5a into rocq-prover:master Dec 19, 2017
maximedenes added a commit that referenced this pull request Dec 19, 2017
@Zimmi48 Zimmi48 added this to the 8.7.2 milestone Dec 19, 2017
ejgallego pushed a commit to ejgallego/coq that referenced this pull request Dec 19, 2017
@Zimmi48
Copy link
Member
Zimmi48 commented Dec 20, 2017

I'm kind of wondering how this got merged without any testing on Travis, while it was clearly likely to affect Travis.

Also, when I said "remove the badge", did you understand that my main reason for asking this was that the badge points to @SkySkimmer's fork? I guess you didn't and I should have made myself clearer, but this was difficult to shout over the room while a talk was starting.

@ejgallego
Copy link
Member

The new badges look very ugly and out of alignment

@maximedenes
Copy link
Member

I'm kind of wondering how this got merged without any testing on Travis, while it was clearly likely to affect Travis.

Yep, I didn't realize, sorry about that.

Also, when I said "remove the badge", did you understand that my main reason for asking this was that the badge points to @SkySkimmer's fork? I guess you didn't and I should have made myself clearer, but this was difficult to shout over the room while a talk was starting.

Oh! Indeed I thought you meant just a cosmetic thing because you had written:

I'm not fond of the bullet points for the badges. Everything on the same line would look better and is more usual.

Anyway, let's quickly solve these problems and the CI situation should improve.

@SkySkimmer SkySkimmer deleted the circle-ci branch December 22, 2017 15:08
Zimmi48 added a commit to Zimmi48/coq that referenced this pull request Dec 22, 2017
Zimmi48 added a commit to Zimmi48/coq that referenced this pull request Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature request or implementation. kind: infrastructure CI, build tools, development tools.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0