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(ml-sources): check that a library is available before tracking it #10355

Conversation

anmonteiro
Copy link
Collaborator

Since #10307, dune started allowing libraries to share the same name.

Thus, checking enabled_if isn't enough for libraries; dune now needs to check whether the library is available before tracking it in ml_sources.

addresses #10307 (comment)

@anmonteiro anmonteiro force-pushed the anmonteiro/ml-sources-handle-optional-libraries branch 2 times, most recently from bed7faa to c660244 Compare April 2, 2024 07:03
@anmonteiro
Copy link
Collaborator Author

This diff is way better without whitespace https://github.com/ocaml/dune/pull/10355/files?w=1

@rgrinberg
Copy link
Member

One issue with this PR is that it's introducing a library database look up before we're able to resolve the sources of a library. That now has the potential for cycles. E.g:

(rule
 (with-stdout-to foo "m_%{lib-available:bar"))
(library
 (name foo)
 (modules %{read:foo}))

It would be better to delay this availability check until the source is needed by somebody

@anmonteiro
Copy link
Collaborator Author

One issue with this PR is that it's introducing a library database look up before we're able to resolve the sources of a library. That now has the potential for cycles. E.g:

(rule
 (with-stdout-to foo "m_%{lib-available:bar"))
(library
 (name foo)
 (modules %{read:foo}))

It would be better to delay this availability check until the source is needed by somebody

This cycle is present even if I revert my diff. Are you suggesting that we're adding to it rather than causing it?

@anmonteiro anmonteiro force-pushed the anmonteiro/ml-sources-handle-optional-libraries branch from c660244 to cea6fcc Compare April 3, 2024 00:47
@anmonteiro
Copy link
Collaborator Author

I changed the implementation to delay querying Lib.DB.available_by_id until we query for modules_and_obj_dir. The implementation is fairly minimal, but it makes modules_and_obj_dir return a Memo.t and an extra Lib.DB.t argument, which make the diff a bit larger.

@anmonteiro anmonteiro force-pushed the anmonteiro/ml-sources-handle-optional-libraries branch 5 times, most recently from 55d7708 to 0a42bd7 Compare April 4, 2024 21:49
@anmonteiro
Copy link
Collaborator Author

@rgrinberg I think this is still waiting for another look.

@rgrinberg
Copy link
Member

I'll try and take a look again this week

@anmonteiro anmonteiro force-pushed the anmonteiro/ml-sources-handle-optional-libraries branch 2 times, most recently from c1766de to 027131a Compare April 12, 2024 22:18
Since ocaml#10307, dune started allowing libraries to share the same name.
Thus, checking `enabled_if` isn't enough for libraries; dune now needs
to check whether the library is available before tracking it in
ml_sources

Signed-off-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@anmonteiro anmonteiro force-pushed the anmonteiro/ml-sources-handle-optional-libraries branch from 027131a to e1f08ed Compare April 15, 2024 20:46
@anmonteiro anmonteiro merged commit 2a0d4e8 into ocaml:main Apr 15, 2024
26 of 27 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/ml-sources-handle-optional-libraries branch April 15, 2024 21:03
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.

3 participants