Skip to content

Conversation

@gridbugs
Copy link
Collaborator

This fixes a bug in computing dependencies of a dev tool package when using dune package management. It only affected the utop dev tool. This change also adds an integration test for the utop dev tool. The test is a github action workflow rather than a cram test so that the real utop package can be tested, since the utop rules have some logic which is specific to the utop package and thus is non-trivial to accurately test with a mock utop package as we do with other dev tool tests.

@gridbugs gridbugs requested review from Alizter and Sudha247 November 19, 2025 03:09
@gridbugs gridbugs added the bug label Nov 19, 2025
(* Disallow sharing so that the only packages in the DB are the ones from
the universe's respective lock directory. *)
DB.of_ctx ctx ~allow_sharing:false
| Dev_tool tool -> DB.of_dev_tool tool >>| fst
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change looks correct to me. However, how does this affect only utop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function here (all_deps) is used when communicating dependencies outside of Pkg_rules. Mechanisms for traversing the dependency closure entirely within Pkg_rules are unchanged. However building a utop top level with access to the code in the current project requires an OCAMLPATH with all the dependencies of utop, and the function which computes this (Pkg_rules.dev_tool_ocamlpath) finds dependencies using all_deps.

nix-${{ runner.os }}-${{ github.job }}-${{ hashFiles('**/*.nix', '**/flake.lock') }}
restore-prefixes-first-match: nix-${{ runner.os }}-${{ github.job }}-
gc-max-store-size-linux: 2G
- name: Set up a project and build it with alice in a nix shell
Copy link
Member

Choose a reason for hiding this comment

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

This name doesn't seem accurate to the step, also at least this part is wrong :)

Suggested change
- name: Set up a project and build it with alice in a nix shell
- name: Set up a project and build it with dune in a nix shell

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha woops! Fixed.

@Alizter
Copy link
Collaborator

Alizter commented Nov 19, 2025

Would you mind describing the bug here and why the fix fixes it?

@gridbugs
Copy link
Collaborator Author

Building a utop top level for a project requires knowing the paths to all the libraries that the utop package depends on (e.g.

dune/src/dune_rules/utop.ml

Lines 188 to 197 in 194210a

let lib_db sctx ~dir =
let* scope = Scope.DB.find_by_dir dir in
let* lock_dir_exists = Memo.Lazy.force utop_dev_tool_lock_dir_exists in
match lock_dir_exists with
| false -> Memo.return (Scope.libs scope)
| true ->
let* ocamlpath = Memo.Lazy.force utop_ocamlpath in
Lib.DB.of_paths (Super_context.context sctx) ~paths:ocamlpath
>>| Lib.DB.with_parent ~parent:(Some (Scope.libs scope))
;;
). When building the utop top level with dune package management, the location of each of utop's dependencies is computed in Pkg_rules and exposed with the function
val dev_tool_ocamlpath : Dune_pkg.Dev_tool.t -> Path.t list Memo.t

#12526 changed the representation of dependencies in Pkg_rules allowing dev tools to share dependencies with the default context of the project. I updated Pkg_rules.all_deps (called by Pkg_rules.dev_tool_ocamlpath) to work with this change but introduced a bug where calling it on a dev tool would return the dependencies of the default context of the project rather than the dependencies of the dev tool.

This PR changes all_deps to return the dependencies of a dev tool when it's passed a dev tool rather than returning the dependencies of the project's default context in this case.

Copy link
Member

@shonfeder shonfeder left a comment

Choose a reason for hiding this comment

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

Thanks for catching the bug and fixing it!

This fixes a bug in computing dependencies of a dev tool package when
using dune package management. It only affected the utop dev tool. This
change also adds an integration test for the utop dev tool. The test is
a github action workflow rather than a cram test so that the real utop
package can be tested, since the utop rules have some logic which is
specific to the utop package and thus is non-trivial to accurately test
with a mock utop package as we do with other dev tool tests.

Signed-off-by: Stephen Sherratt <[email protected]>
@gridbugs gridbugs enabled auto-merge (squash) November 20, 2025 06:39
@gridbugs gridbugs merged commit 6382802 into ocaml:main Nov 20, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants