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

Warn when incompatible dune version detected in dune-project #3705

Closed

Conversation

ulrikstrid
Copy link

This is quite hacky but something to get the ball rolling.

Fixes #3700

@ulrikstrid ulrikstrid force-pushed the 3700-warn-on-bad-dune-version branch from e8abc2e to cb7cf6c Compare August 13, 2020 20:02
src/dune/opam_create.ml Outdated Show resolved Hide resolved
@ulrikstrid ulrikstrid force-pushed the 3700-warn-on-bad-dune-version branch from cb7cf6c to c47978e Compare August 14, 2020 06:44
@ulrikstrid
Copy link
Author

I seem to have had the wrong ocamlformat version and then it didn't format. I formatted the code and force pushed now.

@rgrinberg
Copy link
Member

LGTM. Can you add a test case for this behavior and make sure it only fires for dune 2.8 and above? We try not to alter dune's behavior for a fixed version.

@kit-ty-kate do you agree with the behavior proposed here?

->
User_warning.emit
~hints:
[ Pp.textf "Set dune constraint to >= %s"
Copy link
Member

Choose a reason for hiding this comment

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

note that currently if (lang dune ...) is set to 2.6 or above, and the constraint is set, dune won't do anything clever and will just add two constraints (i.e. will become "dune" {>= "2.6" & >= "2.6"}). It's not wrong but visually annoying imo, so maybe we could add something for that or simply say "remove the constraint if (dune lang ...) is set to 2.6 or above". Idk. I don't mind too much either way.

@kit-ty-kate
Copy link
Member

make sure it only fires for dune 2.8 and above? We try not to alter dune's behavior for a fixed version.

Does this apply even if it's just a warning?

@rgrinberg
Copy link
Member

Does this apply even if it's just a warning?

That's what we've done when introducing other warnings. For example, the merlin warning.

@kit-ty-kate
Copy link
Member

the thing is that otherwise the warning is rather useless as from (lang dune 2.6) the issue has been fixed anyway by the automatic addition of >= {dune_version}

@rgrinberg
Copy link
Member

the thing is that otherwise the warning is rather useless as from (lang dune 2.6) the issue has been fixed anyway by the automatic addition of >= {dune_version}

Even if the user already set a constraint manually?

@kit-ty-kate
Copy link
Member

yeah :/

| Some c -> And [ constraint_; c ] )

@rgrinberg
Copy link
Member

This behavior seems a bit weird.

For example, if I add a >= 2.3.0 constraint manually on dune, I generate the following opam file:

+  "dune" {>= "2.7" & >= "2.3.0"}

I wonder if would be better to just have a warning rather than use the existing constraint.

I suppose we could add the warning retroactively, but I think we need to make sure it doesn't fire up in the vendored directory.

Signed-off-by: Ulrik <ulrik.strid@outlook.com>
Signed-off-by: Ulrik <ulrik.strid@outlook.com>
Signed-off-by: Ulrik <ulrik.strid@outlook.com>
@ulrikstrid ulrikstrid force-pushed the 3700-warn-on-bad-dune-version branch from acb74ff to b590efc Compare August 17, 2020 06:00
@ulrikstrid
Copy link
Author

Added tests but they will fail until 2.8 is a valid language version.

@ulrikstrid
Copy link
Author

@rgrinberg what can I do to move this PR forward?

@rgrinberg
Copy link
Member

Isn't the issue solved by upgrading your dune language version already? Then the up to date constraint is always inserted.

Base automatically changed from master to main January 14, 2021 17:08
@rgrinberg rgrinberg closed this Feb 28, 2023
@ulrikstrid ulrikstrid deleted the 3700-warn-on-bad-dune-version branch February 28, 2023 18:28
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.

OPAM file generation doesn't respect (lang dune)
3 participants