Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(public_name -) different behaviour between executable and executables #5852

Closed
maiste opened this issue Jun 8, 2022 · 9 comments · Fixed by #7576
Closed

(public_name -) different behaviour between executable and executables #5852

maiste opened this issue Jun 8, 2022 · 9 comments · Fixed by #7576
Labels
docs Documentation improvements

Comments

@maiste
Copy link
Collaborator

maiste commented Jun 8, 2022

Expected Behavior

Hi, dear dune maintainers,
While working with executable and executables I noticed a difference in behaviour with the stanza (public_name -) and (public_names - -).
As described in the documentation, they should work the same way:

The executables stanza is the same as the executable stanza except that it’s used to describe several executables sharing the same configuration, so the plural executables stanza is used to represent more than one executable.

Actual Behavior

IIUC, the two executables descriptions should work the same way as one is just a contraction of the other one:

 (executable
   (public_name -)
   (name example_1)
   (modules example_1)
   (package dune-example))

 (executable
   (public_name -)
   (name example_2)
   (modules example_2)
   (package dune-example))
 (executables
   (public_names - -)
   (names example_1 example_2)
   (package dune-example))

Whereas 2. works without any issue and the - name is taken as ignored this field, 1. raised the following error:

Error: Multiple rules generated for _build/install/default/bin/-:
- examples/dune:3
- examples/dune:9
-> required by _build/default/dune-example.install
-> required by alias all
-> required by alias default

Reproduction

The reproduction steps are:

git clone https://github.com/maiste/dune-example.git
cd dune-example
dune build .

Specifications

  • Version of dune (output of dune --version): 3.2.0
  • Version of ocaml (output of ocamlc --version): 4.14.0
  • Operating system (distribution and version): Ubuntu 20.04 / EndeavourOS Linux
@nojb
Copy link
Collaborator

nojb commented Jun 9, 2022

one is just a contraction of the other one

Orthogonal to the issue described here, but note that this is not actually correct: executables allow sharing modules between the different executables, while this is not possible if you replace it by multiple executable stanzas. Probably a good idea to fix the documentation to point this out.

raised the following error:

This is probaly because using - in a single-executable stanza doesn't make a lot of sense: you may as well just supress the public_name field altogether. On the other hand, it may be a good idea to implement the same behaviour as for executables just for the sake of uniformity.

@maiste
Copy link
Collaborator Author

maiste commented Jun 9, 2022

Orthogonal to the issue described here, but note that this is not actually correct: executables allow sharing modules between the different executables, while this is not possible if you replace it by multiple executable stanzas. Probably a good idea to fix the documentation to point this out.

Apart from the possibility of sharing modules between executables, are they acting in the same way or is there other difference?

This is probably because using - in a single-executable stanza doesn't make a lot of sense: you may as well just supress the public_name field altogether. On the other hand, it may be a good idea to implement the same behaviour as for executables just for the sake of uniformity.

I agree that we should be able to remove this (public_name -). However, it needed to be used in case of some examples... This is the only hack I found to bypass the problem mentioned in ocaml/dune#5621.
By the same behaviour, do you that if (public_names - - -) look like this, it should raise an error?

@Alizter Alizter added the docs Documentation improvements label Jun 9, 2022
@nojb
Copy link
Collaborator

nojb commented Jun 9, 2022

By the same behaviour, do you that if (public_names - - -) look like this, it should raise an error?

No, I mean that any - in the public name fields (either in executable or executables stanzas) should be ignored.

Leonidas-from-XIV added a commit to Leonidas-from-XIV/dune that referenced this issue Apr 6, 2023
One aspect of ocaml#5852 revealed that this feature is under-documented, so
documenting the current behavior is a good first step.

Signed-off-by: Marek Kubica <marek@tarides.com>
rgrinberg pushed a commit that referenced this issue Apr 12, 2023
* Document advantage of `executables` over `executable`

One aspect of #5852 revealed that this feature is under-documented, so
documenting the current behavior is a good first step.
@emillon
Copy link
Collaborator

