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.

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

fix(melange): account for preprocessing when getting library's Modules.t during emission #10297

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Mar 22, 2024

  • this is a regression introduced in fix(melange): track immediate .cmj deps as dependencies of JS rules #10286, caused by reading immediate_deps_of the current cmj file
  • Ocamldep.read_immediate_deps_of needs to account for the fact that the file may have been pre-processed by a dialect, and use the final filename instead.
  • we need to account for preprocessing (which changes the module target filenames in the build directory) when getting the Modules.t structure where we consult .impl.d files
  • this diff is best reviewed without whitespace

the error can be seen in the first commit:

  Error: ocamldep returned unexpected output for _build/default/lib/foo.myd:
  > lib/foo.myd.ml: Bar
  -> required by _build/default/output/lib/foo.js
  -> required by alias mel
  [1]

@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from 8a5b9f9 to 0d4c945 Compare March 22, 2024 00:11
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from 0d4c945 to 18fc55a Compare March 22, 2024 00:12
@anmonteiro anmonteiro requested a review from rgrinberg March 22, 2024 00:36
@anmonteiro anmonteiro changed the title test: show melange.emit error with dialects fix: read the dependency section for the correct filename in Ocamldep.read_immediate_deps_of Mar 22, 2024
@anmonteiro anmonteiro requested a review from jchavarri March 22, 2024 00:38
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from 0003a37 to 1fa5da7 Compare March 22, 2024 00:40
@anmonteiro anmonteiro changed the title fix: read the dependency section for the correct filename in Ocamldep.read_immediate_deps_of fix: read dependency section for the correct filename in Ocamldep.read_immediate_deps_of Mar 22, 2024
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from 986a7ca to 5791466 Compare March 22, 2024 01:34
…result

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from 5791466 to 6715575 Compare March 22, 2024 02:44
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from 6715575 to a5b6c5a Compare March 22, 2024 02:54
@anmonteiro
Copy link
Collaborator Author

I tested this in a real world project impacted by the regression and it fixed the issue too.

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/melange-error-ocamldep-with-dialect branch from c26c83f to 70c757d Compare March 22, 2024 03:51
@anmonteiro anmonteiro changed the title fix: read dependency section for the correct filename in Ocamldep.read_immediate_deps_of fix(melange): account for preprocessing when getting library's Modules.t during emission Mar 22, 2024
src/dune_rules/melange/melange_rules.ml Show resolved Hide resolved
src/dune_rules/melange/melange_rules.ml Show resolved Hide resolved
src/dune_rules/melange/melange_rules.ml Show resolved Hide resolved
@anmonteiro anmonteiro enabled auto-merge (squash) March 22, 2024 21:41
@anmonteiro anmonteiro disabled auto-merge March 22, 2024 21:41
@anmonteiro anmonteiro merged commit f9950b2 into ocaml:main Mar 22, 2024
26 of 27 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/melange-error-ocamldep-with-dialect branch March 22, 2024 22:37
pmwhite pushed a commit to pmwhite/dune that referenced this pull request Apr 2, 2024
…s.t during emission (ocaml#10297)

* test: show melange.emit regression attempting to read wrong ocamldep result

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

* fix: read the processed file in Ocamldep.read_immediate_deps_of

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

* fix(virtual_lib_compilation_test): only add dependency if impl exists

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

---------

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants