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

Add a stage to lint documentation errors #105

Merged
merged 1 commit into from
Apr 23, 2020

Conversation

craigfe
Copy link
Member

@craigfe craigfe commented Feb 12, 2020

This resolves #94.

This stage follows the same structure as the existing lint step, but shares some logic with the Opam build steps in order to install the appropriate dependencies before building the documentation.

@craigfe craigfe requested a review from talex5 February 12, 2020 16:55
lib/lint.ml Outdated Show resolved Hide resolved
@craigfe craigfe force-pushed the doc-linting branch 2 times, most recently from fd1281d to 765c2a6 Compare February 14, 2020 18:15
@craigfe
Copy link
Member Author

craigfe commented Feb 14, 2020

Rebased over #108 to get the CI working.

@craigfe
Copy link
Member Author

craigfe commented Mar 11, 2020

Rebased again. @talex5 / @avsm, can I get another review?

@talex5
Copy link
Contributor

talex5 commented Mar 11, 2020

CI is failing...

@craigfe
Copy link
Member Author

craigfe commented Mar 11, 2020

Thanks; bad rebase. Should be fixed now.

service/pipeline.ml Outdated Show resolved Hide resolved
lib/lint.ml Outdated Show resolved Hide resolved
lib/opam_build.ml Outdated Show resolved Hide resolved
@talex5
Copy link
Contributor

talex5 commented Mar 12, 2020

This failed on the first two packages I tried it on, e.g.

#10 [6/8] RUN opam depext --update opam-zi.dev
#10 5.608 # Detecting depexts using vars: arch=x86_64, os=linux, os-distribution=alpine, os-family=alpine
#10 8.503 [ERROR] No solution for opam-zi.dev: Your request can't be satisfied:
#10 8.503           - unknown package

How are you testing it? I'm using ocaml-ci-local.

I think we should be using the same code to generate the Dockerfile installation lines for the lint stages as for the main build stages.

Is odoc just a binary? Perhaps we could build that in a separate Docker build stage and then COPY --from=odoc .... Then we could cache both the building of the project and the building of odoc.

@craigfe
Copy link
Member Author

craigfe commented Apr 20, 2020

I've rebased and changed the approach slightly. The logic for installing Opam dependencies of the project is now properly shared with opam_build.ml by exposing some more logic inside that module.

Tested to be working on Alcotest, Irmin and OCurrent in ocaml-ci-local mode.

@talex5
Copy link
Contributor

talex5 commented Apr 20, 2020

I've deployed it... let's see what happens!

@avsm
Copy link
Member

avsm commented Apr 20, 2020

Odoc is just a binary, and is installed that way in the duniverse_build.ml (install_bin). @craigfe, why is the doc target disabled in the duniverse build? The intention is very much to build docs there, so that the stage can be further extended in the future to a deployment/staging branch for visual inspection.

@craigfe
Copy link
Member Author

craigfe commented Apr 20, 2020

The intention is very much to build docs there, so that the stage can be further extended in the future to a deployment/staging branch for visual inspection.

@avsm: I'm keen for that feature too, but it's not clear to me why it should apply only to Duniverse projects. Shouldn't Opam builds get a deployment/staging branch for their docs too? That's why I proposed taking it out of duniverse_build.ml: right now it seems like duplicated functionality unique to Duniverse projects.

@avsm
Copy link
Member

avsm commented Apr 21, 2020

But you've taken out the feature without adding it anywhere else for them. Remember that right now, duniverse builds are distinct from opam builds. I'm happy to refactor it when we add the staging support, but right now I'd like to ensure that it stays building correctly with duniverse projects.

@craigfe
Copy link
Member Author

craigfe commented Apr 21, 2020

This linting step should still run for Duniverse builds currently, no? These jobs are defined outside of the regular build matrix.

It seems to be running correctly on e.g. realworldocaml/book:

image

@talex5
Copy link
Contributor

talex5 commented Apr 22, 2020

For me, book is now failing:

#125 4.218 The following dependencies couldn't be met:
#125 4.218   - rwo -> ocaml >= 4.10.0
#125 4.218       base of this switch (use `--unlock-base' to force)

https://ci.ocamllabs.io:8100/job/2020-04-21/184252-docker-build-be7e04

@avsm
Copy link
Member

avsm commented Apr 22, 2020

Right, that'll fail because the duniverse build is different from the opam build. The doc linting step is trying to copy all the opam files, and to pin them, and then run depexts, and then install them, etc. The duniverse build doesn't install opam deps, and uses the locally vendored copies instead with a single dune build.

There's a material difference in how the opam and the duniverse builds work. I think in the interests of pushing this PR forward, it should leave them distinct, and we can work on converging them in a next step.

@craigfe
Copy link
Member Author

craigfe commented Apr 22, 2020

Linting steps are now disabled for Duniverse projects, and I've reverted the change to not build docs during the build stages of Duniverse project.

The current solution uses a bind to conditionally execute linting according to the results of the analysis step.

@craigfe craigfe force-pushed the doc-linting branch 2 times, most recently from 2acddb5 to 6658187 Compare April 22, 2020 14:54
@talex5 talex5 merged commit aa02f37 into ocurrent:master Apr 23, 2020
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.

Lint documentation comments in the CI
4 participants