Skip to content

feat: Synopsis to rule stanza, display synopses in dune show #11609

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

Open
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

maxim092001
Copy link
Contributor

@maxim092001 maxim092001 commented Apr 5, 2025

Implementation to resolve #11522

This PR makes multiple changes:

  • Allow to attach optional (synopsis "Doc") to rule stanza
  • Print all synopses with their locations (if loc is not none) via dune show aliases
    • If one alias attached to multiple rules with synopses then all synopses printed with rule locations
  • Print synopsis for each target if there is one via dune show targets
  • Attach synopsis to default alias
    • Had to adjust some tests for new behavior
    • Will be done in separate PRs
  • Bumped version to 3.19 Merged d274531

Some examples from cram tests are:

(rule
(targets file1.ml)
(aliases rule-with-synopses and-another-rule)
(synopsis "Rule creates file1.ml")
(action
    (write-file file1.ml "")))

(rule
(targets file2.ml)
(aliases rule-with-synopses and-another-rule)
(synopsis "Rule creates file2.ml")
(action
    (write-file file2.ml "")))

(rule
(targets file3.ml)
(alias and-another-rule)
(synopsis "Rule creates file3.ml")
(action
    (write-file file3.ml "")))
  $ dune show targets
  dune
  dune-project
  file1.ml
    - dune:1 Rule creates file1.ml
  file2.ml
    - dune:8 Rule creates file2.ml
  file3.ml
    - dune:15 Rule creates file3.ml
dune show aliases
  all
  and-another-rule
    - dune:1 Rule creates file1.ml
    - dune:8 Rule creates file2.ml
    - dune:15 Rule creates file3.ml
  default
    - This alias corresponds to the default argument for dune build. More
      information on
      https://dune.readthedocs.io/en/latest/reference/aliases/default.html
  fmt
  ocaml-index
  pkg-install
  rule-with-synopses
    - dune:1 Rule creates file1.ml
    - dune:8 Rule creates file2.ml

@maxim092001 maxim092001 changed the title Add optional synopsis to alias, add test for alias with synopsis feat: Add optional synopsis to alias, add test for alias with synopsis Apr 5, 2025
@maxim092001 maxim092001 changed the title feat: Add optional synopsis to alias, add test for alias with synopsis feat: Add optional synopsis field to rule stanza Apr 6, 2025
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
@maxim092001 maxim092001 force-pushed the alias-with-synopsis branch from c4e5fac to 796679e Compare April 25, 2025 14:22
@maxim092001 maxim092001 force-pushed the alias-with-synopsis branch from 4883dca to 765d926 Compare April 25, 2025 15:42
maxim092001 and others added 10 commits April 25, 2025 17:56
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
Signed-off-by: Maxim Grankin <[email protected]>
val add_deps
: t
-> ?loc:Stdune.Loc.t
-> ?synopsis:Synopsis.t option
Copy link
Member

Choose a reason for hiding this comment

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

I don't get what would the synopsis mean in this instance. Does this mean an alias can have multiple actions attached to it with different descriptions? When do we make use of this?

Copy link
Contributor Author

@maxim092001 maxim092001 May 3, 2025

Choose a reason for hiding this comment

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

It is used in

Rules.Produce.Alias.add_deps

Does this mean an alias can have multiple actions attached to it with different descriptions?

Yes, it has multiple rules with different targets attached to it.

This is essentially what allows us to print synopses in dune show aliases

and+ targets = add_user_rule sctx ~dir ~rule ~action ~expander in

Attaches synopsis to targets, so it can be showed in dune show targets

Copy link
Member

Choose a reason for hiding this comment

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

In dune, there's currently two ways to attach dependencies to an alias. First, using the target of a rule:

(rule
 (alias ..))

Second, by attaching it using the alias stanza:

(alias
 (name ..)
 (deps ..))

In both cases, we're attaching an edge from a target to the alias in that directory. I think it would be wrong to attach an edge to one form but not the other.

But more over, it doesn't even seem right to allow annotating this edge at all. In other words, you're adding the ability to add the same target to different aliases with a different description. Is that really what we're looking for? The target should have one description.

I think it would make more sense to attach the description somewhere in Dune_engine.Rule.t. E.g see the info module for example. For loaded aliases, we don't have an explicit type, but I imagine allowing for one synopsis per alias name per directory is the right way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 5ff4fd9 and 4f204c3 I removed passing synopsis through add_deps.

I also removed ability to add synopsis to alias stanza.

Now, you can only add alias to rule stanza and tests are passing as before. You can still get all aggregated synopses for alias if it attached to multiple rules.

I think it would make more sense to attach the description somewhere in Dune_engine.Rule.t.

Yes, I believe it is attached there now

@maxim092001 maxim092001 force-pushed the alias-with-synopsis branch from 692b097 to a1fd6ec Compare May 8, 2025 09:14
@maxim092001
Copy link
Contributor Author

maxim092001 commented May 8, 2025

@Leonidas-from-XIV I've added docs in a1fd6ec. Is there anything else I need to do from administrative PoV? For changelog entry, do I need to add this feat to Changes.m?

@Leonidas-from-XIV
Copy link
Collaborator

For changelog entry, do I need to add this feat to Changes.m?

No, please add a file called 11609.md to https://github.com/ocaml/dune/tree/main/doc/changes as that creates less merge conflicts and incorrect merges (e.g. when new releases happen in the meantime old changelog entries can get merged in the wrong place). The release manager will take these entries and compose them into a changelog when preparing the release.

@maxim092001
Copy link
Contributor Author

@Leonidas-from-XIV added changelog file in 5ebd4fc

Let me know if there is anything else that needs to be done!

@maxim092001 maxim092001 force-pushed the alias-with-synopsis branch from 05e18ab to 95471b3 Compare July 6, 2025 09:21
@maxim092001 maxim092001 force-pushed the alias-with-synopsis branch from 95471b3 to 4f204c3 Compare July 6, 2025 09:22
Signed-off-by: Maxim Grankin <[email protected]>
@maxim092001 maxim092001 force-pushed the alias-with-synopsis branch from 2434f0a to 5ff4fd9 Compare July 6, 2025 09:31
@maxim092001
Copy link
Contributor Author

Failures are the same as on main branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add synopsis to dune rules/aliases. Show synopsis in dune show aliases
4 participants