ocurrent / ocaml-ci

A CI for OCaml projects
https://ocaml.ci.dev
111 stars 75 forks source link

Fix ocamlformat analysis limiting the other jobs #907

Closed moyodiallo closed 8 months ago

moyodiallo commented 9 months ago

Attempt to fix #893, instead of failing on the analysis, the fmt job is spawned and the error is notified in it (suggestion from @Julow).

benmandrew commented 9 months ago

This works if the .ocamlformat file has no version= line 👍.

However, if the version line contains an incorrect version — such as version=0.99.1 — then the (lint-fmt) job is not spawned as no analysis solution is found. @moyodiallo do you think it'd be good to also have this case propagated as an error to a (lint-fmt) job? We'd have to be careful to do this only when the .ocamlformat file is present, so that we don't fail CI for projects without formatting.

moyodiallo commented 9 months ago

However, if the version line contains an incorrect version — such as version=0.99.1 — then the (lint-fmt) job is not spawned as no analysis solution is found. @moyodiallo do you think it'd be good to also have this case propagated as an error to a (lint-fmt) job? We'd have to be careful to do this only when the .ocamlformat file is present, so that we don't fail CI for projects without formatting.

Yeah I think you're right it is logic, when we have .ocamlformat file present in a project, the purpose is to format the project. And also (lint-fmt) job is always spawned.

moyodiallo commented 8 months ago

@benmandrew Is this OK to your suggestion ?

benmandrew commented 8 months ago

I'm not sure if the behaviour's changed or I missed it before, but if the .ocamlformat file doesn't have a version= line then the job fails in the extract stage with:

2023-11-29 13:22.17: Exec: "git" "ls-files" "**/.ocamlformat-enable" 
                           ".ocamlformat-enable"
2023-11-29 13:22.17: Job failed: Missing 'version=' line in .ocamlformat
Screenshot 2023-12-11 at 16 36 42

My test repo/branch is https://github.com/benmandrew/ci-test-repo/tree/ocamlformat-no-version.

moyodiallo commented 8 months ago

I think you're using the master branch, not the PR one.

Extract failing on this error is fixed on this PR.

benmandrew commented 8 months ago

My mistake, I was looking at an old job! Feel free to merge.