ocaml / dune

A composable build system for OCaml.
https://dune.build/
MIT License
1.62k stars 401 forks source link

"dune init project" doesn't create a dune project file, creates unnecessary _build file #5429

Closed yminsky closed 1 year ago

yminsky commented 2 years ago

Here's the reproduction:

[hello_test]$ dune init proj hello --ppx ppx_inline_test --inline-tests
Success: initialized project component named hello
[hello_test]$ tree
.
├── _build
│   └── log
└── hello
    ├── bin
    │   ├── dune
    │   └── main.ml
    ├── hello.opam
    ├── lib
    │   └── dune
    └── test
        ├── dune
        └── hello.ml

5 directories, 7 files

The project simply won't build with the dune-project file. And if you put the dune-project file next to the hello.opam file, the _build will be generated automatically when you first start to build, and it will be generated one level down in the hierarchy, in the "hello" directory.

Specifications

shonfeder commented 2 years ago

I thought I'd drop a short note with a bit of context. Afaict, creation of dune-project files is fixed on master (I've just tried locally on a fresh build, and this is pretty well covered in the black box integration tests, see https://github.com/ocaml/dune/blob/45ab36f6d36dbf5f15be1d3293770ba744bbb8a6/test/blackbox-tests/test-cases/dune-init.t/run.t#L437).

I don't have full context on why the breakage was introduced, but the previous behavior of dune init had relied on the automatic generation of dune-project files by dune build, but this functionality was removed at some point (see #4239) which motivated the followup #4367. However, this latter has not been included in the 2.9.x releases.

So, baring a unexpected regression, the problem of the missing dune-project file should be resolved by the pending release of 3.0 (see ocaml/opam-repository#20721).

However, I am also seeing the spurious _build file generated on master, and I'm not sure what's responsible for that.

bobot commented 2 years ago

We still seems to have problems with dune init in 3.0: #5460

shonfeder commented 2 years ago

I added a reply there. But please note that #5460 is a design/UX bug.

afaik, with 3 released the only relevant part of this I issue is the spurious _build file.

bobot commented 2 years ago

I'm fine with making dune init always create the root at cwd with any template.

dune init [lib|exec|...] NAME [PATH] [OPTION]...

@rgrinberg I was proposing to do it only when [PATH], where the component is created, is not already in a dune project.

The relation between cwd and [PATH] is interesting. Is it reasonable to require cwd to always be a parent of [PATH] even if [PATH]is already in a dune project, or should we fail only if it is not the case when [PATH] is not in a dune project?

rgrinberg commented 2 years ago

@bobot I pushed it to 3.1.0 as it seems like the fix needs a bit more discussion.

bobot commented 2 years ago

afaik, with 3 released the only relevant part of this I issue is the spurious _build file.

That's true. with 3.0 the only direct problem here is the spurious _build.

shonfeder commented 1 year ago

This topic came up again recently in a slack channel. Does anyone know off the top of their head why the spurious _build dir is being introduced and/or what would need to be tweaked to fix it?

gridbugs commented 1 year ago

The workspace root is being set to the cwd when dune init proj foo gets run. The fix would be to set the workspace root to the newly-create project directory instead.

The workspace root is currently set during command-line argument parsing of Common.t, in Common.term here: https://github.com/ocaml/dune/blob/d1ed7defa48a8f267d6ecf3f3e162610e8b58301/bin/common.ml#L1021 Also present in that function is the logic that creates the rpc lock file inside the _build directory in the workspace root.

The newly-created project's name isn't know until after command-line arguments have been parsed, so we'd need to change how Common.t is initialized so that the workspace root can be changed after parsing. This is complicated by the fact that the argument parser for that type has side effects (creating the rpc lock file) that depend on the workspace root.

One solution might be to introduce a builder for Common.t that contains all of its fields but whose creation doesn't imply side effects, and parse the builder type instead of the Common.t type. Then any changes to workspace root can be made in the builder before crystalising it as a Common.t.

gridbugs commented 1 year ago

I've had a go at fixing this in https://github.com/ocaml/dune/pull/6707