Skip to content

Use (format-dune-file) instead of dune format-dune-file #1338

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

Merged
merged 2 commits into from
Jul 4, 2025

Conversation

nojb
Copy link
Contributor

@nojb nojb commented Apr 4, 2025

Hello,

You are currently calling dune format-dune-file from one of your tests. This command does not honour the Dune lang version used in the project which means that your test will break whenever we change the formatting style in Dune.

In Dune 3.18 we introduced a built-in action (format-dune-file <src> <dst>) precisely for this purpose. This action honours the current Dune lang version and does not require shelling out to an external process.

Note that dune format-dune-file may evolve in ways that may not be backwards compatible in the near future (as the CLI interface does not have any backwards compatibility or versioning guarantees), so we are encouraging users of this command to switch to the action instead.

See ocaml/dune#10892 (comment) for some of the context.

@maiste
Copy link

maiste commented Apr 8, 2025

Thanks @nojb!
I was about to do it today but you were clearly faster 😄

One question I have regarding this: shouldn't it add a dependency on dune >= 3.18 to make sure it supports the functionality?

@nojb
Copy link
Contributor Author

nojb commented Apr 8, 2025

Thanks @nojb! I was about to do it today but you were clearly faster 😄

One question I have regarding this: shouldn't it add a dependency on dune >= 3.18 to make sure it supports the functionality?

Yes, indeed. I pushed a fix (I think ; I am not very familiar with OPAM).

@panglesd panglesd added the no changelog This pull request does not need a changelog entry label May 14, 2025
Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

Thanks @nojb! That looks good.

The benchmark CI failure seems to be related to ocaml/dune#11045.
It does not happen on ocaml-ci, which install deps and then call dune. On the contrary, the benchmark ci seems to pin everything and install them with opam, which calls dune subst and that is what is breaking.

Maybe that would be better to wait for the ocaml/dune#11045 fix to be released before raising dune lang if that is what triggers the bug?

@maiste
Copy link

maiste commented May 26, 2025

The new fix was merged. It should be available in 3.20.

@panglesd
Copy link
Collaborator

panglesd commented Jun 3, 2025

With the newly released dune, the benchmark CI still fails :(

#10 217.7 #=== ERROR while compiling odoc-parser.dev ====================================#
#10 217.7 # context              2.1.6 | linux/x86_64 | ocaml-base-compiler.5.0.0 | pinned(file:///home/opam/bench-dir)
#10 217.7 # path                 ~/.opam/5.0/.opam-switch/build/odoc-parser.dev
#10 217.7 # command              ~/.opam/5.0/bin/dune subst
#10 217.7 # exit-code            1
#10 217.7 # env-file             ~/.opam/log/odoc-parser-63-674de6.env
#10 217.7 # output-file          ~/.opam/log/odoc-parser-63-674de6.out
#10 217.7 ### output ###
#10 217.7 # File "odoc-bench.opam", line 1, characters 0-0:
#10 217.7 # Error: repository does not contain any version information

@maiste
Copy link

maiste commented Jun 3, 2025

I'm not sure it contains the patch from: ocaml/dune@ad06a13.

@panglesd
Copy link
Collaborator

panglesd commented Jun 4, 2025

I think it does include it. The old error message was:

File "odoc-bench.opam", line 1, characters 10-25:
1 | # This file is generated by dune, edit dune-project instead
              ^^^^^^^^^^^^^^^
Error: repository does not contain any version information

and it is now:

File "odoc-bench.opam", line 1, characters 0-0:
Error: repository does not contain any version information

I guess the error is either on the benchmark CI or on dune's side, so it would be a pity to block this PR on it. I don't know how much the benchmark CI is used, at least I struggle with its UI...

@shonfeder
Copy link

Hi, all 👋 !

I've just learned that we are blocking new release on dune until this can be resolved, and that some are waiting on fixes that are ready to be released there.

IIUC, from the issue here, the problem in the CI does not seem to be fixable from this PR. So could we please move forward with merging this fix, and create a followup issue wherever needed to fix the benchmarking issue?

@jonludlam
Copy link
Member

OK, we'll assume a fix is on its way and merge this anyway.

@jonludlam jonludlam force-pushed the use-format-dune-file-action branch from 913e49e to 69881ee Compare July 4, 2025 08:49
@jonludlam jonludlam merged commit 9fe50ca into ocaml:master Jul 4, 2025
9 of 11 checks passed
@shonfeder
Copy link

Thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants