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

fix(melange): track immediate .cmj deps as dependencies of JS rules #10286

Merged
merged 7 commits into from
Mar 21, 2024
Merged

fix(melange): track immediate .cmj deps as dependencies of JS rules #10286

merged 7 commits into from
Mar 21, 2024

Conversation

anmonteiro
Copy link
Collaborator

@anmonteiro anmonteiro commented Mar 20, 2024 •
edited
Loading

fixes #9190

Root cause assessment

  • currently, if foo.ml -> starting_dir/bar.ml and we rebuild after moving bar.ml to end_dir/bar.ml, the foo.cmj file produced in both builds will be byte-by-byte identical.
  • this is a problem because there's a missing dependency foo.js -> (dependencies of foo.cmj) (foo.cmj -> bar.cmj and bar.ml changed)

Discussion & Solution

  • this is only an issue if the rules change between builds, so I believe we only need to account for local builds (i.e. melange.emit entries and local libraries -- we assume that the installed world didn't change)

  • the solution: we add the immediate dependencies of the .cmj we're compiling to JS as Hidden_deps of the JS target rule, which takes care of invalidating the JS builds if a source path has changed

  • This PR is best reviewed commit-by-commit

@rgrinberg I'm requesting your review on this one for the performance impact: we're now calling Dep_rules.immediate_deps_of for every JS target, which in turn calls Ocamldep.read_immediate_deps_of. My understanding is that the latter call is memoized so this change shouldn't have any noticeable impact. Please double check this assumption.

@anmonteiro
refactor: add labeled arguments to Dep_rules.immediate_deps_of
f9c3dc4 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro changed the title Anmonteiro/track immediate dependent cmj paths fix(melange): track immediate .cmj deps as dependencies of JS rules Mar 20, 2024
@anmonteiro anmonteiro mentioned this pull request Mar 20, 2024
Closed
jchavarri
jchavarri reviewed Mar 20, 2024
View reviewed changes
Copy link
Collaborator

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

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

Great work, thanks for the fix!

src/dune_rules/virtual_rules.ml Show resolved Hide resolved
src/dune_rules/melange/melange_rules.ml Outdated Show resolved Hide resolved
src/dune_rules/melange/melange_rules.ml Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

I'm not sure I understand this issue exactly, but the purpose of the additional dependency is only to indirectly depend on the relative path of the module wrt to the library? If that's the case, isn't this a pretty big over-approximation of that?

@anmonteiro
Copy link
Collaborator Author

I'm not sure I understand this issue exactly, but the purpose of the additional dependency is only to indirectly depend on the relative path of the module wrt to the library? If that's the case, isn't this a pretty big over-approximation of that?

I don't think this is an over approximation. Here's my understanding:

  • each melange module imports other modules. the generated JS needs to know where to require them
  • in order to know where to import (because each .cmj records its own path/module system spec), a melange emission step consults its immediate .cmj dependencies
  • therefore, this PR is tracking that immediate dependency at JS emission time, which I believe is correct

@rgrinberg
Copy link
Member

in order to know where to import (because each .cmj records its own path/module system spec), a melange emission step consults its immediate .cmj dependencies

Okay so I got that right, the melange compiler reads these files?

@anmonteiro
Copy link
Collaborator Author

Indeed it does

@anmonteiro
Copy link
Collaborator Author

Can you think of a better approach than this? My understanding is that the cmj dependencies are de facto dependencies of the JS emission step.

@rgrinberg
Copy link
Member

If the melange compiler reads them, then I see no better approach. One should be able to reproduce the bug by turning on the sandboxing for the melange rules. Melange attempt to find some foo.cmj and fail - indicating a missing dependency.

@anmonteiro anmonteiro force-pushed the anmonteiro/track-immediate-dependent-cmj-paths branch from ded8bd5 to fed936f Compare March 20, 2024 23:23
@anmonteiro
fix(melange): track immediate .cmj dependents as dependencies of JS…
ccf34ca 
… rules

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
test(melange): show outdated paths for include_subdirs and melange.emit
bbe3a36 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
fix(melange): track immediate .cmj dependents in melange.emit too
387fe04 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
chore: add a changelog entry
34ade5f 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro
fix: symlink .impl.d files for virtual library modules too
58dd3ba 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/track-immediate-dependent-cmj-paths branch from fed936f to 58dd3ba Compare March 20, 2024 23:27
@anmonteiro
Copy link
Collaborator Author

This should be ready for another look

@anmonteiro anmonteiro mentioned this pull request Mar 20, 2024
Closed
@anmonteiro anmonteiro enabled auto-merge (squash) March 21, 2024 02:58
@anmonteiro anmonteiro merged commit ad457c5 into ocaml:main Mar 21, 2024
1 of 2 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/track-immediate-dependent-cmj-paths branch March 21, 2024 02:58
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Mar 21, 2024
@anmonteiro
fix: fixed after ocaml#10286
df005ab
anmonteiro added a commit to anmonteiro/dune that referenced this pull request Mar 21, 2024
@anmonteiro
fix: fixed after ocaml#10286
b706d73 
Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro mentioned this pull request Mar 22, 2024
Merged
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Mar 26, 2024
Closed
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Mar 26, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.15.0~alpha1)
68b4763 
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
Leonidas-from-XIV added a commit to Leonidas-from-XIV/opam-repository that referenced this pull request Apr 3, 2024
@Leonidas-from-XIV
[new release] dune (15 packages) (3.15.0)
950c529 
CHANGES:

