Skip to content

pkg: recognize default dune build commands #13592

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.

Sign up for GitHub

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

Open
gridbugs wants to merge 1 commit into
base: main
Choose a base branch
from
Open

pkg: recognize default dune build commands #13592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 45 additions & 3 deletions src/dune_pkg/resolved_package.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,48 @@ let opam_file = function
| Rest t -> t.opam_file
;;

let commands_are_dune_default ~build ~install =
let with_test_var = OpamVariable.of_string "with-test" in
let with_doc_var = OpamVariable.of_string "with-doc" in
let dev_var = OpamVariable.of_string "dev" in
let is_default_subst_command = function
| ( [ (OpamTypes.CString "dune", None); (CString "subst", None) ]
, Some (OpamTypes.FIdent ([], dev_var_, None)) ) ->
OpamVariable.equal dev_var_ dev_var
| _ -> false
in
let is_default_build_command = function
Copy link
Copy Markdown
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV Feb 10, 2026

Choose a reason for hiding this comment

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

I think this is a bit too constraining. e.g. imagine a package removes @doc or sets -j 1 then it doesn't trigger anymore.

I would rather suggest parsing out whether there's a call to dune build (and maybe potentially some infos about the targets but it doesn't have to be in this PR).

Copy link
Copy Markdown
Collaborator

@Alizter Alizter Feb 10, 2026

Choose a reason for hiding this comment

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

It seems to me that the point of this PR is not to capture every dune invocation but only the default one.

Copy link
Copy Markdown
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV Feb 10, 2026

Choose a reason for hiding this comment

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

Yes, my point is that this is very fragile and the PR would be a lot more valuable if it detected more cases. Otherwise the semantics are pretty hard to understand from a users perspective: a package can't just call dune build to be recognized as building with Dune, but it would need to exactly mirror the exact same, fairly complex and potentially over time changing incantation that dune generates (note that Dune used to generate different lines before). Also note that not all users use Dune to generate their opam files, so they might call dune in other ways, which then would not be recognized.

In my opinion the starting point should be a specification of what we consider a "dune build", because there's a lot of questions from the top of my head that would need answering:

  1. Is dune subst required? Is it optional? Is it forbidden?
  2. Do we detect builds that call dune but not in the OPAM file? How hard do we try to detect whether a package builds with dune? Do we inspect just the OPAM metadata? Do we inspect the tarballs?
  3. Which dune commands are required for a build to be considered a dune build? dune build for sure, but which targets? Is @install required? Is @doc {with-doc} required, even if the package has no docs?
  4. Do additional commands have an influence on whether a package is considered a dune build? Is calling ./configure before dune build still a dune build? Is cp _build/default/install/bin/foo ${bin}/foo after dune build still a valid dune build? Are there commands that are ok or commands that are forbidden?

I think the answers to questions like these are important because that helps users to understand e.g. why their build is not a dune build and what they need to change to make it a dune build.

Copy link
Copy Markdown
Collaborator Author

@gridbugs gridbugs Feb 11, 2026

Choose a reason for hiding this comment

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

The goal is to detect packages that could be built without shelling out, so we need to make sure that the build command would have had the same meaning as if Dune was to build the package internally. If a package builds with dune build -j1 for example, it's possible that it's not safe to build the package in parallel, so maybe Dune should still shell out in that case. Same for packages that run a configure script before building, or invoke dune indirectly (like from a Makefile).

Perhaps a better alternative would be for dune to add a filed to opam files it generates indicating that the package can be built with dune's default command rather than having Dune attempt to parse the command and decide whether it's the default command?

| ( [ (OpamTypes.CString "dune", None)
; (CString "build", None)
; (CString "-p", None)
; (CIdent "name", None)
; (CString "-j", None)
; (CIdent "jobs", None)
; (CString "@install", None)
; (CString "@runtest", Some (OpamTypes.FIdent ([], with_test_var_, None)))
; (CString "@doc", Some (OpamTypes.FIdent ([], with_doc_var_, None)))
]
, None ) ->
OpamVariable.equal with_test_var_ with_test_var
&& OpamVariable.equal with_doc_var_ with_doc_var
| _ -> false
in
(* Accept packages with no install command, and whose build commands look
like the ones inserted by Dune when it generates opam files. *)
match install with
| _ :: _ -> false
| [] ->
(match build with
| [ build_command ] -> is_default_build_command build_command
| [ subst_command; build_command ] ->
is_default_subst_command subst_command && is_default_build_command build_command
| _ -> false)
;;

let opam_file_has_default_dune_commands (opam_file : OpamFile.OPAM.t) =
commands_are_dune_default ~build:opam_file.build ~install:opam_file.install
;;

let with_opam_file opam_file = function
| Dune ->
Code_error.raise
Expand All @@ -54,7 +96,7 @@ let extra_files = function
let git_repo package (loc, opam_file) rev ~files_dir ~url =
let opam_file = Opam_file.opam_file_with ~package ~url opam_file in
Rest
{ dune_build = false
{ dune_build = opam_file_has_default_dune_commands opam_file
; loc
; package
; opam_file
Expand All @@ -66,7 +108,7 @@ let local_fs package (loc, opam_file) ~dir ~files_dir ~url =
let files_dir = Option.map files_dir ~f:(Path.append_local dir) in
let opam_file = Opam_file.opam_file_with ~package ~url opam_file in
Rest
{ dune_build = false
{ dune_build = opam_file_has_default_dune_commands opam_file
; loc
; package
; extra_files = Inside_files_dir files_dir
Expand Down Expand Up @@ -103,7 +145,7 @@ let local_package ~command_source (loc, opam_file) opam_package =
let dune_build =
match (command_source : Local_package.command_source) with
| Assume_defaults -> true
| Opam_file _ -> false
| Opam_file { build; install } -> commands_are_dune_default ~build ~install
in
let opam_file = Opam_file.opam_file_with ~package:opam_package ~url:None opam_file in
let package = OpamFile.OPAM.package opam_file in
Expand Down
52 changes: 52 additions & 0 deletions test/blackbox-tests/test-cases/pkg/lockfiles-dune-keyword.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
When Dune generates a lockfile for a package, if the package had the default
Dune build command, use the "dune" keyword in the lockfile rather than copying
its build command.

$ mkrepo
$ add_mock_repo_if_needed

$ mkpkg foo <<EOF
> build: [
> ["dune" "subst"] {dev}
> [
> "dune"
> "build"
> "-p"
> name
> "-j"
> jobs
> "@install"
> "@runtest" {with-test}
> "@doc" {with-doc}
> ]
> ]
> EOF

$ cat > dune-project <<EOF
> (lang dune 3.21)
> (package
> (name bar)
> (depends foo))
> EOF

$ cat > bar.ml <<EOF
> let () = print_endline Foo.foo
> EOF

$ cat > dune <<EOF
> (executable
> (public_name bar)
> (libraries foo))
> EOF

Lock, build, and run the executable in the project:
$ dune_pkg_lock_normalized
Solution for dune.lock:
- foo.0.0.1

The lockfile contains the "dune" keyword rather than the build command:
$ cat dune.lock/foo.0.0.1.pkg
(version 0.0.1)

(build
(all_platforms ((dune))))
Loading