Conversation
Prior to this commit the link phase was only using the include paths coming from `compile-deps` on the unit being linked. The fix is to use the library dependencies coming from an external source - META files or dune metadata. We can't simply use the union of the compile-deps paths from all units as we might still miss some dependencies due to `--no-alias-deps`.
This ensures all units in the 'library' paths are accessible without adding the /libname to the reference
jonludlam
added
the
no changelog
panglesd
left a comment
panglesd
left a comment
There was a problem hiding this comment.
Nice, carefulness is much better than the over-approximation that did not follow the convention!
| Fpath.(path / Printf.sprintf ".%s.objs" libname / "byte") | ||
| in | ||
| let all_lib_deps = Util.StringMap.empty in | ||
| (* TODO *) |
There was a problem hiding this comment.
Do you intend to implement that for voodoo and dune in this PR too?
There was a problem hiding this comment.
I was putting it off, it's a bit of a PITA, especially for the dune mode!
| let index_dir = match index_dir with None -> output_dir | Some dir -> dir in | ||
| (* This isn't a hashtable, but a table of hashes! Yay! *) | ||
| let hashtable = | ||
| let hashtable, lib_dirs = |
There was a problem hiding this comment.
Since we are using StringMap quite a lot, why not use that instead of an associative list?
| let name = mld_path |> Fpath.rem_ext |> Fpath.basename |> ( ^ ) "page-" in | ||
| let unit = | ||
| make_unit ~name ~kind ~rel_dir ~input_file:mld_path ~pkg ~include_dirs | ||
| ~lib_deps:[] |
There was a problem hiding this comment.
Is there anything bigger but still sensible we could use here?
- We likely cannot add the union of the the
lib_depsfrom the libraries of the package: I think there might be different deps with the same name? (not sure here).
I know we will provide a way to extend this, but having a bigger default would be nice!
There was a problem hiding this comment.
I think the libraries in the package is what we decided before.
| current_dir; | ||
| } | ||
| in | ||
| let directories = directories @ List.map ~f:snd lib_roots in |
There was a problem hiding this comment.
I was about to say that this requires an update to the man for the -L option, but it was actually already saying that!
So whenever we can write {!/libname/Module} we can also write {!Module}.
There was a problem hiding this comment.
I wasn't sure whether adding the paths to the include path was the best approach, or whether we should do something else elsewhere?
|
Superseded by #1208 |
2 participants
Make sure the set of libraries provided during linking only comes from the dependency libraries of the library being linked.
Also ensure that the modules in the libraries passed are in the normal include path.