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 dependencies to fix opam-dune-lint #50

Merged
merged 2 commits into from
Jan 15, 2024
Merged

Conversation

mbarbin
Copy link
Contributor

@mbarbin mbarbin commented Jan 6, 2024

So that it is easier to avoid duplicating entries, use a canonical order to sort the libraries.

The canonical order I used here was to put ocaml first, and then the rest of dependencies alphabetically.

Note that this PR is based on top of #49, so it is currently marked as draft.

@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 6, 2024

I noticed the following item didn't pass in #48 in the list of checks:

EXPERIMENTAL: (lint-opam) (failure: Lint failed for grpc-async.opam)

The motivation for this PR is to fix that check. Rebasing the PR across this change will also allows to verify that #48 doesn't introduce breaking changes for opam-dune-lint.

@mbarbin mbarbin marked this pull request as ready for review January 8, 2024 11:16
@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 8, 2024

This satisfies opam-dune-lint when checked locally. Before merging we also need to verify the result of the check on the ci.

$ opam-dune-lint
grpc-async.opam: OK             
grpc-bench.opam: OK
grpc-eio.opam: OK
grpc-examples.opam: OK
grpc-lwt.opam: OK
grpc.opam: OK

@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 8, 2024

  • This PR fixes the opam-dune-lint (lint-opam) check in the ci

https://ocaml.ci.dev/github/dialohq/ocaml-grpc/commit/cbac7ecaebebb366c2825fb8ba69e2a6a42e6e9b/variant/%28lint-opam%29#L808-814

/src: (run (shell "opam exec -- opam-dune-lint"))
grpc-async.opam: OK
grpc-bench.opam: OK
grpc-eio.opam: OK
grpc-examples.opam: OK
grpc-lwt.opam: OK
grpc.opam: OK

@mbarbin mbarbin changed the title add dependencies to pass [opam-dune-lint] Add dependencies to fix opam-dune-lint Jan 8, 2024
@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 8, 2024

I just noticed that a new check is failing:

EXPERIMENTAL: freebsd-5.1_opam-2.1 (failure: Failed: Build failed)

I don't have experience with ocaml-ci nor FreeBSD builds. I haven't managed to access the log for the failure (500 error page on ocaml-ci). I don't know how the changes in this PR could cause this check to fail. There's a possibility this is a spurious or flickering failure. Putting this out there for your consideration.

Note that opam-dune-lint is also experimental, but I think it is important to try and validate this check, because it may indicate missing dependencies that could cause build failure on any platform, and also because I believe there's an ongoing effort to make build lint eventually required in ocaml-ci. I don't know how important is the Freebsd check for ocaml-grpc, and if it is reasonable to prioritize opam-dune-lint over it.

This makes it is easier to avoid duplicated entries as well as making
it obvious were new dependencies need to be added, which makes it more
straight forward to write PRs that add new dependencies.

In the process I discovered that the notty dependency was duplicated
in `grpc-bench.opam`, which I fixed. This had no incidence on the
build.

The canonical order I used here was to put `ocaml` first, and then the
rest of dependencies alphabetically.
For each added dependency, I looked elsewhere in the file for
occurrences of the same library, and kept the same version bound if
there was one, and none otherwise.
@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 10, 2024

The check appears to be successful again.

https://github.com/dialohq/ocaml-grpc/pull/50/checks

✅ freebsd-5.1_opam-2.1 (passed)

If this isn't a transient issue, one potential cause could be the unintentional
initial increase of the uri dependency bound from >=4.0.0 to >= 4.4.0,
which I've since reverted. However, I'm skeptical that this was the root cause,
as I vaguely remember seeing version 4.4.0 being fetched anyways by the CI at
some point (though I can't recall the exact details, and I'm currently unable to
access the log for the check due to a 500 error).

Regardless, I think that this PR doesn't make the situation worse regarding this
check.

@quernd
Copy link
Collaborator

quernd commented Jan 12, 2024

I think this makes sense. We didn't have e.g. h2 as a dependency of grpc-eio because it depends on grpc which depends on h2 but since it also uses h2 directly it should arguably be made explicit. The canonical order is also appreciated. Thanks!

@mbarbin
Copy link
Contributor Author

mbarbin commented Jan 12, 2024

Thank you @quernd for reviewing the PR. I've observed that the CI checks have re-run and some are now showing an internal error status. I'm unsure why they re-ran as they were all passing previously. Could you possibly request a re-run? I'm hopeful this is a transient issue that will resolve itself.

@quernd
Copy link
Collaborator

quernd commented Jan 12, 2024

Thank you @quernd for reviewing the PR. I've observed that the CI checks have re-run and some are now showing an internal error status. I'm unsure why they re-ran as they were all passing previously. Could you possibly request a re-run? I'm hopeful this is a transient issue that will resolve itself.

I triggered a re-run but I'm seeing many "Internal Error" failures like this:

# Run eval $(opam env) to update the current shell environment
2024-01-12 15:30.25 ---> saved as "325465377ee1d011b19968dfebc53b21e0af2792e4207fc22a135c090b7376b6"


/src: (copy (src .) (dst /src))
Uncaught exception: Failure("\"rm\" \"-rf\" \"/var/cache/obuilder/merged/ece1ab8e3d253142eeecd688f1483cca1180f7e651edb0539f927b21490b21f9\" \"/var/cache/obuilder/work/ece1ab8e3d253142eeecd688f1483cca1180f7e651edb0539f927b21490b21f9\" failed with exit status 1")
2024-01-12 15:30.26: Job failed: Failed: Internal error

I don't know how rm -rf can fail with exit status 1, to be honest. @tmcgilchrist you understand OCaml-CI better, do you know what's going on?

@quernd
Copy link
Collaborator

quernd commented Jan 15, 2024

All green now, likely a transient issue. Thanks @mbarbin

@quernd quernd merged commit 71d1d84 into dialohq:main Jan 15, 2024
3 checks passed
@mbarbin mbarbin deleted the opam-dune-lint branch January 15, 2024 17:44
@tmcgilchrist
Copy link
Collaborator

tmcgilchrist commented Jan 15, 2024 via email

@quernd
Copy link
Collaborator

quernd commented Jan 15, 2024

It was an issue on the Mac workers with removing cached results. In future if you see issues please report them on https://github.com/ocurrent/ocaml-ci/issues

Thanks, will keep it in mind!

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