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 melc_flags #6569

Merged
merged 8 commits into from
Nov 30, 2022
Merged

melange: add melc_flags #6569

merged 8 commits into from
Nov 30, 2022

Conversation

jchavarri
Copy link
Collaborator

Fixes #6470.

@jchavarri
melange: add melc_flags
4f3b14a 
Signed-off-by: Javier Chavarri <[email protected]>
@jchavarri jchavarri added the melange Melange rules and generator label Nov 24, 2022
rgrinberg
rgrinberg reviewed Nov 24, 2022
View reviewed changes
src/dune_rules/ocaml_flags.ml Outdated
and+ native = field_oslu "ocamlopt_flags" in
let specific = Mode.Dict.make ~native ~byte in
and+ native = field_oslu "ocamlopt_flags"
and+ melange = field_oslu "melc_flags" in
Copy link
Member

Choose a reason for hiding this comment

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

This should be guarded behind the melange syntax. And the name should be melange.flags or melange.melc_flags

Copy link
Member

Choose a reason for hiding this comment

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

Actually, never mind. We have a strange situation here:

  1. For the library stanza, we want the melange. prefix for the flags
  2. For the emit stanza, this prefix is redundant.

Let me think a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so indeed we should have a melange. prefix in the library field and in the env field. In the emit stanza, we don't need such a prefix. I'd personally vote for the names melange.compile_flags and compile_flags in library & emit respectively.

Copy link
Collaborator Author

@jchavarri jchavarri Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in ad69059 64b6729.

rgrinberg
rgrinberg reviewed Nov 24, 2022
View reviewed changes
test/blackbox-tests/test-cases/melange/ocaml-flags.t
> (target output)
> (entries main)
> (module_system commonjs)
> (ocamlc_flags -w -14-26))
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 think we should even accept this field in melange.emit.

Copy link
Collaborator Author

@jchavarri jchavarri Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in 8043ed0 64b6729.

@rgrinberg
Copy link
Member

One more thing to think about, melange compilation has two stages, (.ml -> .cmj, .cmj -> .js). We should have a way to provide compilation options to both stages.

Finally, we should also add melange support to the env stanza.

@anmonteiro anmonteiro mentioned this pull request Nov 25, 2022
Closed
@anmonteiro
Copy link
Collaborator

anmonteiro commented Nov 25, 2022 •
edited
Loading

We should have a way to provide compilation options to both stages.

I'm not sure that there are currently any interesting flags to be passed to the .cmj -> .js stage. So I don't think this should be a priority right now.

Finally, we should also add melange support to the env stanza.

Opened #6572 to track this.

@jchavarri
Copy link
Collaborator Author

I think I tackled the comments mentioned above. The PR now:

  • Uses melange.compile_flags in the library stanza field (guarded behind the melange syntax)
  • Uses compile_flags in melange.emit stanza field
  • Disallows using ocamlc_flags and ocamlopt_flags in melange.emit stanza

For now there's no support for flags configuration in the .cmj -> .js stage, following @anmonteiro guidance above. Once we see it's useful to add it, we could use emit_flags name, as opposed to compile_flags.

@rgrinberg
Copy link
Member

Some tests need to be updated:

+++ b/_build/.sandbox/5c5d50a72d3c83ee6dba737c279ad17c/default/test/blackbox-tests/test-cases/workspaces/workspace-env.t/run.t.corrected
[79](https://github.com/ocaml/dune/actions/runs/3581799104/jobs/6025279404#step:11:80)
@@ -6,6 +6,7 @@ Workspaces also allow you to set the env for a context:
[80](https://github.com/ocaml/dune/actions/runs/3581799104/jobs/6025279404#step:11:81)
   (ocamlc_flags
[81](https://github.com/ocaml/dune/actions/runs/3581799104/jobs/6025279404#step:11:82)
    (-g -verbose))
[82](https://github.com/ocaml/dune/actions/runs/3581799104/jobs/6025279404#step:11:83)
   (ocamlopt_flags (-g))
[83](https://github.com/ocaml/dune/actions/runs/3581799104/jobs/6025279404#step:11:84)
+  (melange.compile_flags (-g))

@jchavarri
fix tests
c7d0dc0 
Signed-off-by: Javier Chavarri <[email protected]>
@jchavarri
Copy link
Collaborator Author

Oops. Checked the test failures, but saw the whitespace diffs and was too quick to assume everything else was ok. Fixed in c7d0dc0.

@rgrinberg rgrinberg merged commit ebb57f5 into ocaml:main Nov 30, 2022
@jchavarri jchavarri deleted the melange/melc_flags branch November 30, 2022 21:51
moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Dec 2, 2022
@jchavarri @moyodiallo
feature(melange): add compile_flags (ocaml#6569)
dd0c056 
Signed-off-by: Javier Chavarri <[email protected]>
jchavarri added a commit to jchavarri/dune that referenced this pull request Dec 5, 2022
@jchavarri
Merge branch 'main' into melange/non-melange-lib-error
81c1aed 
* main: (54 commits)
  doc: how we write `to_dyn` and `equal` (ocaml#6621)
  test(cache): test output of man pages
  test: dune utop for (subdir ..) (ocaml#6629)
  refactor: improve style of utop rules (ocaml#6628)
  test: depend on utop (ocaml#6627)
  refactor(stdune): Add Appendable_list.cons (ocaml#6624)
  doc: tighten wording in README.md
  test: add a repro case for ocaml#6607 (ocaml#6612)
  doc: cleanup status badges in README.md (ocaml#6618)
  doc(README): rewrap paragraphs and cleanup links
  coq: more resilient config query
  fix: link time code gen (ocaml#6606)
  fix(melange): run melc ppx with merlin (ocaml#6476)
  feature(melange): add compile_flags (ocaml#6569)
  refactor: move `modules: Modules.t` from `Dune_package.Lib` to `Lib_info` (ocaml#6605)
  Set CLICOLOR_FORCE=0 (ocaml#6608)
  fix: declare deps for ccomp detection (ocaml#6610)
  refactor: assume Cmdliner.Arg.conv is abstract (ocaml#6609)
  refactor: gen_rules pattern matching (ocaml#6604)
  Add CI for MSVC using dkml-workflows (ocaml#6540)
  ...
@jchavarri jchavarri mentioned this pull request Dec 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@anmonteiro anmonteiro Awaiting requested review from anmonteiro

Assignees
No one assigned
Labels
melange Melange rules and generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feature(melange): allow to set compiler flags just for melange or for ocaml
3 participants
@jchavarri @rgrinberg @anmonteiro