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

maintenance: explicitely declare direct dependencies #4810

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

psafont
Copy link
Member

@psafont psafont commented Oct 5, 2022

Previously a lot of libraries were used but not declared because a dependent library was declared pulled them.

To enable the explicit mode (implicit_transitive_deps false) needs to be added to dune-project. I believe enabling it requires a high bar of knowledge around libraries, so it's best to leave it disabled. For example, the error when not importing declaring dune-build-info when it's needed is indecipherable:

File "ocaml/gencert/.gencert.eobjs/build_info_data.ml-gen", line 1:
Error: Could not find the .cmi file for interface
       ocaml/gencert/.gencert.eobjs/build_info_data.ml-gen.

While dune does not warn of unused libraries, I've removed some unused libraries, like xapi-idl (because only xapi-log was being used), or threads, which is not needed anymore as dune assumes -mt and forces linking against threads.posixanyway. Read https://dune.readthedocs.io/en/stable/dune-files.html?highlight=threads#implicit-transitive-deps-1 and https://dune.readthedocs.io/en/stable/advanced-topics.html?highlight=threads#findlib-integration

For users of the gzip and zstd I've decided to reexport the symbols from the library xapi-compression. I saw little value in forcing users to declare both.

@psafont psafont force-pushed the explicit-deps branch 2 times, most recently from d43ff92 to a5531de Compare October 18, 2022 13:10
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
@lindig
Copy link
Contributor

lindig commented Oct 21, 2022

So now that dependencies are correct, does it not make sense to use (implicit_transitive_deps false) and be alerted when libraries are missing? Otherwise we will be back where we were quickly.

@psafont
Copy link
Member Author

psafont commented Oct 21, 2022

I recommend against that setting, this is because sadly the dune library that provides an ocaml module is often not obvious. While I can guess it by grepping ocamlfind output, this is exceedingly difficult for non-experts.

I understand that it will get out-of-date but I rather have a PR a year updating it until dune includes a helpful error message telling users library xxx provides module yyy, please add it to the list of dependencies in file zzz

@psafont psafont merged commit 3bb0e50 into xapi-project:master Oct 24, 2022
@psafont psafont deleted the explicit-deps branch October 24, 2022 08:54
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.

3 participants