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

Driver: Read a config file to apply the right -P and -L. #1194

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

panglesd
Copy link
Collaborator

This PR makes the driver follow a convention to know which packages/libraries to include in -P/-L during linking, for opam-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 in packages 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:

  • a -P for each package listed
  • a -L for each library of each package listed

This 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!

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.

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 }
Copy link
Collaborator

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.

@panglesd panglesd added the no changelog This pull request does not need a changelog entry label Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants