Skip to content

feat(artifacts): narrow binary lookups to declared dependencies - #15381

Open
punchagan wants to merge 13 commits into
ocaml:mainfrom
punchagan:narrow-pkg-which
Open

feat(artifacts): narrow binary lookups to declared dependencies#15381
punchagan wants to merge 13 commits into
ocaml:mainfrom
punchagan:narrow-pkg-which

Conversation

@punchagan

@punchagan punchagan commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Description

Binary lookups made from a directory owned by a package (via the package's
(dir ...) field) are narrowed to the packages that package depends on,
transitively. This covers %{bin:...}, %{bin-available:...}, the PATH
given to actions, and the binaries staged by (deps %{bin:...}).

Directories with no owning package are unaffected: nothing is narrowed and
every package is searched, as before. Binaries found on the ambient PATH,
and those bound by (env (binaries ...)), are also unaffected.

Related Issue and Motivation

To know which binaries a lock directory package provides, dune has to build
it -- the metadata currently available does not say. So resolving a single
%{bin:X} meant building every package in the lock directory to find out
which one provides X. Narrowing the search to the owning package's
dependency closure removes that: only the closure is built.

This also removes an inconsistency. dune build -p masks away sibling
packages' install stanzas, so a stanza using an undeclared sibling's binary
fails at release time -- during opam install, or in the opam-repository CI
-- while a plain dune build resolves it happily. The narrowing surfaces
that failure at dev time, with a hint naming the missing dependency.

