Dune should not look up other sub-directories when given `-p`

kit-ty-kate commented 4 years ago

When investigating the issue ligo's developers have here https://github.com/ocaml/dune/issues/2131#issuecomment-550475432 with @rgrinberg, we discovered that, when given a directory containing two sub-directories with two projects like so:

+ /
  - dune-project
  + project-a/
    - dune-project
    - project-a.opam
    - [...]
  + project-b/
    - dune-project
    - project-b.opam
    - [...]

dune build @install, even when given -p project-a, will scan for every packages used by project-b, thus, if opam install some of them at the same time, a race condition can occur in the same way as it happened in https://github.com/ocaml/dune/issues/2131

Here is a sample of the strace for dune build -p ocplib-resto with this archive:

stat("/home/kit_ty_kate/.opam/4.07.1/lib/ocaml/stdlib.dune", 0x7ffed0b53020) = -1 ENOENT (No such file or directory)
stat("/home/kit_ty_kate/.opam/4.07.1/lib/tezos-protocol-environment-sigs/dune-package", 0x7ffed0b530c0) = -1 ENOENT (No such file or directory)
stat("/home/kit_ty_kate/.opam/4.07.1/lib/tezos-protocol-environment-sigs/META", 0x7ffed0b530a0) = -1 ENOENT (No such file or directory)
stat("/home/kit_ty_kate/.opam/4.07.1/lib/META.tezos-protocol-environment-sigs", 0x7ffed0b530a0) = -1 ENOENT (No such file or directory)

kit-ty-kate commented 4 years ago

This error happened while I was downgrading dune from the dev version to 2.0.0:

#=== ERROR while compiling dune.2.0.0 =========================================#
# context     2.0.5 | linux/x86_64 | ocaml-base-compiler.4.09.0 | https://opam.ocaml.org/#4e17ba8d
# path        ~/.opam/4.09/.opam-switch/build/dune.2.0.0
# command     ~/.opam/4.09/.opam-switch/build/dune.2.0.0/./dune.exe build -p dune --profile dune-bootstrap -j 4
# exit-code   1
# env-file    ~/.opam/log/dune-10924-9a1f0d.env
# output-file ~/.opam/log/dune-10924-9a1f0d.out
### output ###
# File "/home/kit_ty_kate/.opam/4.09/lib/base/dune-package", line 1, characters 11-14:
# 1 | (lang dune 2.1)
#                ^^^
# Error: Version 2.1 of dune is not supported.
# Supported versions:
# - 1.0 to 1.12
# - 2.0

Every time dune itself is built, packages used in the tests are going to be looked for and opam has no information about that so base were not uninstalled before building the dune package.

cc @diml I think this increases the severity of the issue. Can this github issue be tagged accordingly?

kit-ty-kate commented 4 years ago

Btw, if anybody ever get the above error message too, the way to get out this without trashing your entire opam switch is to call: rm ~/.opam/<the-broken-switch>/lib/*/dune-package just before downgrading dune

ghost commented 4 years ago

This scenario is currently not supported by Dune. -p was designed for a single project with multiple packages. If the above scenario is valuable to users, we can extend -p to cover it. I propose to extend it as follow:

For any project in the workspace that has at least one package and such that the intersection between the set of packages defined by this project and the set of packages passed to -p is empty, then all the directories of this project are treated as data-only.

In the above example, if the toplevel dune-project file defined a package project-c, then the top-level directory and project-b would be treated as data-only directories. This also means that an env stanza in a top-level dune file would be ignored.

@rgrinberg what do you think? @kit-ty-kate could you cc the author of the project that triggered this discussion, just so that we can get feedback on why the project was organised this way and if the changes we are planning would work for them?

rgrinberg commented 4 years ago

I think with the env being ignored in such situations is going to be surprising to users.

Also, this still doesn't fix the issue in general. See the issue with private libraries pointed out earlier.

I wonder if it's possible to just fix the problem at the root. From what I understand, this problem wouldn't occur if dune did not create rules that it did not end up using. For example, if we setup all library & executable rules in their own leaf sub directories, there would be no problem. This still wouldn't be enough for some weird (rule ..) stanzas that access %{lib:..} for example.

kit-ty-kate commented 4 years ago

@kit-ty-kate could you cc the author of the project that triggered this discussion, just so that we can get feedback on why the project was organised this way and if the changes we are planning would work for them?

If you want. cc @tomjack. However as emphasized in https://github.com/ocaml/opam-repository/pull/15526#issuecomment-565613855 pointing to my second comment of this thread, I really think this issue should be addressed anyway and you seemed to agreed on this yourself in https://github.com/ocaml/dune/issues/2131#issuecomment-559504934 I'm not sure what has changed since.

ghost commented 4 years ago

I think with the env being ignored in such situations is going to be surprising to users.

Actually, scratch that. That's already the current semantic: env customisation already don't cross project boundaries.

@kit-ty-kate it's just the context that changed. But let's discuss this live, that will be simpler.

ejgallego commented 3 years ago

Hi folks , a pretty related issue is the one we just saw in Coq, c.f.

ocaml / dune

Dune should not look up other sub-directories when given `-p` #2913