Look for META files rather than just directories to list plugins #10458

shym · 2024-04-25T17:08:27Z

This PR proposes a possible fix for #10457.

If the plugin directory contains anything but directories that each contain a META file, loading plugins fails. This PR proposes to check additionally for the presence of the META files and ignore other entries in the plugin directory.

anmonteiro

works for me. it'd be nice to have a test case for this

otherlibs/dune-site/src/plugins/plugins.ml

anmonteiro · 2024-04-25T22:32:24Z

otherlibs/dune-site/src/plugins/plugins.ml

  List.concat
    (List.map
-       (fun dir -> Array.to_list (Sys.readdir dir))
+       (fun dir ->


this can probably be a List.filter_map instead.

It's available since OCaml 4.08, so it should be safe to use inside dune (https://github.com/ocaml/ocaml/blob/8b6b7cfbf26ae50ae87d5849c827e609a1309c98/stdlib/list.mli#L195-L200)

Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>

If the plugin directory contains anything but directories that each contain a `META` file, loading plugins fails This can happen naturally in particular in the following scenario: - install a package providing a plugin to a command in another package - and remove that plugin package. This leaves an empty directory which triggers that failure. Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>

shym · 2024-04-26T13:57:08Z

Thank you for your review!

I wasn’t sure about the best way to test this behaviour. I have added such a possible test, where the empty directory is explicitly created to mimick what happens with OPAM.

Your suggestion to go with filter_map sent me on another exploration. Checking that Sys.file_exists on each candidate directory is not enough to ensure readdir will not fail. So I ended up with the following proposition: just try to Sys.readdir each plugins directory and handle the errors as empty directories.

rgrinberg · 2024-05-08T17:35:27Z

ping @anmonteiro

otherlibs/dune-site/test/opam-uninstall.t

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

emillon · 2024-05-22T08:25:31Z

Thanks. Can you add a changelog entry in doc/changes/?

Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>

…ml#10458) * Add a test including the uninstallation of an OPAM plugin package Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net> * Look for META files rather than just directories to list plugins If the plugin directory contains anything but directories that each contain a `META` file, loading plugins fails This can happen naturally in particular in the following scenario: - install a package providing a plugin to a command in another package - and remove that plugin package. This leaves an empty directory which triggers that failure. Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net> * Update otherlibs/dune-site/test/opam-uninstall.t Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> * Add a changelog entry Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net> --------- Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net> Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com> Co-authored-by: Etienne Millon <me@emillon.org>

@anmonteiro

CHANGES: ### Added - allow libraries with the same `(name ..)` in projects as long as they don't conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro, @jchavarri) - `dune describe pp` now finds the exact module and the stanza it belongs to, instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro) - Print the result of `dune describe pp` with the respective dialect printer. (ocaml/dune#10322, @anmonteiro) - Add new flag `--context` to `dune ocaml-merlin`, which allows to select a Dune context when requesting Merlin config. Add `dune describe contexts` subcommand. Introduce a field `generate_merlin_rules` for contexts declared in the workspace, that allows to optionally produce Merlin rules for other contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri) - melange: add include paths for private library `.cmj` files during JS emission. (ocaml/dune#10416, @anmonteiro) - `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`, `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the index(es). (ocaml/dune#10422, @voodoos) - Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate indexes that can be read by tools such as Merlin to provide project-wide references search. (ocaml/dune#10422, @voodoos) - merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to configure a merlin reader (ocaml/dune#8567, @andreypopp) ### Changed - melange: treat private libraries with `(package ..)` as public libraries, fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415, @anmonteiro) - install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego) ### Fixed - Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes ocaml/dune#10056, @jonludlam) - Make `dune-site`'s `load_all` function look for `META` files so that it doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes ocaml/dune#10457, @shym) - Fix incorrect warning for libraries defined inside non-existant directories using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525, @rgrinberg) - Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes ocaml/dune#7671, @lzy0505) - Make sure to truncate dune's lock file after locking and unlocking so that users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg) - mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro)

@anmonteiro

CHANGES: ### Added - allow libraries with the same `(name ..)` in projects as long as they don't conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro, @jchavarri) - `dune describe pp` now finds the exact module and the stanza it belongs to, instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro) - Print the result of `dune describe pp` with the respective dialect printer. (ocaml/dune#10322, @anmonteiro) - Add new flag `--context` to `dune ocaml-merlin`, which allows to select a Dune context when requesting Merlin config. Add `dune describe contexts` subcommand. Introduce a field `generate_merlin_rules` for contexts declared in the workspace, that allows to optionally produce Merlin rules for other contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri) - melange: add include paths for private library `.cmj` files during JS emission. (ocaml/dune#10416, @anmonteiro) - `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`, `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the index(es). (ocaml/dune#10422, @voodoos) - Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate indexes that can be read by tools such as Merlin to provide project-wide references search. (ocaml/dune#10422, @voodoos) - merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to configure a merlin reader (ocaml/dune#8567, @andreypopp) ### Changed - melange: treat private libraries with `(package ..)` as public libraries, fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415, @anmonteiro) - install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego) ### Fixed - Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes ocaml/dune#10056, @jonludlam) - Make `dune-site`'s `load_all` function look for `META` files so that it doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes ocaml/dune#10457, @shym) - Fix incorrect warning for libraries defined inside non-existant directories using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525, @rgrinberg) - Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes ocaml/dune#7671, @lzy0505) - Make sure to truncate dune's lock file after locking and unlocking so that users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg) - mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro) - virtual libraries: fix an issue where linking an executable involving several virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)

@anmonteiro

CHANGES: ### Added - allow libraries with the same `(name ..)` in projects as long as they don't conflict during resolution (via `enabled_if`). (ocaml/dune#10307, @anmonteiro, @jchavarri) - `dune describe pp` now finds the exact module and the stanza it belongs to, instead of guessing the name of the preprocessed file. (ocaml/dune#10321, @anmonteiro) - Print the result of `dune describe pp` with the respective dialect printer. (ocaml/dune#10322, @anmonteiro) - Add new flag `--context` to `dune ocaml-merlin`, which allows to select a Dune context when requesting Merlin config. Add `dune describe contexts` subcommand. Introduce a field `generate_merlin_rules` for contexts declared in the workspace, that allows to optionally produce Merlin rules for other contexts besides the one selected for Merlin (ocaml/dune#10324, @jchavarri) - melange: add include paths for private library `.cmj` files during JS emission. (ocaml/dune#10416, @anmonteiro) - `dune ocaml-merlin`: communicate additional directives `SOURCE_ROOT`, `UNIT_NAME` (the actual name with wrapping) and `INDEX` with the paths to the index(es). (ocaml/dune#10422, @voodoos) - Add a new alias `@ocaml-index` that uses the `ocaml-index` binary to generate indexes that can be read by tools such as Merlin to provide project-wide references search. (ocaml/dune#10422, @voodoos) - merlin: add optional `(merlin_reader CMD)` construct to `(dialect)` stanza to configure a merlin reader (ocaml/dune#8567, @andreypopp) ### Changed - melange: treat private libraries with `(package ..)` as public libraries, fixing an issue where `import` paths were wrongly emitted. (ocaml/dune#10415, @anmonteiro) - install `.glob` files for Coq theories too (ocaml/dune#10602, @ejgallego) ### Fixed - Don't try to document non-existent libraries in doc-new target (ocaml/dune#10319, fixes ocaml/dune#10056, @jonludlam) - Make `dune-site`'s `load_all` function look for `META` files so that it doesn't fail on empty directories in the plugin directory (ocaml/dune#10458, fixes ocaml/dune#10457, @shym) - Fix incorrect warning for libraries defined inside non-existant directories using `(subdir ..)` and used by executables using `dune-build-info` (ocaml/dune#10525, @rgrinberg) - Don't try to take build lock when running `coq top --no-build` (ocaml/dune#10547, fixes ocaml/dune#7671, @lzy0505) - Make sure to truncate dune's lock file after locking and unlocking so that users cannot observe incorrect pid's (ocaml/dune#10575, @rgrinberg) - mdx: link mdx binary with `byte_complete`. This fixes `(libraries)` with foreign archives on Linux. (ocaml/dune#10586, fixes ocaml/dune#10582, @anmonteiro) - virtual libraries: fix an issue where linking an executable involving several virtual libries would cause an error. (ocaml/dune#10581, fixes ocaml/dune#10460, @rgrinberg)

shym force-pushed the plugin-meta branch from d46c45b to 3c30884 Compare April 25, 2024 17:09

rgrinberg requested a review from anmonteiro April 25, 2024 22:28

anmonteiro reviewed Apr 25, 2024

View reviewed changes

shym force-pushed the plugin-meta branch from c177103 to 7658834 Compare April 26, 2024 13:43

shym and others added 2 commits April 26, 2024 15:46

Add a test including the uninstallation of an OPAM plugin package

6ec2787

Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>

shym force-pushed the plugin-meta branch from 7658834 to 7607948 Compare April 26, 2024 13:46

anmonteiro reviewed May 9, 2024

View reviewed changes

otherlibs/dune-site/test/opam-uninstall.t Outdated Show resolved Hide resolved

Update otherlibs/dune-site/test/opam-uninstall.t

87980b6

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

anmonteiro approved these changes May 9, 2024

View reviewed changes

anmonteiro and others added 2 commits May 8, 2024 19:50

Merge branch 'main' into plugin-meta

46f276c

Merge branch 'main' into plugin-meta

03220ab

emillon linked an issue May 22, 2024 that may be closed by this pull request

An empty directory in the plugin directory makes load_all fail #10457

Closed

Add a changelog entry

35db5fd

Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>

emillon merged commit 3ec11db into ocaml:main May 22, 2024
27 of 28 checks passed

shym deleted the plugin-meta branch May 22, 2024 11:48

gridbugs mentioned this pull request Jun 5, 2024

[new release] dune (15 packages) (3.16.0~alpha1) ocaml/opam-repository#26022

Closed

emillon mentioned this pull request Jun 17, 2024

[new release] dune (15 packages) (3.16.0) ocaml/opam-repository#26101

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Look for META files rather than just directories to list plugins #10458

Look for META files rather than just directories to list plugins #10458

shym commented Apr 25, 2024

anmonteiro left a comment

anmonteiro Apr 25, 2024

shym commented Apr 26, 2024

rgrinberg commented May 8, 2024

emillon commented May 22, 2024

Look for META files rather than just directories to list plugins #10458

Look for META files rather than just directories to list plugins #10458

Conversation

shym commented Apr 25, 2024

anmonteiro left a comment

Choose a reason for hiding this comment

anmonteiro Apr 25, 2024

Choose a reason for hiding this comment

shym commented Apr 26, 2024

rgrinberg commented May 8, 2024

emillon commented May 22, 2024