-
Notifications
You must be signed in to change notification settings - Fork 87
Make visible and hidden libloc files separate #3974
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other executables such as the ones in extract_externals/extract_externals.ml
and toplevel
may need to be updated. Maybe search for all the place where include_dirs
and hidden_include_dirs
are used, if you haven't yet.
\ compiler know which libraries should be accessible via this `.libloc`\n\ | ||
\ directory. Difference between <libs> and <hidden_libs> is the same as\n\ | ||
\ the difference between -I and -H flags" | ||
let mk_I_paths f = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the motivation for the proposed format, in particular, do we have to separate basename
and path
? It'd be much simpler and more intuitvite to have each line of the input file as a single path. Then we could update the doc to something like this:
-I-paths
<file> Add each line of <file> to the list of paths that the compiler can reference.
Similar to -I, but specifies individual files instead of adding the whole directory.
-H-paths
<file> Same as -I-paths, but adds to the list of "hidden" paths (like -H).
\ <path>. If <path> is relative, then it is relative to a parent directory\n\ | ||
\ of <file>." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is still confusing, it was confusing in the oridingal version too. Is it avoidable? It would be nice to match the logic of -I <dir>
.
let path = if Filename.is_relative path then | ||
(* Paths are relative to parent directory of libloc directory *) | ||
Filename.concat (Filename.dirname libloc) path | ||
(* Paths are relative to parent directory of path list file *) | ||
Filename.concat (Filename.dirname path_list_file) path | ||
else | ||
path | ||
in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this logic into read_path_list_file
.
List.iter (fun path_list_file -> | ||
visible_dirs := Dir.create_from_path_list_file ~hidden:false ~path_list_file :: !visible_dirs; | ||
) !Clflags.include_paths_files; | ||
List.iter (fun path_list_file -> | ||
hidden_dirs := Dir.create_from_path_list_file ~hidden:true ~path_list_file :: !hidden_dirs; | ||
) !Clflags.hidden_include_paths_files; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not symmetric to the use of visible
and hidden
paramters, should include_paths_files
and hidden_include_paths_files
also be parameters?
Change libloc arguments from
-libloc <dir>:<libs>:<hidden_libs>
to-I-paths <file>
and-H-paths <file>
. This removes hardcoding libloc directory structure from compiler and makes it more similar to-I
and-H
files. We also removelibloc
naming completely to make it simpler.This change is not backwards compatible, but this is completely fine and expected. We don't use libloc anywhere in production yet.