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

[new release] ocamlmig (5.2-20250202) #27353

Merged
merged 3 commits into from
Feb 5, 2025

Conversation

v-gb
Copy link
Contributor

@v-gb v-gb commented Feb 3, 2025

OCaml source code rewriting tool

CHANGES:

Correct a failure to use some opam-installed files.

CHANGES:

Correct a failure to use some opam-installed files.
"csexp"
"ppx_partial"
"ocaml" {>= "4.08"}
"alcotest" {"1" = "0" & >= "1.3.0"}
Copy link
Member

@mseri mseri Feb 4, 2025

Choose a reason for hiding this comment

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

Suggested change
"alcotest" {"1" = "0" & >= "1.3.0"}
"alcotest" {"1" = "0" & >= "1.3.0"}

Why you have these funny false bounds (also below)? If you want you could use {with-dev-setup} and make them available only when invoking opam with that flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The package vendors (a modified copy of) ocamlformat, which means the dependencies section of my opam file copies the dependencies section from ocamlformat's opam file, which is where the alcootest dependency and others come from.
In the ocamlformat package, alcootest is listed as {with-dev-setup & >= "1.3.0"}, but that doesn't make sense for my package, since my tests don't use alcootest. So I mechanically replace all the with-dev-setup that come from ocamlformat with false. But since syntactically that's not allowed (not sure if that's a shortcoming of dune-project or opam itself), instead of false, I use "1" = "0" which means the same thing :/.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't need these, why not dropping them altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The import of ocamlformat's dependencies is done by a script. Dropping the dependencies entirely would involve parsing opam dependencies, which is much more involved than a call to sed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it would involve parsing ocamlformat's dune-project, but the general point is the same.

Copy link
Member

Choose a reason for hiding this comment

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

I meant more if it was ok for me to remove them here :)

@mseri mseri added the question label Feb 4, 2025
@v-gb
Copy link
Contributor Author

v-gb commented Feb 5, 2025 via email

@mseri
Copy link
Member

mseri commented Feb 5, 2025

It does not take much, and avoid unnecessary checks in the dependency solver. I'll keep an eye out and remove it also in future releases

@mseri
Copy link
Member

mseri commented Feb 5, 2025

By the way, why not leaving it with with-dev-setup instead? Opam will practically ignore them, and they will not be installed in any switch unless you specifically use that flag

@v-gb
Copy link
Contributor Author

v-gb commented Feb 5, 2025

The ocp-indent change is not correct though.

@v-gb
Copy link
Contributor Author

v-gb commented Feb 5, 2025

By the way, why not leaving it with with-dev-setup instead? Opam will practically ignore them, and they will not be installed in any switch unless you specifically use that flag

I'll likely want to use with-dev-setup for my own purpose.
If you'd be fine if the opam file said with-dev-setup, why would a static condition be more expensive?

@mseri
Copy link
Member

mseri commented Feb 5, 2025

I think the flags are filtered a priori. But that is not really my point, it was more to cleanup the dependencies and to have filters that are easier to read. I am going to open an issue in dune to support :false.

For ocp-indent you could just have the bound without "false" = "false" though.

@mseri mseri merged commit 053dec3 into ocaml:master Feb 5, 2025
3 checks passed
@v-gb v-gb deleted the release-ocamlmig-5.2-20250202 branch February 5, 2025 11:37
@mseri
Copy link
Member

mseri commented Feb 5, 2025

I just went to check, if you use :false in your dune-project file, it will appear as false in the constraint in the generated file without the need of workarounds. this works fine in recent dune releases

@v-gb
Copy link
Contributor Author

v-gb commented Feb 5, 2025

Thanks, I'll try that!

@v-gb
Copy link
Contributor Author

v-gb commented Feb 5, 2025

So I tried this, and even though the opam files look fine, dune itself doesn't understand what's going on, which causes dune pkg lock to drop the dependency on ocp-indent (I assume because dune treats the variable "true" as unset, hence false), which breaks the build.

@mseri
Copy link
Member

mseri commented Feb 5, 2025

For the true case, the true should be completely unnecessary though, you only need “version bound or (false and version bound)”. Am I wrong?

@mseri
Copy link
Member

mseri commented Feb 5, 2025

In any case true or false should be correctly interpreted, if that is not the case we should raise an issue on the dune repository

@v-gb
Copy link
Contributor Author

v-gb commented Feb 5, 2025

For the true case, the true should be completely unnecessary though, you only need “version bound or (false and version bound)”. Am I wrong?

Right, I suppose in this case, I can just delete the true case.

In any case true or false should be correctly interpreted, if that is not the case we should raise an issue on the dune repository

ocaml/dune#11447

@mseri
Copy link
Member

mseri commented Feb 5, 2025

Thanks!🙏

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.

2 participants