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

refactor: move modules: Modules.t from Dune_package.Lib to Lib_info #6605

Merged
merged 3 commits into from
Nov 30, 2022
Merged

refactor: move modules: Modules.t from Dune_package.Lib to Lib_info #6605

merged 3 commits into from
Nov 30, 2022

Conversation

anmonteiro
Copy link
Collaborator

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

@anmonteiro anmonteiro marked this pull request as ready for review November 30, 2022 05:49
anmonteiro
anmonteiro commented Nov 30, 2022
View reviewed changes
src/dune_rules/lib.ml Outdated Show resolved Hide resolved
anmonteiro
anmonteiro commented Nov 30, 2022
View reviewed changes
src/dune_rules/lib.ml
@@ -1945,7 +1945,7 @@ let to_dune_lib ({ info; _ } as lib) ~modules ~foreign_objects ~dir :
~foreign_objects ~obj_dir ~implements ~default_implementation ~sub_systems
~modules
in
Dune_package.Lib.of_dune_lib ~info ~modules ~main_module_name
Dune_package.Lib.of_dune_lib ~info ~main_module_name
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

callout: in this function to_dune_lib, we get ~modules as an argument and ignore modules from info.

I suspect it's correct, since info.modules will always probably be Local, but wanted to call out anyway.

@anmonteiro anmonteiro changed the title chore: move modules: Modules.t from Dune_package.Lib to Lib_info refactor: move modules: Modules.t from Dune_package.Lib to Lib_info Nov 30, 2022
anmonteiro
anmonteiro commented Nov 30, 2022
View reviewed changes
src/dune_rules/dir_contents.ml Outdated
match Lib_info.modules info with
| External modules -> Memo.return (Some modules)
| Local -> (
match Path.as_in_build_dir dir with
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should I be using Path.as_in_build_dir_exn here?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. The answer is no because it will fail for findlib libraries. We need to improve the types per my comment #6605 (comment) or Ali's suggestion #6605 (comment)

@anmonteiro anmonteiro mentioned this pull request Nov 30, 2022
Merged
rgrinberg
rgrinberg reviewed Nov 30, 2022
View reviewed changes
src/dune_rules/lib_info.mli Outdated
@@ -150,6 +150,8 @@ val special_builtin_support : _ t -> Special_builtin_support.t option

val modes : _ t -> Lib_mode.Map.Set.t

val modules : _ t -> Modules.t Source.t
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this type isn't quite correct actually. it should really be:

type t =
  | Local
  | Unavailable
  | Present of Modules.t

Since findlib modules do not have this information.

@Alizter
Copy link
Collaborator

Alizter commented Nov 30, 2022

I have a similar change in #6489, there I chose: Modules.t option Source.t.

@anmonteiro
Copy link
Collaborator Author

Pushed a1f0b46 changing to Modules.t option Source.t as in #6489.

@anmonteiro anmonteiro force-pushed the anmonteiro/modules-in-lib-info branch from a1f0b46 to a8c22c7 Compare November 30, 2022 17:16
@rgrinberg
Copy link
Member

Now we can use Path.as_in_build_dir_exn

@rgrinberg rgrinberg merged commit 4a1da01 into ocaml:main Nov 30, 2022
@anmonteiro anmonteiro deleted the anmonteiro/modules-in-lib-info branch November 30, 2022 19:30
moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Dec 2, 2022
@anmonteiro @moyodiallo
refactor: move modules: Modules.t from Dune_package.Lib to `Lib_i…
9943f5e 
…nfo` (ocaml#6605)

Signed-off-by: Antonio Nuno Monteiro <[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)
  ...
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

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

Successfully merging this pull request may close these issues.
3 participants
@anmonteiro @Alizter @rgrinberg