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

melange - add melange.emit targets to @all alias as well by default #7926

Merged
merged 2 commits into from
Jun 11, 2023
Merged

melange - add melange.emit targets to @all alias as well by default #7926

merged 2 commits into from
Jun 11, 2023

Conversation

haochenx
Copy link
Contributor

@haochenx haochenx commented Jun 9, 2023 •
edited

It is confusing that dune build does not build targets specified by a melange.emit stanza by default.
This PR intends to address this by adding melange.emit targets to @all alias as well as (the current default) @melange alias by default.

This stems from the discussion of #7924.
more specifically, this PR implements the suggestion in #7924 (comment)

Subtasks

  • adding melange.emit stanza targets to @all alias by default (in addition to the @melange alias)
  • add test cases
  • [ ] fix existing test cases if necessary (update: does not seem to break any existing test)
  • change log entry

anmonteiro
anmonteiro reviewed Jun 9, 2023
View reviewed changes
Copy link
Collaborator

@anmonteiro anmonteiro left a comment •
edited

Choose a reason for hiding this comment

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

This not what I had in mind. We want to keep the melange implicit alias, and additionally add the melange rules to the default one.

I tried to explain the required changes in #7924 (comment), how can I help clarify?

@haochenx
Copy link
Contributor Author

haochenx commented Jun 9, 2023

This not what I had in mind. We want to keep the melange implicit alias, and additionally add the melange rules to the default one.

ah, I see. i'll try that approach.

@haochenx

This comment was marked as outdated.

Sign in to view
@haochenx haochenx requested a review from anmonteiro June 9, 2023 08:31
@haochenx
Copy link
Contributor Author

haochenx commented Jun 9, 2023

@anmonteiro i have just revised the approach. please check whether this matches your expectation

@haochenx haochenx force-pushed the melange-default-alias branch 2 times, most recently from 013da4a to f5823c7 Compare June 9, 2023 09:01
@haochenx

This comment was marked as outdated.

Sign in to view
@haochenx haochenx marked this pull request as ready for review June 9, 2023 09:13
@haochenx haochenx mentioned this pull request Jun 9, 2023
Merged
@haochenx haochenx changed the title melange - change default alias to @all melange - add melange.emit targets to 'all' alias as well by default Jun 9, 2023
@haochenx haochenx changed the title melange - add melange.emit targets to 'all' alias as well by default melange - add melange.emit targets to @all alias as well by default Jun 9, 2023
@rgrinberg rgrinberg marked this pull request as draft June 9, 2023 12:47
anmonteiro
anmonteiro reviewed Jun 10, 2023
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.

I think we also want to attach the rules to the default alias, even if the user specifies (alias foo).

My mental model:

  • rules are unconditionally attached to the default alias
  • if (alias foo) is specified, additionally attach them to the foo alias; otherwise attach them to the implicit alias.

@haochenx haochenx requested a review from anmonteiro June 10, 2023 06:08
@haochenx
Copy link
Contributor Author

@anmonteiro I have addressed your last comments. PTAL

Also I'm not sure what to put in the change log. Could you please help with that?

anmonteiro
anmonteiro reviewed Jun 10, 2023
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.

Just a few small suggestions, otherwise starting to look good. Ends up being a nice cleanup.

test/blackbox-tests/test-cases/melange/aliases.t Outdated Show resolved Hide resolved
src/dune_rules/melange/melange_rules.ml Outdated
@@ -172,6 +172,18 @@ let build_js ~loc ~dir ~pkg_name ~mode ~module_systems ~output ~obj_dir ~sctx
; Dep src
]))

(** attach [deps] to the specified [alias] AND the (dune default) [all] alias.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(** attach [deps] to the specified [alias] AND the (dune default) [all] alias.
(* attach [deps] to the specified [alias] AND the (dune default) [all] alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reflected in 7c49c1d

src/dune_rules/melange/melange_rules.ml Outdated Show resolved Hide resolved
@haochenx haochenx requested a review from anmonteiro June 10, 2023 06:54
anmonteiro
anmonteiro approved these changes Jun 10, 2023
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.

LGTM, thanks for working on this, @haochenx!

@anmonteiro
Copy link
Collaborator

For the changelog entry, how about:

attach melange rules to the default alias

@haochenx haochenx marked this pull request as ready for review June 10, 2023 17:26
@haochenx haochenx requested a review from Alizter June 10, 2023 17:26
@haochenx
Copy link
Contributor Author

haochenx commented Jun 10, 2023 •
edited

I've rebased with the main and added Changelog as per @anmonteiro suggested.

@anmonteiro @Alizter Thank you for your guidance!

I'm not sure what's the procedure for the closure of a PR here (especially who's to resolve discussions and how to ask the maintainer for a consider of merge). Please advice how to proceed.

@anmonteiro
Copy link
Collaborator

I'm not sure what's the procedure for the closure of a PR here (especially who's to resolve discussions and how to ask the maintainer for a consider of merge). Please advice how to proceed.

I made a few small edits to test / changes. PR is approved from my perspective, but I'd like to have @rgrinberg take a final look. Thanks!

rgrinberg
rgrinberg approved these changes Jun 11, 2023
View reviewed changes
Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

LGTM

test/blackbox-tests/test-cases/melange/aliases.t Show resolved Hide resolved
@rgrinberg rgrinberg added this to the 3.9.0 milestone Jun 11, 2023
@haochenx @anmonteiro
test(melange): melange.emit rules are not part of the default alias
9bc1592 
Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
fix(melange) attach melange.emit targets to the default alias
de9445b 
Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro merged commit c7cf52b into ocaml:main Jun 11, 2023
21 of 22 checks passed
@anmonteiro
Copy link
Collaborator

Thanks!

@haochenx haochenx deleted the melange-default-alias branch June 12, 2023 04:13
haochenx added a commit to haochenx/dune that referenced this pull request Jun 12, 2023
@haochenx
reflect reviewer's comments and changes by ocaml#7926
e672a48 
Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>
anmonteiro added a commit that referenced this pull request Jun 14, 2023
@haochenx @anmonteiro
doc/melange.rst - make the alias attachment behavior of `melange.emit…
157bcb3 
…` clearer (#7924)

* doc/melange.rst - make default target alias clear in introduction

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

* reflect reviewer's comments and changes by #7926

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

* fix formatting

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

* reflect reviewer's comments

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>

* Update doc/melange.rst

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

---------

Signed-off-by: Haochen Kotoi-Xie <hx@kxc.inc>
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@emillon emillon mentioned this pull request Jun 23, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 23, 2023
@emillon
[new release] dune (15 packages) (3.9.0~alpha1)
e84760b 
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
@emillon emillon mentioned this pull request Jun 28, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 28, 2023
@emillon
[new release] dune (15 packages) (3.9.0)
2e2aa14 
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Add `--all` option to `dune rpc status` to show all Dune RPC servers running.
  (ocaml/dune#8011, fix ocaml/dune#7902, @Alizter)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
@jchavarri jchavarri mentioned this pull request Feb 16, 2024
Open
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

@anmonteiro anmonteiro anmonteiro approved these changes

@Alizter Alizter Awaiting requested review from Alizter

Assignees
No one assigned
Labels
melange
Projects
None yet
Milestone
3.9.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@haochenx @anmonteiro @rgrinberg @Alizter