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

Add "dune describe package-entries" #7480

Merged
merged 4 commits into from
Jul 24, 2023
Merged

Add "dune describe package-entries" #7480

merged 4 commits into from
Jul 24, 2023

Conversation

moyodiallo
Copy link
Collaborator

@moyodiallo moyodiallo commented Apr 3, 2023 •
edited
Loading

The need is about knowing if a private executable is going to be installed before building the project.

This PR is related to ocurrent/opam-dune-lint#46:
The command dune describe external-lib does not give the package name of private executable if it going to be installed, dune describe external-lib-deps is used by opam-dune-lint to extract the external dependencies.

@moyodiallo moyodiallo mentioned this pull request Apr 3, 2023
Merged
@rgrinberg
Copy link
Member

The implementation looks fine but how about a clearer name like package-entries? entries alone doesn't make it clear what is being printed.

@moyodiallo
Copy link
Collaborator Author

The implementation looks fine but how about a clearer name like package-entries? entries alone doesn't make it clear what is being printed.

I buy package-entries.

@moyodiallo moyodiallo marked this pull request as ready for review April 4, 2023 09:22
@Alizter Alizter changed the title Add "dune describe entries" Add "dune describe package-entries" Apr 4, 2023
@rgrinberg rgrinberg requested a review from emillon April 4, 2023 19:27
emillon
emillon requested changes Apr 11, 2023
View reviewed changes
src/dune_rules/dune_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/install.ml Outdated Show resolved Hide resolved
test/blackbox-tests/test-cases/describe-package-entries.t Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator

emillon commented Apr 11, 2023

The implementation is fine, but just to check, does opam-dune-lint work with this PR or is this in anticipation? I'd prefer if we have a clear idea of what's necessary in dune rather than add this "just in case". (and if that's all that is necessary, that's perfect)

@moyodiallo
Copy link
Collaborator Author

moyodiallo commented Apr 11, 2023 •
edited
Loading

The implementation is fine, but just to check, does opam-dune-lint work with this PR or is this in anticipation? I'd prefer if we have a clear idea of what's necessary in dune rather than add this "just in case". (and if that's all that is necessary, that's perfect)

I have already tested with opam-dune-lint. I'm working on finishing opam-dune-lint. I had an intention to notify it, after matching the all needs of opam-dune-lint.

@emillon
Copy link
Collaborator

emillon commented Apr 11, 2023

ok then I'd hold off onto merging these PRs until they've been tested with opam-dune-lint. let me know what that's the case.

@moyodiallo moyodiallo force-pushed the describe-entries branch 2 times, most recently from 4e6ee5b to 2d7b54a Compare April 18, 2023 10:23
@moyodiallo
Copy link
Collaborator Author

Ready for another review and to be merged.

@moyodiallo moyodiallo requested a review from emillon April 18, 2023 14:06
@emillon
Copy link
Collaborator

emillon commented Apr 20, 2023

has this been tested with opam-dune-lint?

@emillon emillon mentioned this pull request Apr 20, 2023
Merged
emillon
emillon requested changes Apr 20, 2023
View reviewed changes
bin/describe.ml Outdated Show resolved Hide resolved
src/dune_rules/install.ml Outdated Show resolved Hide resolved
src/dune_rules/install.ml Outdated Show resolved Hide resolved
src/dune_rules/install.ml Outdated Show resolved Hide resolved
src/dune_rules/install_rules.ml Outdated Show resolved Hide resolved
@emillon emillon mentioned this pull request Apr 20, 2023
Closed
@Alizter Alizter mentioned this pull request May 8, 2023
Open
@moyodiallo
Copy link
Collaborator Author

@emillon Ready to move forward, the needs are already addressed. Same as #7478.

emillon
emillon reviewed Jun 8, 2023
View reviewed changes
bin/describe.ml Outdated Show resolved Hide resolved
src/dune_rules/install.ml Outdated Show resolved Hide resolved
@moyodiallo moyodiallo requested a review from emillon June 8, 2023 17:04
rgrinberg
rgrinberg reviewed Jun 8, 2023
View reviewed changes
src/dune_rules/install.ml Outdated Show resolved Hide resolved
@Alizter
Copy link
Collaborator

Alizter commented Jun 8, 2023

FTR I am in the middle of a big refactoring of dune describe. It might make sense to wait for that. Notably you will have to introduce a new subcommand for package-entries.

@emillon
Copy link
Collaborator

emillon commented Jun 9, 2023

FTR I am in the middle of a big refactoring of dune describe. It might make sense to wait for that. Notably you will have to introduce a new subcommand for package-entries.

I wouldn't put an order between those. Both branches exist and a rebase will have to be done, it can be done in a direction or the other.

@Alizter
Copy link
Collaborator

Alizter commented Jun 16, 2023 •
edited
Loading

It looks like my branch got merged first. Could you add a package_entries.ml file to bin/describe/ and follow the other describe commands? Let me know if you need any help.

@moyodiallo moyodiallo requested a review from rgrinberg June 27, 2023 12:09
Alizter
Alizter approved these changes Jun 27, 2023
View reviewed changes
Copy link
Collaborator

@Alizter Alizter left a comment

Choose a reason for hiding this comment

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

LGTM, can you add a package_entries.mli?

emillon
emillon reviewed Jul 7, 2023
View reviewed changes
Copy link
Collaborator

@emillon emillon left a comment

Choose a reason for hiding this comment

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

Looks good once the to_dyn is changed!

src/install/entry.ml Outdated Show resolved Hide resolved
@emillon
Copy link
Collaborator

emillon commented Jul 7, 2023

Can you promote the tests and add a changelog entry?

@emillon
Copy link
Collaborator

emillon commented Jul 12, 2023

ping?

moyodiallo and others added 4 commits July 24, 2023 13:04
@moyodiallo @emillon
Add "dune describe entries"
8d0d819 
    The need is about knowing if a private executable is going to be
    installed before building the project.

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
@moyodiallo @emillon
Update src/install/entry.ml
20b87be 
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
Signed-off-by: Alpha Issiaga DIALLO <alpha@tarides.com>
@moyodiallo @emillon
Fix tests
6ae4006 
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
@moyodiallo @emillon
Update changelog
15871bd 
Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
@emillon emillon force-pushed the describe-entries branch from 94914b4 to 15871bd Compare July 24, 2023 11:09
@emillon emillon merged commit 8aeadd5 into ocaml:main Jul 24, 2023
@moyodiallo moyodiallo deleted the describe-entries branch July 24, 2023 11:47
@emillon emillon mentioned this pull request Jul 28, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 28, 2023
@emillon
[new release] dune (15 packages) (3.10.0~alpha1)
9ddddfc 
CHANGES:

- Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter)

- Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more
  items. (ocaml/dune#8196, @Alizter)

- Add `dune show installed-libraries` as an alias of the `dune
  installed-libraries` command. (ocaml/dune#8135, @Alizter)

- Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193,
  @Alizter)

- Add `dune build --dump-gc-stats FILE` argument to dump garbage collection
  stats to a named file. (ocaml/dune#8072, @Alizter)

- Fix bug with ppx and Reason syntax due to missing dependency in sandboxed
  action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter)

- Add `dune describe package-entries` to print all package entries (ocaml/dune#7480,
  @moyodiallo)

- Improve `dune describe external-lib-deps` by adding the internal dependencies
  for more information. (ocaml/dune#7478, @moyodiallo)

- Re-enable background file digests on Windows. The files are now open in a way
  that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 31, 2023
@emillon
[new release] dune (15 packages) (3.10.0)
82e64cd 
CHANGES:

- Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter)

- Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more
  items. (ocaml/dune#8196, @Alizter)

- Add `dune show installed-libraries` as an alias of the `dune
  installed-libraries` command. (ocaml/dune#8135, @Alizter)

- Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193,
  @Alizter)

- Add `dune build --dump-gc-stats FILE` argument to dump garbage collection
  stats to a named file. (ocaml/dune#8072, @Alizter)

- Fix bug with ppx and Reason syntax due to missing dependency in sandboxed
  action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter)

- Add `dune describe package-entries` to print all package entries (ocaml/dune#7480,
  @moyodiallo)

- Improve `dune describe external-lib-deps` by adding the internal dependencies
  for more information. (ocaml/dune#7478, @moyodiallo)

- Re-enable background file digests on Windows. The files are now open in a way
  that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
@emillon emillon mentioned this pull request Jul 31, 2023
Merged
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Aug 10, 2023
@moyodiallo @emillon
Add "dune describe package-entries" (ocaml#7480)
9910cbb 
* Add "dune describe entries"

    The need is about knowing if a private executable is going to be
    installed before building the project.

Signed-off-by: Alpha DIALLO <moyodiallo@gmail.com>
Co-authored-by: Etienne Millon <etienne.millon@gmail.com>
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@emillon @nberth
[new release] dune (15 packages) (3.10.0)
5407dc7 
CHANGES:

- Add `dune show rules` as alias of the `dune rules` command. (ocaml/dune#8000, @Alizter)

- Fix `%{deps}` to expand properly in `(cat ...)` when containing 2 or more
  items. (ocaml/dune#8196, @Alizter)

- Add `dune show installed-libraries` as an alias of the `dune
  installed-libraries` command. (ocaml/dune#8135, @Alizter)

- Fix the `severity` of error messages sent over RPC which was missing. (ocaml/dune#8193,
  @Alizter)

- Add `dune build --dump-gc-stats FILE` argument to dump garbage collection
  stats to a named file. (ocaml/dune#8072, @Alizter)

- Fix bug with ppx and Reason syntax due to missing dependency in sandboxed
  action (ocaml/dune#7932, fixes ocaml/dune#7930, @Alizter)

- Add `dune describe package-entries` to print all package entries (ocaml/dune#7480,
  @moyodiallo)

- Improve `dune describe external-lib-deps` by adding the internal dependencies
  for more information. (ocaml/dune#7478, @moyodiallo)

- Re-enable background file digests on Windows. The files are now open in a way
  that prevents race condition around deletion. (ocaml/dune#8262, fixes ocaml/dune#8268, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alizter Alizter Alizter approved these changes

@emillon emillon emillon requested changes

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

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

Successfully merging this pull request may close these issues.
4 participants
@moyodiallo @rgrinberg @emillon @Alizter