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

fix(describe-pp): try more candidates for pped dialect sources #10321

Merged
merged 6 commits into from
Mar 31, 2024
Merged

fix(describe-pp): try more candidates for pped dialect sources #10321

merged 6 commits into from
Mar 31, 2024

Conversation

anmonteiro
Copy link
Collaborator

  • I was trying out this feature, and I noticed that dune doesn't always produce the same files for preprocessed dialect sources.
  • this change looks for both foo.pp.re.ml and foo.re.pp.ml (which can happen based on whether preprocessing is (action ..) or (pps ..)

@anmonteiro anmonteiro requested a review from emillon March 28, 2024 04:54
@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-fix-extension branch 2 times, most recently from ffd5e5a to fd5953a Compare March 28, 2024 04:57
@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-fix-extension branch 2 times, most recently from 123f96d to 8a10564 Compare March 28, 2024 19:01
@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-fix-extension branch 3 times, most recently from c807847 to f7b3db8 Compare March 31, 2024 00:26
@anmonteiro
Copy link
Collaborator Author

In the latest version:

  • derive Module.t from the file passed via the CLI
  • change the staged_pps check to use the new code

rgrinberg
rgrinberg approved these changes Mar 31, 2024
View reviewed changes
bin/describe/describe_pp.ml
| None -> Memo.return None
| Some (for_, preprocess) ->
let+ modules =
Dune_rules.Dir_contents.get sctx ~dir
Copy link
Member

Choose a reason for hiding this comment

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

There's a bunch of logic here that's similar to topmodule. I wonder if it can be unified in some way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. I'll look into seeing whether that's possible.

bin/describe/describe_pp.ml Outdated Show resolved Hide resolved
anmonteiro and others added 5 commits March 31, 2024 13:48
@anmonteiro
fix(describe-pp): try more candidates for pped dialect sources
799333c 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@rgrinberg @anmonteiro
_
98944db 
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@anmonteiro
use the Module.t to find the correct filename in obj dir
b65d917 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
refactor: reuse the code to get dialect and ml_kind
8388e29 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
fix: staged_pps detection and fallback
32df23c 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-fix-extension branch from e7881c7 to 3088dae Compare March 31, 2024 20:48
@anmonteiro
fix: no fallback
66d4634 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/describe-pp-fix-extension branch from 3088dae to 66d4634 Compare March 31, 2024 20:49
@anmonteiro anmonteiro merged commit 4296464 into ocaml:main Mar 31, 2024
26 of 27 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/describe-pp-fix-extension branch March 31, 2024 21:07
@anmonteiro anmonteiro mentioned this pull request Apr 1, 2024
Merged
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Apr 1, 2024
@anmonteiro
chore: add changelog entry for ocaml#10321
7bb3498 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro mentioned this pull request Apr 1, 2024
Merged
anmonteiro added a commit that referenced this pull request Apr 1, 2024
@anmonteiro
chore: add changelog entry for #10321 (#10352)
f6b1dda 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
@anmonteiro @rgrinberg
fix(describe-pp): try more candidates for pped dialect sources (ocaml…
809265b 
…#10321)

* fix(describe-pp): try more candidates for pped dialect sources

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

* _

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

* use the `Module.t` to find the correct filename in obj dir

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

* refactor: reuse the code to get dialect and ml_kind

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

* fix: staged_pps detection and fallback

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

* fix: no fallback

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

---------

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
@anmonteiro
chore: add changelog entry for ocaml#10321 (ocaml#10352)
2188845 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

@emillon emillon Awaiting requested review from emillon

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

Successfully merging this pull request may close these issues.
2 participants
@anmonteiro @rgrinberg