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

Dedicated support for duniverse builds #142

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Conversation

avsm
Copy link
Member

@avsm avsm commented Apr 15, 2020

  • Add a duniverse builder which does incremental builds without
    requiring an opam environment. It does this by building all
    the required platform binaries (dune, odoc, ocamlformat and
    duniverse) in a multistage build and copying them into the
    main package build.

  • Extract only the OCaml versions necessary from the lockfile,
    and use that for duniverse builds. No solver required to infer
    this as it is explicit.

  • Detect duniverses by the presence of a dune-get file, and not
    the duniverse/ directory. If the sources aren't checked in, then
    duniverse will pull the locked sources as part of the build.
    This is all incrementally cached correctly.

  • Remove the duniverse support from opam builder, so that can
    be simplified back to its original state.

There are a few TODOs in here. @talex5 indicated that he could try to get rid of the bind in the analysis phase, so feel free to push to my branch with that...

Two good test repositories with the correct dune-get files are duniverse and realworldocaml/book.

@talex5
Copy link
Contributor

talex5 commented Apr 16, 2020

I've pushed some changes to my branch at https://github.com/talex5/ocaml-ci/tree/duniverse:

  • Rebased onto master and removed binds.
  • Errors parsing dune-get are now fatal.
  • Get dune-get from the project, not the cwd.
  • Skip reading dune-get if not a dune project.
  • Removed maxdepth change. Duniverse doesn't seem to care about opam files anyway, except for detecting ocamlformat in the unit-tests, which were previously broken.
  • Just split the Dockerfile generation from opam_build, not the pipeline.
  • Removed Dockerfile cache. Shouldn't be needed now with incremental evaluation.

I squashed the changes because it's more useful to see the diff with master than with the previous commit (e.g. there are fewer changes to pipeline.ml now).

A test realworldocaml build failed, but it doesn't look like a CI problem, e.g.

--- a/book/files-modules-and-programs/README.md
+++ b/book/files-modules-and-programs/README.md.corrected
@@ -903,9 +903,9 @@ Error: The implementation counter.ml
          type median = Median of string | Before_and_after of string * string
        is not included in
          type median = Before_and_after of string * string | Median of string
+       Constructors number 1 have different names, Median and Before_and_after.
        File "counter.mli", lines 21-22, characters 0-32: Expected declaration
        File "counter.ml", lines 18-19, characters 0-51: Actual declaration
-       Fields number 1 have different names, Median and Before_and_after.

@avsm
Copy link
Member Author

avsm commented Apr 16, 2020

That was a real bug; merged realworldocaml/book#3300 to fix it (the dune-get file was out of sync in the main rwo repo). i've force pushed your branch to this PR.

@avsm
Copy link
Member Author

avsm commented Apr 16, 2020

Removed maxdepth change. Duniverse doesn't seem to care about opam files anyway, except for detecting ocamlformat in the unit-tests, which were previously broken

Why undo this? For opam projects, only the toplevel .opam files should matter. It was only for duniverse projects that we previously needed to scan lower ones (to get depexts). For opam projects, using the toplevel opam files only matches the behaviour of opam itself.

@talex5
Copy link
Contributor

talex5 commented Apr 16, 2020

opam projects with vendored dependencies (including ocaml-ci itself) need to scan for opam files in submodules.

~repo:"ocaml/odoc" ~tag:"1.5.0"
~bins:["bin/odoc", "/usr/bin/odoc"] "odoc" in
let ocamlformat = install_bin ~compiler
~repo:"ocaml-ppx/ocamlformat" ~tag:"0.14.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, this is probably buggy. We need to install the precise ocamlformat version required by the project, and that is found in the analysis phase.

@avsm
Copy link
Member Author

avsm commented Apr 16, 2020

Ah I see. Something of a hybrid opam/duniverse workflow there.

- Add a duniverse builder which does incremental builds without
  requiring an opam environment.  It does this by building all
  the required platform binaries (dune, odoc, ocamlformat and
  duniverse) in a multistage build and copying them into the
  main package build.

- Extract only the OCaml versions necessary from the lockfile,
  and use that for duniverse builds. No solver required to infer
  this as it is explicit.

- Remove the duniverse support from opam builder, so that can
  be simplified back to its original state.

Some changes from talex5:

- Rebased onto master and removed binds.
- Errors parsing dune-get are now fatal.
- Get dune-get from the project, not the cwd.
- Skip reading `dune-get` if not a dune project.
- Removed maxdepth change. Duniverse doesn't seem to care about opam
  files anyway, except for detecting ocamlformat in the unit-tests,
  which were previously broken.
- Just split the Dockerfile generation from `opam_build`, not the
  pipeline.
- Removed Dockerfile cache. Shouldn't be needed now with incremental
  evaluation.
@avsm
Copy link
Member Author

avsm commented Apr 16, 2020

I've pushed an ocamlformat sync, so this all passes CI now. I can refine the ocamlformat/duniverse semantics in a future PR. For now merging/deploying this would be useful to let the live CI's on realworldocaml and duniverse start passing again :-)

@talex5
Copy link
Contributor

talex5 commented Apr 16, 2020

I've deployed it for testing. There was a slight problem where duniverse projects with no compilers would report failure to GitHub, but not show any failure when you went to the page (because the analysis had passed and there were no failed jobs). I've made the analysis phase fail in that case.

@avsm
Copy link
Member Author

avsm commented Apr 16, 2020

Thanks, that sounds right. I'll work on refining the handling of ocamlformat/odoc/dune versioning constraints in dune-get/duniverse next.

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.

2 participants