emillon commented Apr 17, 2023

@maiste now that this behaviour has been documented in #7505, is there anything that is left to be done on this issue?

@Alizter
Copy link
Collaborator

Alizter commented Apr 17, 2023

@emillon It seems to me like executable and executables have their public_name's validated differently.

@maiste
Copy link
Collaborator Author

maiste commented Apr 17, 2023

There was indeed two issues in this issue:

  • The problem with the documentation which was fixed in the PR you mentioned.
  • The inconsistent behaviour of - which is still an issue.

@emillon
Copy link
Collaborator

emillon commented Apr 17, 2023

Ok, I see. The alternatives are:

  1. make (public_name -) equivalent to no public name (consistent with public_names in executables)
  2. disallow (public_name -) in singular (executable) stanza

(Both solutions prevent the creation of an executable named - but that's still possible with a manual install stanza.)

I don't like adding equivalent forms ((public_name -) equivalent to no field at all), but this is already the case in the plural form. So that's an argument for solution 1. We can then forbid (public_name -) and (public_names) where all names are - in a future step if necessary.

Does that sound good to you?

@maiste
Copy link
Collaborator Author

maiste commented Apr 18, 2023

Yes, I agree with this proposition 👍

emillon added a commit to emillon/dune that referenced this issue Apr 18, 2023
Fixes ocaml#5852

This brings consistency with `(public_names)` in `(executables)`: a
plural stanzas with only `-` in `(public_names)` installs nothing.

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Copy link
Collaborator

emillon commented Apr 19, 2023

Implemented in #7576 .

emillon added a commit to emillon/dune that referenced this issue May 2, 2023
Fixes ocaml#5852

This brings consistency with `(public_names)` in `(executables)`: a
plural stanzas with only `-` in `(public_names)` installs nothing.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this issue May 2, 2023
* Make (public_name -) equivalent to no public name

Fixes #5852

This brings consistency with `(public_names)` in `(executables)`: a
plural stanza with only `-` in `(public_names)` installs nothing.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/opam-repository that referenced this issue May 3, 2023
CHANGES:

- Correctly set `MANPATH` in `dune exec`. Previously, we would use the `bin/`
  directory of the context. (ocaml/dune#7655, @rgrinberg)

- Allow overriding the `ocaml` binary with findlib configuration (ocaml/dune#7648,
  @rgrinberg)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will cause Dune to emit a warning which will become an error in 3.9. (ocaml/dune#7608,
  fixes ocaml/dune#7026, @Alizter)

- Dune can now detect Coq theories from outside the workspace. This allows for
  composition with installed theories (not necessarily installed with Dune).
  (ocaml/dune#7047, @Alizter, @ejgallego)

- Fix regression where Merlin was unable to handle filenames with uppercase
  letters under Windows. (ocaml/dune#7577, @nojb)

- On nix+macos, pass `-f` to the codesign hook to avoid errors when the binary
  is already signed (ocaml/dune#7183, fixes ocaml/dune#6265, @greedy)

- Fix bug where RPC clients built with dune-rpc-lwt would crash when closing
  their connection to the server (ocaml/dune#7581, @gridbugs)

- Introduce mdx stanza 0.4 requiring mdx >= 2.3.0 which updates the default
  list of files to include `*.mld` files (ocaml/dune#7582, @Leonidas-from-XIV)

- Fix RPC server on Windows (used for OCaml-LSP). (ocaml/dune#7666, @nojb)

- In `(executable)`, `(public_name -)` is now equivalent to no `(public_name)`.
  This is consistent with how `(executables)` handles this field.
  (ocaml/dune#7576 , fixes ocaml/dune#5852, @emillon)

- Change directory of odoc assets to `odoc.support` (was `_odoc_support`) so
  that it works with Github Pages out of the box. (ocaml/dune#7588, fixes ocaml/dune#7364,
  @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation improvements
Projects
None yet
4 participants