Add repro for #7335.

[rlepigre]

No description provided.

[rgrinberg]

I think you're forgetting that root discovery is disabled inside the cram tests.

[rlepigre]

I think you're forgetting that root discovery is disabled inside the cram tests.

I did not know about that when I set up this test, but the issue is real as this is a minimized example of something I came upon in a real project.

[Alizter]

You need to unset the INSIDE_DUNE env var to allow root detection, but even then it will be the wrong root hence why INSIDE_DUNE exists. In other tests, we explicitly set the root when we need to, but that would mean that in this case we are not able to reproduce this behaviour of incorrect root finding.

Would you be able to create a repo with a minimal example? Then we can determine if Dune's behaviour is really incorrect.

[rlepigre]

I don't really see the point of putting that in a separate repo, but I can easily put that in a tarball that you guys can use to test. It has basically the same contents as the test. This is still broken in main.

Here it is: install-workspace.tar.gz.

It contains a README.md which explains the steps to reproduce the various problems.

[Alizter]

@rlepigre OK I can reproduce the issue, it definitely looks like something fishy is going on when using -p. I think I should be able to modify your cram test to reproduce.

[Alizter]

The bug here is that when -p is used, the workspace detection behaviour seems to change. The behaviours of dune build and dune clean are however intended. You can unset INSIDE_DUNE for this cram test since there is a dune-workspace file that will be chosen as the root. (by build and clean).

[Alizter]

I'm going to overtake this PR.

[Alizter]

So for the record, here is what the test looks like from before when unsetting INSIDE_DUNE:

The setup for this test is two nested dune projects:
- a project at the root of the workspace (defining package "test1",
- a project in the "dep" folder defining package "test2".

When we prepare installation for the "test2" package from the workspace root,
the installation files are produced under "_build/install".

  $ unset INSIDE_DUNE

  $ dune build --display=quiet -p test2 @install
  $ find . -name _build
  ./_build
  $ dune clean

If we do the same from the "dep" directory, we expect the same result, but the
files are rather produced in "dep/_build/install".

  $ (cd dep && dune build --display=quiet -p test2 @install)
  $ find . -name _build
  ./dep/_build

Calling "dune clean" at the workspace root does not clean the nested "_build"
directory that was produced by the previous call to "dune build".

  $ dune clean
  $ find . -name _build
  ./dep/_build

  $ (cd dep && dune clean)
  Entering directory '$TESTCASE_ROOT'
  Leaving directory '$TESTCASE_ROOT'
  $ find . -name _build
  ./dep/_build

Similarly, calling "dune build @install" in the "dep" folder only prepares the
installation of the packages under "dep", not all packages of the workspace.
The resulting files end up again under "dep/_build".

  $ (cd dep && dune build @install)
  Entering directory '$TESTCASE_ROOT'
  Leaving directory '$TESTCASE_ROOT'
  $ find . -name _build
  ./dep/_build
  ./_build

As you can see, dune build and dune clean have the correct behaviour and it is only -p that gets the wrong root.

[Alizter]

The reason -p is getting the wrong root by the way, is that it is a command line alias for:

--release --only-packages

and then release is super fun:

          [ "--root"
          ; "."
          ; "--ignore-promoted-rules"
          ; "--no-config"
          ; "--profile"
          ; "release"
          ; "--always-show-command-line"
          ; "--promote-install-files"
          ; "--require-dune-project-file"
          ; "--default-target"
          ; "@install"
          ]

I am not very happy with --release's behaviour. At the very least it should be better documented. The root setting is kind of stupid IMO. @rgrinberg WDYT about the fact that --release (and by extension -p) is setting the root to the current directory?

[rgrinberg]

It can't be changed at this point. Too many released packages rely on it. So it sounds like this bug was from improper use of -p.