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

refactor: use the proposed implementation for redirects #12

Conversation

anmonteiro
Copy link

cc @jchavarri @rgrinberg

anmonteiro and others added 8 commits March 20, 2024 14:44
…ocaml#10286)

* refactor: add labeled arguments to `Dep_rules.immediate_deps_of`

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

* fix(melange): track immediate `.cmj` dependents as dependencies of JS rules

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

* test(melange): show outdated paths for include_subdirs and melange.emit

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

* fix(melange): track immediate `.cmj` dependents in `melange.emit` too

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

* chore: add a changelog entry

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

* fix: symlink `.impl.d` files for virtual library modules too

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

---------

Signed-off-by: Antonio Nuno Monteiro <[email protected]>
…s.t during emission (ocaml#10297)

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

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

* fix: read the processed file in Ocamldep.read_immediate_deps_of

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

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

Signed-off-by: Antonio Nuno Monteiro <[email protected]>

---------

Signed-off-by: Antonio Nuno Monteiro <[email protected]>
Copy link
Owner

@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.

Thanks so much for your help, I really appreciate it 🤗 I leave the PR opened so it's easier to review for @rgrinberg, but I would merge it otherwise.

I couldn't understand how to make the suggested Id module API work

I wasn't sure about this either. I started trying to implement it but changing the returning value of find with an option led to multiple changes in the codebase (I prob was applying the change wrongly).

in
if enabled
then Lib.DB.Resolve_result.redirect_in_the_same_db lib
else Lib.DB.Resolve_result.not_found
Copy link
Owner

Choose a reason for hiding this comment

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

@rgrinberg mentioned that in this case we should redirect to the parent db (I can apply this change in a follow up commit)

and enabled = lib.enabled_if in
{ lib_name; enabled }
in
{ loc; new_public_name; old_name; project = lib.project }
Copy link
Owner

Choose a reason for hiding this comment

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

Just an idea for the future, but I wonder if old_name should be renamed to lib_info or target_info or something like that.

Copy link
Author

Choose a reason for hiding this comment

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

I had it old_info in an earlier draft but then I changed it back to avoid polluting the diff.

rgrinberg and others added 2 commits March 24, 2024 21:27
Signed-off-by: Antonio Nuno Monteiro <[email protected]>
@anmonteiro anmonteiro force-pushed the anmonteiro/try-public-libs branch from c1d1c32 to 7434bcb Compare March 25, 2024 08:39
Signed-off-by: Javier Chávarri <[email protected]>
@jchavarri jchavarri force-pushed the anmonteiro/try-public-libs branch from fb00000 to 3eb7e79 Compare March 25, 2024 10:10
jchavarri and others added 6 commits March 25, 2024 11:09
This nests the pages too deeply and requires extra clicks. Instead we
put this directly in the reference hierarchy and will highlight them in
a separate section when using cards.

Signed-off-by: Etienne Millon <[email protected]>
@jchavarri
Copy link
Owner

I have to stop midway through the bigarray fix. One thing I found out and that is strange is that while in main the resolve function in create_from_findlib is called for the case that is failing (dune exec c/c.exe), in this branch, neither find_stanza_id nor resolve get called at all 🤔 But I haven't found yet where these functions should be called from in this case.

@jchavarri
Copy link
Owner

Continues in ocaml#10307.

@jchavarri jchavarri closed this Mar 26, 2024
@jchavarri jchavarri deleted the anmonteiro/try-public-libs branch March 26, 2024 09:31
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.

5 participants