### Added

- Add link flags to to `ocamlmklib` for ctypes stubs (ocaml/dune#8784, @frejsoya)

- Remove some unnecessary limitations in the expansions of percent forms in
  install stanza. For example, the `%{env:..}` form can be used to select files
  to be installed. (ocaml/dune#10160, @rgrinberg)

- Allow artifact expansion percent forms (`%{cma:..}`, `%{cmo:..}`, etc.) in
  more contexts. Previously, they would be randomly forbidden in some fields.
  (ocaml/dune#10169, @rgrinberg)

- Allow `%{inline_tests}` in more contexts (ocaml/dune#10191, @rgrinberg)

- Remove limitations on percent forms in the `(enabled_if ..)` field of
  libraries (ocaml/dune#10250, @rgrinberg)

- Support dialects in `dune describe pp` (ocaml/dune#10283, @emillon)

- Allow defining executables or melange emit stanzas with the same name in the
  same folder under different contexts. (ocaml/dune#10220, @rgrinberg, @jchavarri)

### Fixed

- coq: Delay Coq rule setup checks so OCaml-only packages can build in hybrid
  Coq/OCaml projects when `coqc` is not present. Thanks to @vzaliva for the
  test case and report (ocaml/dune#9845, fixes ocaml/dune#9818, @rgrinberg, @ejgallego)

- Fix conditional source selection with `select` on `bigarray` in OCaml 5
  (ocaml/dune#10011, @moyodiallo)

- melange: fix inconsistency in virtual library implementation. Concrete
  modules within a virtual library can now refer to its virtual modules too
  (ocaml/dune#10051, fixes ocaml/dune#7104, @anmonteiro)

- melange: fix a bug that would cause stale `import` paths to be emitted when
  moving source files within `(include_subdirs ..)` (ocaml/dune#10286, fixes ocaml/dune#9190,
  @anmonteiro)

- Dune file formatting: output utf8 if input is correctly encoded (ocaml/dune#10113,
  fixes ocaml/dune#9728, @moyodiallo)

- Fix expanding dependencies and locks specified in the cram stanza.
  Previously, they would be installed in the context of the cram test, rather
  than the cram stanza itself (ocaml/dune#10165, @rgrinberg)

- Fix bug with `dune exec --watch` where the working directory would always be
  set to the project root rather than the directory where the command was run
  (ocaml/dune#10262, @gridbugs)

- Regression fix: sign executables that are promoted into the source tree
  (ocaml/dune#10263, fixes ocaml/dune#9272, @emillon)

- Fix crash when decoding dune-package for libraries with `(include_subdirs
  qualified)` (ocaml/dune#10269, fixes ocaml/dune#10264, @emillon)

### Changed

- Remove the `--react-to-insignificant-changes` option. (ocaml/dune#10083, @rgrinberg)
@Leonidas-from-XIV Leonidas-from-XIV mentioned this pull request Apr 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchavarri jchavarri jchavarri approved these changes

@rgrinberg rgrinberg rgrinberg approved these changes

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

Successfully merging this pull request may close these issues.

Melange: moving modules inside a library with include_subdirs leads to broken paths
3 participants
@anmonteiro @rgrinberg @jchavarri