Possible regression in the `select` stanza

vsiles commented 2 years ago

Expected Behavior

In dune lang 1.X, we are able to access files in subdirectories using select (like in https://github.com/facebook/hhvm/blob/master/hphp/hack/src/errors/dune#L16)

Actual Behavior

We are trying to move to dune lang 2.9 and this now raise an error:

Error: The format for files in this select branch must be a.{name}.ml

Reproduction

Here is a small POC that shows the error:

$ tree
.
├── a.stubs.ml
├── dune
├── dune-project
├── foo
│   └── a.foo.ml
└── xxx.ml
$ cat dune
(library
    (name a)
    (modules )
    (libraries
     (select a.ml from
      (foo -> foo/a.foo.ml)
      (-> a.stubs.ml)
    )
    )
)

(executable
    (name xxx)
    (modules xxx)
    (libraries a)
)
$ dune clean && dune build xxx.exe
File "dune", line 6, characters 14-26:
6 |       (foo -> foo/a.foo.ml)
                  ^^^^^^^^^^^^
Error: The format for files in this select branch must be a.{name}.ml

Specifications

$ dune --version
2.9.1
$ cat dune-project
(lang dune 2.9)
$ ocaml -version

Both on macos and linux (centos), everything installed normally via opam

Additional information

We're use to have a select that look like

    (select error_message_sentinel.ml from
      (facebook -> facebook/error_message_sentinel.ml)
      (-> error_message_sentinel.stubs.ml))))

where the facebook one doesn't have a special extension (because of our internal build too being not dune). If that could stay like that, it would be really great

bobot commented 2 years ago

Indeed the fact that the file is not in the current directory allows to drop the a.{name}.ml requirement. The requirement is made so that the sources of the select are not added automatically in the default set of modules and it ensures some uniformity.

I see two possibilities:

You can add a copy rules (copy# facebook/error_message_sentinel.ml error_message_sentinel.facebook.ml) and use this new file in select
Dune could loosen the requirement

Do you think the first possibility is simple enough?

vsiles commented 2 years ago

@rgrinberg seemed to think this was a regression and we should still allow the "old" behavior. About the possibilities:

it is a bit more complex (see my other issues about enabled_if as the file might not be there for OSS users (everything in the facebook subdirectory is Meta only, and not exported to OSS/github). The file might not be present on disk. Looks like we have to wait for dune 3.0 to get this
I wish that could be the solution :) Even with 3.0, it will require us some internal work to mirror this behavior to our internal build system (not dune) and I already tried, it is quite ugly/broken. Keeping the same pattern would really benefit us.

bobot commented 2 years ago

Do you mean that the copy rules requires the target to exists? I didn't know that.
I'm not against loosening the requirements, it would not break anything to just accept other directories in the check. Just uniformity. What do you think @jeremiedimino ?

ghost commented 2 years ago

What is the problem with the copy rule? @vsiles wit seems to me that with copy rule you can keep exactly the same pattern you have now.

Regarding loosing the requirements, it's going to work in this case and going to require special care for cases that use (include_subdirs ...). Unless it is clear that the copy method is not enough, it doesn't seem worth the extra complexity to me.

vsiles commented 2 years ago

The problem with the copy rule is that the file might be present (when we build inside FB's infra) or not (for github/OSS users). So far I never managed to have a conditional/optional copy:

; with copy_files and a missing file
(copy_files facebook/foo.fb.ml)
; File "dune", line 1, characters 0-0:
; Error: No rule found for foo.fb.ml

; with copy
(rule
  (target foo.fb.ml)
  (action (copy# facebook/foo.fb.ml foo.fb.ml))
)
; File "dune", line 6, characters 0-76:
; 6 | (rule
; 7 |   (target foo.fb.ml)
; 8 |   (action (copy# facebook/foo.fb.ml foo.fb.ml))
; 9 | )
; Error: No rule found for facebook/foo.fb.ml

As I mentioned, it looks like dune 3.0 will enable me to add a enabled_if in the rule to make it conditional, like select is. I reported this after discussing with @rgrinberg who said it was a regression. If it is not, feel free to close it (but I too feel it is a regression ;))

ghost commented 2 years ago

Could you pass --debug-dependency-path to check what is pulling foo.fb.ml?

Looking at the description of this PR, I would expect that foo.fb.ml is only required if the facebook library is present. And my understanding is that this library is not present for github/OSS users.

vsiles commented 2 years ago

Looks like I didn't clear the files correctly. If I remove the full facebook sub dir, using copy_files fails with

File "dune", line 2, characters 12-30:
2 | (copy_files facebook/foo.fb.ml)
                ^^^^^^^^^^^^^^^^^^
Error: Cannot find directory: facebook

But the "rule" version seems to work. I'll make further test and will comment more once I checked that

bobot commented 2 years ago

Great if a rule stanza seems to work. It would be great if copy_files could also work when the directory doesn't exist but it seems not straightforward #1099 @emillon.

ghost commented 2 years ago

It would be great if copy_files could also work when the directory doesn't exist

I'm not sure about that. If the directory doesn't exist, it's more likely to be a typo in the directory name and it's better to tell the user about it rather than silently do nothing.

bobot commented 2 years ago

We do no such check for dependencies in rules, why should we do it here?

ghost commented 2 years ago

It's true that we don't do it for (glob_files <pattern>), however I think we should. It's the same deal: if you make a typo, it will silently evaluate to nothing and it might be difficult to debug for users.

vsiles commented 2 years ago

I think I got around our use case using the rule/copy# solution, instead of copy_files. If you don't consider this access to subdirectories a regression, feel free to close the issue

ghost commented 2 years ago

It is fair to say that it is a regression in this case. However, the check that causes it was added for a good reason and I don't think we should relax it. But we could add a hint in the error message to help users upgrade.

maroneze commented 2 months ago

Just adding two notes to this issue:

The naming restriction is currently not documented, unless I missed it somewhere else;
This restriction makes it impossible to run codept in directories containing such dune files : Fatal error: exception Failure("Invalid module name: \"a.foo\""), which is a bit unfortunate.

Should the latter be reported as a Codept issue?

ocaml / dune