Skip to content

feat(pkg): install layouts for package sets#14373

Draft
Alizter wants to merge 1 commit into
ocaml:mainfrom
Alizter:push-nkmrmtxvyunk
Draft

feat(pkg): install layouts for package sets#14373
Alizter wants to merge 1 commit into
ocaml:mainfrom
Alizter:push-nkmrmtxvyunk

Conversation

@Alizter
Copy link
Copy Markdown
Collaborator

@Alizter Alizter commented Apr 29, 2026

(deps (package ...)) materializes a scoped install layout under _build/install/<context>/.packages/<digest>/ containing only the declared package dependencies. This replaces the old alias-based mechanism where (deps (package foo)) populated the shared _build/install/ staging area, making every package in the workspace discoverable regardless of what was declared.

The layout includes only immediate deps (no transitive closure). Actions should declare what they need explicitly. Environment variables (PATH, OCAMLPATH, CAML_LD_LIBRARY_PATH, OCAMLFIND_IGNORE_DUPS_IN, OCAMLTOP_INCLUDE_PATH, MANPATH) are set from the layout directory and consed onto the directory env via extend_action.

The layout only applies to workspace packages. Lock-dir packages and the site/plugin system are unaffected.

See doc/dev/install-layouts.md for design details.

Related:

Follow-up work is tracked in the TODO section of doc/dev/install-layouts.md.

@Alizter Alizter changed the title chore: convert package-dep.t test to a file test feat(pkg): install layouts for package sets Apr 29, 2026
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch from 465d843 to 3dc8980 Compare April 29, 2026 20:34
Comment thread test/blackbox-tests/test-cases/reporting-of-cycles.t/run.t
@Alizter Alizter self-assigned this Apr 29, 2026
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch 19 times, most recently from 7abe25a to 2cb8e69 Compare May 2, 2026 02:05
Comment thread src/dune_rules/super_context.ml Outdated
Comment thread src/dune_rules/install_layout.mli Outdated
@@ -0,0 +1,19 @@
open Import

(** Must be called during initialization to wire up install entry resolution. *)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was this module introduced to avoid cycles?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've split it off for now to avoid cycles as I was working on the feature. When I think its ready, I'll try to inline and break any cycles unless its just not possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is not possible to detect cycles in a graph where you can't discover all the edges.

You'll be able to do it with a lock dir.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think we're talking about different cycles here. I'm talking about cycles in the build graph due to the ordering of the modules, i.e. ocamldep.

I think you're mentioning cycle detection in the "package graph" and I agree that's not possible. But that's not what this module is for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cycles in the build graph are supposed to be detected automatically by the engine. Why would we have manual detection?

Comment thread src/dune_rules/gen_rules.ml Outdated
Comment thread src/dune_rules/dep_conf_eval.ml Outdated
@rgrinberg
Copy link
Copy Markdown
Member

Looks like this is on the right track. Why do you bother to preserve the old behavior? Do you think there was a way to reasonably way to depend on it by users?

Comment thread src/dune_rules/dep_conf_eval.mli Outdated
Comment thread src/dune_rules/exe_rules.ml Outdated
Comment thread src/dune_rules/foreign_rules.ml Outdated
Comment thread src/dune_rules/pp_spec_rules.ml Outdated
Comment thread src/dune_rules/dep_conf_eval.ml Outdated
Comment thread src/dune_rules/dep_conf_eval.ml Outdated
Comment thread src/dune_rules/gen_rules.ml Outdated
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch 9 times, most recently from f811a2c to e0a137e Compare May 13, 2026 15:30
Alizter added a commit that referenced this pull request May 21, 2026
Resolve `%{bin:...}` to the build artifact path (`Original_path`)
instead of the install staging area. When `%{bin:...}` appears in `(deps
...)`, a bin-layout directory is created under
`_build/install/<context>/.binaries/<digest>/` with correctly-named
symlinks, and added to `PATH`.

