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

[2/3] Restructure LIBDIR: Remove duplicate topdirs.cmi #11199

Merged
merged 3 commits into from May 6, 2022

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Apr 16, 2022

This is the second PR in a series starting with #11198 which seek to separate the components in OCaml's $LIBDIR.

In OCaml 4.00.0, the compiler's libraries started to be installed in $LIBDIR/compiler-libs. This created a small conundrum for topdirs.cmi as the toplevel and the debugger both require this signature to be available.

Curiously, the debugger and toplevel were fixed in different ways. In #5647, the debugger directly enriches the environment with topdirs.cmi read from compiler-libs (see fa0e0b6). This avoids having to add the whole of +compiler-libs to the load path. However, 10 days later the toplevel was fixed with a slightly different sledgehammer in #5691 by putting a second copy of topdirs.cmi into $LIBDIR, keeping the copy which used to be there in 3.12 and earlier (f786ea8). #827 somewhat needlessly duplicates topdirs.cmt, topdirs.cmti and topdirs.mli in $LIBDIR as well.

The duplication of these files are in themselves something a smell (in fact, the presence of two identically named .cmi files upsets ocamlfind). This PR changes the toplevel to adopt the debugger's approach and enriches the environment with topdirs.cmi loaded from +compiler-libs.

This is done immediately after Compmisc.init_path for which are three cases:

  1. Running a script (ocaml script.ml or ocamlnat script.ml) in Toploop.run_script
  2. Running an interactive toplevel, in Topmain.main
  3. In the curiously obscure Topeval.init which is called at the initialisation of Toploop. This, as the comment in toploop.ml explains, is necessary or code linked together with ocamlmktop can't use Topdirs.dir_install_printer

At present, this PR requires a small patch to utop (to call Topcommon.load_topdirs_signature) and, when it's updated for OCaml 5, Coq will require a similar patch for the Drop. instruction in coqtop.byte.

It's tempting to move Compmisc.init_path out of Topmain.main and into Toploop.loop, and so also put the Topcommon.load_topdirs_signature there too. This would eliminate the need for patching Coq, certainly, but the code in utop serves as a reminder of why this (I think) is not a sound course of action: utop, for example, will drop into Toploop.loop only on some code paths, so where it has (or at least should have) setup up the paths itself - and it would still need to call Topcommon.load_topdirs_signature for the other code paths which don't go via the toplevel's own interactive loop.

This PR includes two earlier commits: the first simply adds +toplevel and gets the toplevel to include that directory. Simple, but it doesn't fix the smell! The second commit assumes that topdirs.cmi will be in the load path and falls back to loading it manually otherwise and then the final commit goes all-in on the manual enrichment approach.

The effect of this PR on the installation directory is that topdirs.cmi, topdirs.cmt, topdirs.cmti and topdirs.mli are no longer installed to $LIBDIR. This change has no visible check in opam-health-check on OCaml 5.0 packages. A version of it in OCaml 4.14 revealed some problems with older packages, but all of those packages already require updating for OCaml 5.0.

@dra27
Copy link
Member Author

dra27 commented Apr 16, 2022

(temporarily adding the no-change-entry-needed label just to clear the hygiene check for now)

@shindere
Copy link
Contributor

This PR looks very good to me, many thanks.

One general comment about the precautions we take to remove left-overs
of installations of previous versions (not to be addressed here).
If it is considered these precautions are really necessary, how about
introducing a pre-install target on which install would depend
and which would take care of those?

dra27 added 3 commits May 5, 2022 19:58
If Topdirs is not found in the search path, attempt to load topdirs.cmi
directly from +compiler-libs before aborting.
@dra27
Copy link
Member Author

dra27 commented May 5, 2022

Thanks, @shindere!

One general comment about the precautions we take to remove left-overs of installations of previous versions (not to be addressed here). If it is considered these precautions are really necessary, how about introducing a pre-install target on which install would depend and which would take care of those?

That sounds like a good idea.

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.

None yet

2 participants