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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
May 22, 2024
Merged

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

merged 6 commits into from
May 22, 2024

Conversation

shym
Copy link
Contributor

@shym shym commented Apr 25, 2024

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.

@rgrinberg rgrinberg requested a review from anmonteiro April 25, 2024 22:28
anmonteiro
anmonteiro reviewed Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

otherlibs/dune-site/src/plugins/plugins.ml Outdated Show resolved Hide resolved
otherlibs/dune-site/src/plugins/plugins.ml Outdated
List.concat
(List.map
(fun dir -> Array.to_list (Sys.readdir dir))
(fun dir ->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

shym and others added 2 commits April 26, 2024 15:46
@shym
Add a test including the uninstallation of an OPAM plugin package
6ec2787 
Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
@shym @anmonteiro
Look for META files rather than just directories to list plugins
7607948 
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
Copy link
Contributor Author

shym commented Apr 26, 2024

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
Copy link
Member

ping @anmonteiro

@anmonteiro
Update otherlibs/dune-site/test/opam-uninstall.t
87980b6 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@emillon
Copy link
Collaborator

emillon commented May 22, 2024

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

@emillon 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
@shym
Add a changelog entry
35db5fd 
Signed-off-by: Samuel Hym <samuel.hym@rustyne.lautre.net>
@emillon emillon merged commit 3ec11db into ocaml:main May 22, 2024
27 of 28 checks passed
@shym shym deleted the plugin-meta branch May 22, 2024 11:48
MA0010 pushed a commit to MA0010/dune that referenced this pull request Jun 5, 2024
@shym @anmonteiro
@emillon @MA0010
Look for META files rather than just directories to list plugins (oca…
8eb6944 
…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>
@gridbugs gridbugs mentioned this pull request Jun 5, 2024
Closed
gridbugs added a commit to gridbugs/opam-repository that referenced this pull request Jun 5, 2024
@gridbugs
[new release] dune (15 packages) (3.16.0~alpha1)
0858f47 
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)
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 17, 2024
@emillon
[new release] dune (15 packages) (3.16.0)
a6069b9 
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)
@emillon emillon mentioned this pull request Jun 17, 2024
Merged
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
@emillon @avsm
[new release] dune (15 packages) (3.16.0)
4423f45 
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 added a commit to anmonteiro/dune that referenced this pull request Nov 17, 2024
@shym @anmonteiro @emillon
Look for META files rather than just directories to list plugins (oca…
5a2747e 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro anmonteiro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

An empty directory in the plugin directory makes load_all fail
4 participants
@shym @rgrinberg @emillon @anmonteiro