The legacy staging paths (`_build/install/default/bin`, `lib`, etc.)
remain on the root env for now. They cannot be removed until `(deps
(package ...))` provides its own scoped layout (the install-layout PR
#14373), since actions using `(package ...)` deps still rely on the
staging area for `PATH` and `OCAMLPATH` visibility.

Env binaries (from `(env (binaries ...))`) are excluded from the
bin-layout since they already use `.bin/` on `PATH`.

`Dep_conf_eval.unnamed` and `Dep_conf_eval.named` now return a single
`Env.t Action_builder.t` whose evaluation both registers deps and
produces the env (previously a tuple with a separate `unit
Action_builder.t` for deps registration). Build-tracking-only callers
(`lib_flags`, `simple_rules`) discard the env via
`Action_builder.ignore`.

`(include FILE)` inside `(deps ...)` propagates the recursive call's
`bin_env` via a new `Include_result` variant on `dep_evaluation_result`;
`named_paths_builder` folds the include `bin_env`s into the final
`bin_env` via `Env_path.extend_env_concat_path`. Without this, the
action's `PATH` would not get a bin-layout dir for include-d bin pforms.

The change is not gated by lang version: #3324 affects every lang
version, so gating would leave the bug unfixed for pre-3.24 projects.
The user-visible realignments are confined to in-`_build` paths and the
migration cost is small enough to not warrant a per-project opt-in.

- Fixes #3324
- Depends on #14430 (merged)
- Depends on #14431 (merged)
- [x] changelog
- [x] documentation

## Known limitations (deferred to follow-up)

- dune's sandbox does not relocate the `PATH` environment variable. The
bin-layout dir on `PATH` points at the un-sandboxed `_build/`. This
works locally because absolute paths are still readable from inside the
sandbox, but would break under remote action execution. Tracked by
`test/blackbox-tests/test-cases/bin-pform/sandbox.t`.
- `Bin_layout.Key.reverse_table` grows unboundedly in long-running
`--watch` sessions (one entry per unique sorted bin set). Pattern
inherited from `Ppx_driver.Key`. Bounded by the number of distinct bin
sets the workspace evaluates; unlikely to be a real problem.
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch 2 times, most recently from 63b989d to 4307f17 Compare May 22, 2026 01:14
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch 5 times, most recently from 75238d4 to 47e044e Compare May 22, 2026 16:11
Alizter added a commit that referenced this pull request May 22, 2026
`(deps (:name (package foo)))` is silently accepted by the parser but
`%{name}` resolves to an empty path list. The package contributes no
paths to the binding, only env vars (when `(package ...)` is allowed by
the surrounding code). This is rarely what users intend.

We now reject the construct in `named_paths_builder` with a clear error
pointing at the `(package ...)` location and a hint to move the entry
out of the binding.

This was pre-existing behaviour; not specific to the install-layout PR
#14373 but found while reviewing.
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch from 47e044e to 9ee71bd Compare May 23, 2026 21:47
`(deps (package ...))` materializes a scoped install layout under
`_build/install/<context>/.packages/<digest>/` containing only the declared
package dependencies. This replaces the old alias-based mechanism where
`(deps (package foo))` populated the shared `_build/install/` staging area,
making every package in the workspace discoverable regardless of what was
declared.

The layout includes only immediate deps (no transitive closure). Actions
should declare what they need explicitly. Environment variables (PATH,
OCAMLPATH, CAML_LD_LIBRARY_PATH, OCAMLFIND_IGNORE_DUPS_IN,
OCAMLTOP_INCLUDE_PATH, MANPATH) are set from the layout directory and
consed onto the directory env via extend_action.

The layout only applies to workspace packages. Lock-dir packages and the
site/plugin system are unaffected.

See doc/dev/install-layouts.md for design details and known limitations.

Signed-off-by: Ali Caglayan <alizter@gmail.com>

---

Follow-up work (not blocking):

Pre-existing limitations preserved by this PR:

- Lock-dir packages: (deps (package lockdir-pkg)) builds the package
  but does not set up layout env vars. This was already broken before
  this PR (the old `package` function returned only the build action);
  this PR documents it as a known TODO rather than fixing or worsening
  it. dune exec lockdir-tool continues to work via bin/exec.ml's
  unconditional staging cons.

- Sandbox PATH non-relocation: layout dirs on PATH/OCAMLPATH point at
  un-sandboxed _build/. Works locally; would break remote execution.
  Same mechanism as the bin-layout limitation in ocaml#14432.

New limitations from this PR's scope:

- _root section collisions (lib_root, share_root, libexec_root) raise
  hard errors today. Opam-style override via dep DAG ordering is
  future work.

- Install_layout.Key.reverse_table, Bin_layout.Key.reverse_table, and
  Ppx_driver.Key.reverse_table are three copies of the same digest+
  reverse table pattern. Worth a Digest_key functor; non-blocking.

Test coverage gaps:

- Mixing workspace + lock-dir package deps in a single (deps ...).

CR-someday Alizter: factor the Context.for_host fallback used in
make_bin_env, combined_package_deps_builder and
super_context.ml:make_root_env into a single helper.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the push-nkmrmtxvyunk branch from 9ee71bd to 2bf6fea Compare May 23, 2026 21:55
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.

2 participants