Skip to content

Enable support for Travis CI and Appveyor#200

Merged
c-cube merged 1 commit into
c-cube:masterfrom
kit-ty-kate:ci
Feb 25, 2018
Merged

Enable support for Travis CI and Appveyor#200
c-cube merged 1 commit into
c-cube:masterfrom
kit-ty-kate:ci

Conversation

@kit-ty-kate
Copy link
Copy Markdown
Collaborator

Done using @yomimono's autoci.

@c-cube
Copy link
Copy Markdown
Owner

c-cube commented Feb 22, 2018

Well, the travis check fails… Is it normal? 😆

@yomimono
Copy link
Copy Markdown

yomimono commented Feb 22, 2018

Nope! It's newly failing in my test repository as well: https://travis-ci.org/yomimono/hello-tests/jobs/334482913#L5202 . I think this is a new problem with travis-opam version 1.2.0.

@kit-ty-kate
Copy link
Copy Markdown
Collaborator Author

For some reason some ubuntu images seem to use the system switch over a "clean" one :/
Otherwise for the versions that does have a proper switch:

#=== ERROR while installing containers.2.0 ====================================#
# opam-version         1.2.2 (aa258ecc06d3aea5a67f442a4ffd23f2a457180b)
# os                   linux
# command              jbuilder runtest -p containers -j 4
# path                 /home/opam/.opam/4.06.0/build/containers.2.0
# compiler             4.06.0
# exit-code            1
# env-file             /home/opam/.opam/4.06.0/build/containers.2.0/containers-1861-05296d.env
# stdout-file          /home/opam/.opam/4.06.0/build/containers.2.0/containers-1861-05296d.out
# stderr-file          /home/opam/.opam/4.06.0/build/containers.2.0/containers-1861-05296d.err
### stderr ###
# Error: External library "sequence" not found.
# -> required by "qtest/jbuild (context default)"
# Hint: try: jbuilder external-lib-deps --missing -p containers @runtest
=-=- Error report -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
The following actions failed
  - install containers 2.0
No changes have been performed
'opam install containers -t -v' failed.
'unset TESTS; OPAMYES=1 export OPAMYES; unset OPAMBUILDTEST; set -ue; opam install containers "-t" "-v"' exited 4. Terminating with 4

@c-cube
Copy link
Copy Markdown
Owner

c-cube commented Feb 22, 2018

Naive question: why ubuntu over alpine?

@yomimono
Copy link
Copy Markdown

For some reason some ubuntu images seem to use the system switch over a "clean" one :/

Possibly related to https://github.com/ocaml/ocaml-ci-scripts/blob/master/README-travis.md#testing-system-switches ?

Naive question: why ubuntu over alpine?

A better-populated set of depexts.

@kit-ty-kate
Copy link
Copy Markdown
Collaborator Author

This should fix the problem: ocaml/ocaml-ci-scripts#219

@c-cube
Copy link
Copy Markdown
Owner

c-cube commented Feb 22, 2018

It seems that some libraries that are test-only deps are not installed. I think the jbuild files are fine, but I'm not totally sure.

Error: External library "sequence" not found.
- -> required by "qtest/jbuild (context default)"

@kit-ty-kate
Copy link
Copy Markdown
Collaborator Author

Probably because your test-only deps are marked as optional. I'm not sure what you meant by that and I'm not sure what should be opam's behaviour either.

@yomimono
Copy link
Copy Markdown

Are the dependencies in the depopts field marked with test really optional (i.e., the tests that are run by jbuilder build @runtest can be built and run without them)? Or perhaps they're there to work around some (possibly historical) wrong behavior of the test scripts?

@c-cube
Copy link
Copy Markdown
Owner

c-cube commented Feb 22, 2018

Good call, I guess? Should I also add the optional deps as required {test} deps?

edit: to run tests, these optional deps are necessary too

@yomimono
Copy link
Copy Markdown

Should I also add the optional deps as required {test} deps? edit: to run tests, these optional deps are necessary too

In that case (if I understand correctly) they should all be in depends with a {test} tag.

If you had additional tests that are only built if some dependencies are present (like, a flow where you opportunistically used qtest and qcheck to run some tests if you saw that qcheck was installed, but otherwise didn't) then it would be appropriate to put those in depopts with {test} tagged.

@kit-ty-kate
Copy link
Copy Markdown
Collaborator Author

@yomimono if it is possible is there a problem to accept other kinds of optional dependencies here ? I'm really not sure what should be accepted (given the jbuilder circular dependencies for instance).
I suppose adding a least doc & test with opam 2.0 seems fine and with opam 1.2 given the lack of post attribute they would be broken anyway. Maybe, I don't know :/

@yomimono
Copy link
Copy Markdown

yomimono commented Feb 22, 2018

Yes, but that won't help - including the packages in DEPOPTS just makes sure they're present for the test-with-depopts section of the test run, not overall, so if they're always required and the installer script doesn't otherwise arrange for them to be present, the initial test build will fail. Still, it's sensible to include the {test} tagged depopts in that variable at least, so I'll push a change including them (edited: autoci now includes them in DEPOPTS.)

@c-cube
Copy link
Copy Markdown
Owner

c-cube commented Feb 23, 2018

@jpdeplaix can you rebase on master? I moved test deps into depends

@kit-ty-kate
Copy link
Copy Markdown
Collaborator Author

There is a failure on 4.04.2 only for some reason:

#=== ERROR while installing containers.2.0 ====================================#
# opam-version         1.2.2 (aa258ecc06d3aea5a67f442a4ffd23f2a457180b)
# os                   linux
# command              jbuilder runtest -p containers -j 4
# path                 /home/opam/.opam/4.04.2/build/containers.2.0
# compiler             4.04.2
# exit-code            1
# env-file             /home/opam/.opam/4.04.2/build/containers.2.0/containers-4674-05296d.env
# stdout-file          /home/opam/.opam/4.04.2/build/containers.2.0/containers-4674-05296d.out
# stderr-file          /home/opam/.opam/4.04.2/build/containers.2.0/containers-4674-05296d.err
### stderr ###
# [...]
#    run_qtest alias qtest/runtest (exit 1)
# (cd _build/default/qtest && ./run_qtest.exe)
random seed: 305475584
\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
# Failure: >split_list>../src/core/CCRandom.ml:229
# 
# ../src/core/CCRandom.ml:229:  run ~st:(QCheck_runner.random_state()) ( uniformity_test 50_000 (split_list 10 ~len:3) )
# ///////////////////////////////////////////////////////////////////////////////
# Ran: 1061 tests in: 12.87 seconds.                                        
# FAILURE

@c-cube
Copy link
Copy Markdown
Owner

c-cube commented Feb 25, 2018

That test can fail from time to time (with a low probability), I assume that's not the problem. Thanks for the changes! 😄

@c-cube c-cube merged commit a0d0cf9 into c-cube:master Feb 25, 2018
@kit-ty-kate kit-ty-kate deleted the ci branch February 25, 2018 21:46
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.

3 participants