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

Monorepo mode fixes #1276

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Monorepo mode fixes #1276

merged 7 commits into from
Jan 16, 2025

Conversation

jonludlam
Copy link
Member

No description provided.

@jonludlam
Copy link
Member Author

Built on top of #1275

Copy link
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

This needs to be formatted.

String.sub s 0 (String.length s - 4)
else s
in
root ^ ".html"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we have a link to foo.png, will this output foo.png.html ?

match Option.map Fpath.segs p' with
| Some (objdir :: _ :: _) -> (
match Astring.String.fields ~is_sep:(fun c -> c = '.') objdir with
| [ ""; libname; _ ] -> Some libname
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could assert that we are looking at the right directory:

Suggested change
| [ ""; libname; _ ] -> Some libname
| [ ""; libname; "objs" ] -> Some libname

| p :: _ -> (
let p' = Fpath.relativize ~root:(Fpath.v l.source_dir) (Fpath.v p) in
match Option.map Fpath.segs p' with
| Some (objdir :: _ :: _) -> (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does adding this comment make sense?

Suggested change
| Some (objdir :: _ :: _) -> (
(* cmt files are expected to be in [library_path/.libname.objs/byte/name.cmt]. *)
| Some (objdir :: _ :: _) -> (

match internal_name_of_library lib with
| None -> None
| Some libname ->
let cmtidir =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This cmtidir is already computed inside internal_name_of_library and could be removed to avoid constructing the path again potentially differently.

in
let id_override =
Fpath.relativize
~root:Fpath.(v "_build/default")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this not ~root ?

Copy link
Member Author

Choose a reason for hiding this comment

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

~root has dir prepended:

let root = Fpath.(dir / "_build" / "default") in

|> Option.map Fpath.to_string
in
Logs.debug (fun m -> m "this should never be 'None': %a" Fmt.Dump.(option string) id_override);
if List.mem cmtidir c then
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is checking that cmtidir was actually built ? Perhaps c should be a set to avoid the loop in the loop ?

Comment on lines 285 to 280
if pkg.name = "pkg" then
let others :> t list = Landing_pages.make_custom dirs index_of (List.find (fun p -> p.Packages.name = "pkg") pkgs) in
others @ List.concat
(mld_units @ asset_units @ md_units @ lib_units)
else
List.concat
((pkg_index () :: src_index () :: lib_units)
@ mld_units @ asset_units @ md_units)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When pkg.name = "pkg" is false, this returns the same thing as in the Normal or Voodoo case. The code could be made clearer.

type indices_style =
| Voodoo
| Normal
| Custom
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Custom mode generates index pages in every places it needs. Could it be called Automatic ?

@jonludlam jonludlam force-pushed the monorepo-mode-fixes branch from 6887d13 to 09d16f1 Compare January 15, 2025 17:31
- library dependencies need transitive deps too
- added the ability to specify extra pkgs/libs (like odoc-config.sexp)
- minor improvements to landing pages
- pick up jpgs as assets
@jonludlam jonludlam force-pushed the monorepo-mode-fixes branch from 97c50f4 to 94e47d7 Compare January 16, 2025 15:28
@jonludlam jonludlam merged commit a161f9a into ocaml:master Jan 16, 2025
12 of 14 checks passed
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.

2 participants