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

4 packages from diskuv/dkml-install-api at 0.5.2 #24852

Conversation

jonahbeckford
Copy link
Contributor

This pull-request concerns:
-dkml-install.0.5.2: API and registry for DkML installation components
-dkml-install-installer.0.5.2: Build tools for DkML installers
-dkml-install-runner.0.5.2: Runner executable for DkML installation
-dkml-package-console.0.5.2: Console setup and uninstall executables for DkML installation



🐫 Pull-request generated by opam-publish v2.1.0

Copy link
Collaborator

@haochenx haochenx left a comment

Choose a reason for hiding this comment

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

CI had a few cancelled jobs which I restarted.

I'm not entirely sure the inline comment needs to be addressed, and if it does need address, why its not caught by linter on this PR.

@raphael-proust @mseri please take a look and advice

homepage: "https://github.com/diskuv/dkml-install-api"
bug-reports: "https://github.com/diskuv/dkml-install-api/issues"
depends: [
"dune" {>= "2.9"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likely missing

Suggested change
"dune" {>= "2.9"}
"ocaml" {>= "4.10"}
"dune" {>= "2.9"}

as per #24577 pointed out. Maybe it's better to upstream this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is ocaml.4.10+ necessary?

Copy link
Collaborator

@haochenx haochenx Nov 27, 2023

Choose a reason for hiding this comment

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

I suggested ocaml.4.10+ only because it is so in dkml-install-runner.0.5.1. I don't have a strong opinion here.

It seems that the lower bound wasn't picked by you. As the CI is currently succeeding, we probably do not need this dependency. Yet there is a policy to have a dependency to ocaml, so it would be nice to add it like:

Suggested change
"dune" {>= "2.9"}
"ocaml"
"dune" {>= "2.9"}

@jonahbeckford
Copy link
Contributor Author

Clicked the Rebuild failed button because the failing jobs had "Cancelled" due to 120 minute timeout. Nothing to do with the PR; sounds like the CI machines were stalled for other reasons.

@mseri
Copy link
Member

mseri commented Nov 27, 2023

We did add the ocaml dependency because it was caught by the linter, but it was not essential since dkml-install already comes with an ocaml lower bound. The linter is no longer complaining about that, so I suggest we merge

@mseri mseri merged commit 0a297fc into ocaml:master Nov 27, 2023
2 checks passed
@mseri
Copy link
Member

mseri commented Nov 27, 2023

Thanks to both of you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants