Usability/Documentation issues with copy_files/glob

[maroneze]

Expected Behavior

1. copy_files does not warn if the <glob> expression matches a directory instead of a file. It would be very useful to report such errors, in the same way as the Cannot copy files onto themselves message already does for files in the local directory. Currently, if the user mistakenly uses a directory name (or even the name of a non-existing file), dune silently does nothing, without reporting any errors.

2. It is unclear to me why (copy_files (glob_files_rec <glob>)) does not work. The copy_files documentation mentions:

   <glob> represents the set of files to copy. See the glob for details.

   That page, in turn, mentions:

   Dune supports globbing files in a single directory via (glob_files ...) and, starting with Dune 3.0, in all sub-directories recursively via (glob_files_rec ...).

   I then assumed that <glob> in the copy_files part could be replaced with (glob_files ...). So I tried using (copy_files (files (glob_files_rec ../dir/*))), but it complains with Error: Unexpected list. If I remove the files part, since the copy_files doc mentions that The short form (copy_files <glob>) is equivalent to (copy_files (files <glob>)), it then complains with Error: field files missing. In both cases, I am unable to combine copy_files with glob_files, which seems it should be possible from the documentation. The lack of usage examples for this option makes it harder to understand how it must be written.

Actual Behavior

1. No errors reported in case of non-existing/directory name
2. Error when trying to combine copy_files and glob_files; lack of usage examples.

Reproduction

First issue:

1. cd /tmp && dune init project proj
2. cd proj
3. mkdir d; touch c
4. echo "(copy_files ../d)" >> bin/dune # user meant "../c" instead of "../d"
5. echo "(copy_files ../invalid)" >> bin/dune # user entered a non-existing path
6. dune build

No error/warning is emitted; no files are copied.

Second issue:

1. echo "(copy_files (glob_files_rec ../d/*))" >> bin/dune # documentation suggests it might work
2. dune build # error
3. Erase erroneous line from bin/dune
4. echo "(copy_files (files (glob_files_rec ../d/*)))" >> bin/dune # documentation suggests it might work
5. dune build # another error

Also, there are no examples with glob_files_rec.

Specifications

• Version of dune (output of dune --version): 3.3.1
• Version of ocaml (output of ocamlc --version): 4.08.1
• Operating system (distribution and version): Linux Fedora 36

[maroneze]

Another (minor) issue I just noticed: when a dirs stanza is used, other directories are excluded when using copy_files; however, the error message is Cannot find directory: <dir>. It took me a while to realize it was the dirs stanza from another dune file that was causing it. For instance, if my root dune file contains (dirs bin lib test), but inside lib I have a dune file with copy_files ../share/*, Dune's message Cannot find directory: share will not be that helpful. Ideally, something like "path excluded from Dune's build" would be best. Otherwise, a simple addition to the existing message, such as "Check that the path exists and is not excluded by any dirs stanza" might be enough. Then again, the wording of this message is very important: it should tell users that dirs may exclude certain directories, but not that they should add a dirs stanza; otherwise beginners might feel compelled to add such stanzas to their files...

[maroneze]

Sorry for yet another related comment on this subject:

glob_files/glob_files_rec are useful in deps, but I believe they are rarely (if ever) used to match possibly empty lists. For instance, if I mistakenly write (glob_files_rec ../node_moduless/*.js) (with an extra 's'), the glob will match no files at all, but will silently continue.

There are probably cases where an empty match is desirable; but from my experience with Makefile and the wildcard function (which also evaluates to the empty list without any warnings), I noticed that in most cases users want to be warned when their globs result in empty expansions. For instance, shells default to not-nullglob precisely to avoid issues with accidental empty globbing.

Ideally, two versions of glob_files/glob_files_rec should exist, one of them warning/erroring out if it expands to nothing (preferably the default one), and another with the current behavior. Adding a modifier such as nullglob, e.g. glob_files (nullglob) <files> could allow switching between them.