-
Notifications
You must be signed in to change notification settings - Fork 89
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
Driver: Read a config file to apply the right -P
and -L
.
#1194
base: master
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.
Looks good.
let { pages = config_pages; libs = config_libs } = | ||
pkg_args_of_config pkg.Packages.config | ||
in | ||
{ pages = own_page @ config_pages; libs = own_libs @ config_libs } |
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.
These will be used for both pages and modules ? I think that's fine. My initial idea was that the new dependencies will be specified on the (documentation)
stanza and it might be weird that they also affect modules. It seems that this will be defined in dune-project
instead, which is fine.
8a05685
to
8783452
Compare
I could not make it run in cram tests: it has to be tested by hand.
It needs to be run manually, I could not make it run with cram tests.
15b7be2
to
bd9df7f
Compare
This PR makes the driver follow a convention to know which packages/libraries to include in
-P
/-L
during linking, foropam
-installed libraries. (This convention is what I understood that we proposed in various dev meetings. However, I have a slightly different proposition, see at the end of the post).The file has to be at path
<opam_root>/doc/<pkgname>/odoc-config.sexp
.It has to be a valid sequence of whitespace-separated s-expression. Each s-expression must be of the form
'(libraries <atom list>)
or(packages <atom list>)
.The atoms present in each field correspond respectively to the
-L
and-P
to add to the linking of the pages and modules of the package. That is, atoms in'library
must be library names, while atoms inpackages
must be package names.I tried to add cram tests for the driver, but failed miserably. I thus included instructions on how to run tests.
I think specifying pages at a package level, and libraries at a library level, is confusing for the user and does not add much value.
I would propose to have a single
(deps <package list>)
stanza which adds:-P
for each package listed-L
for each library of each package listedThis is less "fine-grained" but less confusing I think. Let me know if there is some advantages to being able to specify dependencies at the library granularity!