Shrinking the built set is also a prerequisite for the in-and-out work
(#8652), where forcing every package's install cookie closes a dependency
cycle.

Decisions worth review

  • Transitive, not direct. Installing a package into an opam switch also
    installs its transitive dependencies, so their binaries are on PATH
    there. Narrowing to direct dependencies only would be stricter than what
    opam gives you, and would fail builds that opam-repository CI accepts. The
    counter-argument is that (deps (package ...)) (feat(pkg): install layouts for package sets #14373) and
    strict_package_deps are both immediate-only, so there is a case for
    consistency with those instead. This is decidable independently of the rest
    of the PR and does not affect the build-graph change.
  • No lang version gate. The narrowing applies wherever (dir ...) is
    used. (dir ...) is itself opt-in and available since (lang dune 3.21).
  • Not gated on a lock directory. Gating it would mean that adopting
    dune pkg lock silently turns on dependency checking for sibling binaries,
    which has nothing to do with lock files.

Known limitations

  • This reduces the search space; it does not enforce that dependencies are
    declared. A binary that is narrowed out can still be found on the ambient
    PATH.
  • Dependency filters such as {with-test} are not interpreted, so a
    dependency is visible whatever its filter says.

@punchagan
punchagan force-pushed the narrow-pkg-which branch 5 times, most recently from 344e923 to e258dde Compare July 1, 2026 10:12
@punchagan
punchagan requested a review from Alizter July 1, 2026 17:03
@punchagan
punchagan force-pushed the narrow-pkg-which branch 2 times, most recently from 8aaa6fd to e186b0f Compare July 2, 2026 09:12
@punchagan
punchagan force-pushed the narrow-pkg-which branch 2 times, most recently from 2ea1389 to 985e8af Compare July 3, 2026 15:11
Comment thread src/dune_rules/pkg_rules.mli Outdated
Alizter added a commit that referenced this pull request Jul 6, 2026
<!-- Thank you for contributing to dune!

 For general guidelines on contributing to dune, see
https://github.com/ocaml/dune/blob/main/CONTRIBUTING.md#developing-dune
-->

## Description
Extend the test as a precursor to fixing PATH to include only the bin
layouts of packages that the owning package of a stanza explicitly
depend on.

## Related Issue and Motivation
<!-- Why is this change required? What problem does it solve? -->
<!-- If it closes an open issue, link to the issue here. -->
<!-- Non-trivial contributions are expected to be preceded by an issue,
as
     per CONTRIBUTING.md. -->

This change is a preparatory improvement to the test for the changes in
#15381
Alizter added a commit that referenced this pull request Jul 6, 2026
## Description

This commit extends the resolve-program-from-undeclared-pkg.t cram test
in preparation for narrowing of the lockdir packages on PATH based on
the dependency closure of a stanza's owning package.

This PR is similar to #15222

## Related Issue and Motivation
<!-- Why is this change required? What problem does it solve? -->
<!-- If it closes an open issue, link to the issue here. -->
<!-- Non-trivial contributions are expected to be preceded by an issue,
as
     per CONTRIBUTING.md. -->

This is a preparatory change in the resolve-program test case for
narrowing of the lockdir package binaries available on PATH implemented
in #15381.
@punchagan
punchagan force-pushed the narrow-pkg-which branch 2 times, most recently from 8c4b3eb to 4845db6 Compare July 8, 2026 07:34
@punchagan punchagan mentioned this pull request Jul 17, 2026
3 tasks
@punchagan
punchagan force-pushed the narrow-pkg-which branch 6 times, most recently from 10e2ec4 to c88489f Compare July 23, 2026 10:41
Alizter added a commit that referenced this pull request Jul 24, 2026
<!-- Thank you for contributing to dune!

 For general guidelines on contributing to dune, see
https://github.com/ocaml/dune/blob/main/CONTRIBUTING.md#developing-dune
-->

## Description
<!-- Briefly describe your changes -->

This is another preparatory commit with tests for #15381. It captures
current behavior w.r.t bin resolution from both workspace deps, lockdir
deps and a mix of deps.

## Checklist

- [x] Tests added, if applicable.
- [ ] [Change log entry
added](../CONTRIBUTING.md#updating-the-changelog) for any user-facing
changes.
- [ ] Documentation added for any user-facing changes.
@punchagan
punchagan force-pushed the narrow-pkg-which branch 2 times, most recently from 102e137 to 48132a8 Compare July 29, 2026 14:25
punchagan added a commit to punchagan/dune that referenced this pull request Sep 2, 2026
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
punchagan added a commit to punchagan/dune that referenced this pull request Sep 2, 2026
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Alizter added a commit that referenced this pull request Sep 2, 2026
<!-- Thank you for contributing to dune!

 For general guidelines on contributing to dune, see
https://github.com/ocaml/dune/blob/main/CONTRIBUTING.md#developing-dune
-->

## Description
<!-- Briefly describe your changes -->
env vars defined in a (context ...) stanza are documented to have higher
precedence than env vars defined in the root env stanza of a workspace.
The prose in the cram test also indicates this, but the test asserted
incorrect precedence. This seems to have been indicated in a review
comment [here] and marked done, but may have been undone by an incorrect
rebase or something.

Only consumers of Super_context.context_env like `dune exec` were
affected by this. Rules were not affected since they resolved their
environment separately through `Env_node.external_env` which is not
affected by this bug.

[here]: #1147 (comment)


## Related Issue and Motivation
<!-- Why is this change required? What problem does it solve? -->
<!-- If it closes an open issue, link to the issue here. -->
<!-- Non-trivial contributions are expected to be preceded by an issue,
as
     per CONTRIBUTING.md. -->

This issue was discovered while working on #15381 

## Checklist

- [ ] Tests added, if applicable.
- [x] [Change log entry
added](../CONTRIBUTING.md#updating-the-changelog) for any user-facing
changes.
- [ ] Documentation added for any user-facing changes.
punchagan added a commit to punchagan/dune that referenced this pull request Sep 3, 2026
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
punchagan added a commit to punchagan/dune that referenced this pull request Sep 3, 2026
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>

@Alizter Alizter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The precedence in a few places is a bit confusing. Here is an example of something that is unintentionally changed in this PR:

  $ make_lockdir

  $ make_lockpkg provider <<'EOF'
  > (version 0.0.1)
  > (build
  >  (progn
  >   (system "echo '#!/bin/sh' > managed-tool")
  >   (system "echo 'exit 0' >> managed-tool")
  >   (system "chmod +x managed-tool")
  >   (system "echo 'bin: [ \"managed-tool\" ]' > provider.install")))
  > EOF

  $ make_dune_project 3.25
  $ cat >>dune-project <<'EOF'
  > (package
  >  (name consumer)
  >  (allow_empty)
  >  (dir .)
  >  (depends provider))
  > EOF
  $ mkdir empty-bin
  $ cat >dune <<EOF
  > (env
  >  (_
  >   (env_vars
  >    (PATH $PWD/empty-bin))))
  > 
  > (rule
  >  (alias prepare)
  >  (action (run %{bin:managed-tool})))
  > 
  > (rule
  >  (action
  >   (with-stdout-to output
  >    (bash
  >     "command -v managed-tool >/dev/null && echo visible || echo hidden"))))
  > EOF

  $ dune build @prepare
  $ dune build output
  $ cat _build/default/output
  hidden                                                                                                                       

I think we should try to preserve the previous semantics as much as possible. I'm not opposed to changing these semantics at a later point if we think it will make more sense however, but that should be a bit more intentional.

I've given some other comments about some subtle PATH changes.

I think the order should be something like:

  1. Action-local additions: (deps (package ...)) and staged %{bin:...} dependencies.
  2. Directory (env (binaries ...)) bindings
  3. Directory (env_vars (PATH ...))
  4. Automatically inferred package-closure paths
  5. Context/ambient PATH

Comment thread test/blackbox-tests/test-cases/pkg/bin-narrowing/env-binaries.t
Comment thread doc/changes/fixed/15381.md Outdated
Comment on lines +1 to +3
- Expanding `%{bin:...}` in a lock directory context no longer builds every
package in the lock directory. Only the packages in the dependency closure
of the stanza's owning package are built. (#15381, @punchagan)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suppose this claim only holds for stanzas owned by a package, maybe worth mentioning that unowned stanzas still build every package.

Comment thread src/dune_rules/context.ml
Comment thread test/blackbox-tests/test-cases/pkg/bin-narrowing/transitive-deps.t Outdated
Comment thread test/blackbox-tests/test-cases/pkg/bin-narrowing/lockdir-deps.t Outdated
punchagan added a commit to punchagan/dune that referenced this pull request Sep 9, 2026
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
punchagan and others added 13 commits September 9, 2026 08:52
This commit improves the bin-narrowing/transitive-deps.t test to list
the lockdir packages being built for bin pform lookups.

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Co-authored-by: Puneeth Chaganti <punchagan@muse-amuse.in>

Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
To discover the binaries provided by a lockdir package, dune currently
has to build it -- the currently available metadata on packages does not
provide this information. So, searching for a binary meant building all
the lockdir packages to see which one provides a particular binary.

This commit narrows the search space to the set of packages that the
directory's owning package depends upon. A directory has an owning
package when a (package ...) stanza has it as the (dir ...) field value.
When a directory has no owning package, the search is not narrowed and
all the lockdir packages are built as before.

The dependency closure is included in the narrowed search to match what
opam users see: installing a package into an opam switch also installs
the transitive dependencies, and so their binaries are available on
PATH, even when those dependencies aren't explicitly depended upon.

Binaries not found in the narrowed set still fallback to the ambient
PATH, as before.

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
%{bin:...} lookups have been narrowed to the owning package's
dependencies. But, actions can also look up binaries by their names
through PATH, for instance through (system ...). Previously, every
lockdir package's install prefix contributed its bin directory to PATH,
so a binary not visible to %{bin:...} could still be reached via PATH.

This commit puts only the bin directories of the same narrowed
dependency closure on PATH, so that both lookups are consistent with
each other.

PATH is now added via Env_node rather than the context-wide lock
directory env, since the visible packages could be different per
directory. To facilitate this, Env_node's env is now split into two
parts, the inherited part and the node's own PATH, which cannot be
inherited.

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
We restricted the search space when looking up binaries provided by
lockdir packages to the dependencies declared on the owning package.
This commit extends this restriction to binaries provided by local
workspace packages too.

Under `dune build -p`, sibling install stanzas are masked away, so
finding a sibling binary provided by an undeclared dependency fails at
release time during `opam install`, or in the opam-repository CI, while
a plain `dune build` resolves it happily. This narrowing promotes this
validation to dev time and surfaces the failure earlier.

The `(dir ...)` field takes on an additional meaning as a result: the
search for sibling binaries is now narrowed to the transitive closure of
the packages declared in the `(depends ...)` field. The narrowing no
longer depends on a lock directory being present, to keep the behavior
consistent for projects newly adopting package management.

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Naming a binary in `(deps %{bin:...})` also stages it into a `.binaries`
directory that is prepended to the action's PATH, so that the action can
invoke it by bare name. This staging looked the binary up in the
context-wide artifacts, which are not narrowed. So when the directory's
own lookup picked a different file -- an undeclared sibling shadowing a
declared lockdir package of the same name -- the staged copy came first
on PATH, and `(system ...)` and `%{bin:...}` named different files
within a single action.

This commit passes the directory's own artifacts to the staging instead,
so that both name the same file. They are obtained through
`Super_context.artifacts_host`, since the staging is given the host
context but a directory of the target context.

This only matters once local binary lookups are narrowed. Until then the
narrowing only applied to the lockdir fallthrough, whose results the
staging discards anyway, so either set of artifacts staged the same file.

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Previously, creation of bin layouts had two look-ups for binaries, once
in Bin_layout.create where it recorded the lookup name and install
names, and the second time when actually creating the symlinks. This
commit collapses them into a single look-up by storing the original path
for the binary to create the symlink.

Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
Signed-off-by: Puneeth Chaganti <punchagan@muse-amuse.in>
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.

3 participants