-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Prefix compiler-libs, Take 2 #9694
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.
This is an impressive amount of work. Thanks for the very detailed description!
I had a few naive questions while reading the code. Toplevel remarks:
-
I wish we spent more time working on "internal suffixes" for compiler unit names, to support clashes of module names, instead of spending this much effort using module aliases as an ugly workaround. (This is not a criticism of the present work, and indeed it is useful today.)
-
Re
Ocamlcommon
vs.Ocaml_common
, I like the fact thatOcaml_common
makes the approach compatible to Janestreet's prefixed re-export, https://github.com/janestreet/ocaml-compiler-libs. (In particular, people could write code with explicit prefixing and use ocaml-compiler-libs for backward compatibility with older compiler releases.) -
I'm not excited by the shiny new
awk
andsed
scripts. -
You mention a message of Jacques that explained the best approach available before
ocamldep
grew support for module aliases, in the form of-open
and-as-map
(I implemented a debugger-specific use of it in Prefix ocamldebug modules to minimize clashes #9621). Did you try using those? Is there a specific reason for doing it the old-style way?
* expect | ||
*) | ||
[@@@alert "-deprecated"] | ||
|
||
module L = Longident | ||
|
||
[%%expect {| | ||
module L = Longident | ||
module L = Ocamlcommon.Longident |
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.
Why is the output modified here? (Would passing -short-paths
avoid this?)
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.
Good question, I'm not sure. I will experiment a bit and report back.
@@ -1,6 +1,5 @@ | |||
(* TEST | |||
flags = "-I ${ocamlsrcdir}/parsing -I ${ocamlsrcdir}/toplevel" | |||
include ocamlcommon | |||
flags = "-I ${ocamlsrcdir}/parsing -I ${ocamlsrcdir}/compilerlibs -open Ocamlcommon" |
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.
Why did you need a change here? I thought that include ocamlcommon
would imply -I +compilerlibs -open Ocamlcommon
?
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.
Because of the way * expect
is currently implemented, it doesn't fully respect up the include ocamlcommon
modifier. It could probably be fixed, but I did not want to increase the diff in this PR too much.
compilerlibs/Makefile.compilerlibs
Outdated
# bytecomp/ocamlcommon_meta.cmx: bytecomp/meta.ml | ||
# $(CAMLC) $(COMPFLAGS) $(OPTCOMPFLAGS) -o $@ -c $< | ||
# | ||
# compilerlibs/meta.ml: compilerlibs/Makefile.compilerlibs |
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 hsould be bytecomp/meta.ml
, right?
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.
I don't think so: this is the compatibility/deprecation shim for bytecomp/meta.ml
and it is generated inside compilerlibs
.
# the following rules: | ||
# | ||
# bytecomp/ocamlcommon__meta.cmo: bytecomp/meta.ml | ||
# $(CAMLC) $(COMPFLAGS) -o $@ -c $< |
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.
I don't understand why this does not depend on a .cmi
file. Is the idea that the dependency is provided in the .depend
file? (A comment could help)
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.
That's exactly it. I will add a comment explaining this point.
That's a fair argument. On the other hand, I wonder why they chose that name to begin with; those tiny differences between module and library names are a headache to deal with.
Indeed. There are three new scripts in this PR:
From these, 1. supposedly would be going away soon-ish, so I'm not very concerned about. 3. is only used when doing As for 2., it is used during compilation. In principle it could be an OCaml script, but any bootstrapping issues need to be examined (at first sight it seems it should work). Another possibility (just brainstorming) would be to commit these map files into the repository and define the variables in I will investigate rewriting the scripts in OCaml and report back.
Yes, I did :) The issue is that
|
Two quick remarks:
Why not add this feature to ocamldep, then?
If we're willing to put up with the headache, why not make it look nicer and have the same prefix (say OCamlInternals or OCamlCompiler or maybe even just OCaml) for all modules of all compiler-libs? That these modules are grouped in 5 libraries is an implementation detail, not something that should necessarily be reflected in the sources of client modules. |
I haven't looked at what is involved. The original PR #286 had such a feature originally, but it was removed before merging due to a lack of consensus. Maybe this PR is a good time to revisit the issue.
I agree with having a toplevel |
Concerning the documentation, I would not worry too much on long term compatibility with ocamldoc: odoc has now a man page backend (ocaml/odoc#428) and I have a prototype of a latex backend that can build the full standard library. Thus, It seems quite possible that we could switch to building the compiler-libs documentation with |
0f1be66
to
472d154
Compare
I pushed three new commits:
On the other hand, maybe I haven't updated the issue description nor the tests. I will wait for the discussion to converge to do that. |
Nice work! With regards to naming, I would slightly weight in favor of OCamlInternals or OCamlCompiler rather than just OCaml, just to make it very clear that this is not part of the standard library. |
I renamed the top-level module |
Following discussion in the developer's meeting and in the spirit of trying to exercise a bit of PR discipline, I'm shutting down this one down, since it is not clear that it has a viable path forward. |
This is Take 2 of my previous attempt to prefix
compiler-libs
, #2218. Building on what was done in #9211, the present PR is simpler, and shorter, than the previous attempt.The PR is ready for review. It can be read commit-by-commit (but keep in mind that due to the nature of the PR, the intermediate commits are not fully compilable -- most often due to missing dependency information). Note that the PR has already been submitted to
precheck
, which it passes successfully.To get a sense of what the PR is doing, you can see a diff in the list of installed files here and a log of the build of the compiler here.
What?
We modify the compilation scheme of
compiler-libs
so as to nest every one of its modules within just a few top-level modules, corresponding to the existing libraries:Ocamlcommon
Ocamlbytecomp
Ocamlmiddleend
andOcamloptcomp
Ocamltoplevel
andOcamlopttoplevel
(libraries on the same line share common modules.)
For example,
Typecore
becomesOcamlcommon.Typecore
,Arch
becomesOcamloptcomp.Arch
, etc.The main advantage is that the global scope is no longer (as) polluted.
How?
Using module aliases.
The approach is the one explained in this message by @garrigue:
-open
to compile using the "short" names, and-o
to produce unit names with "long" names, andocamldep
to rewrite "short" names into "long" names.Below we give more details.
Per-file compilation rules
For each module
compiler-libs
we generate a compilation rule using-o
targetting a unit name with the corresponding prefix. For example, takingbytecomp/meta
as a running example, it is included in the libraryocamlbytecomp
and the prefix isOcamlbytecomp__
. For this file, we will generate a compilation ruleNote that some modules are included in more than one library: this is the case notably for the modules in
ocamlmiddleend
, which are also included inocamloptcomp
. In that case, we simply made a choice of prefix (in that case,ocamloptcomp__
was chosen).The rules for each file are generated using a
make
macro, from the variables defined inMakefile.compilerlibs
.Map files
In order to map "short" (unprefixed) names to "long" (prefixed) names we generate "map" files (one per library) of the form
These files are generated in
compilerlibs/
using anawk
script, again based on the variables ofMakefile.compilerlibs
.The map files are compiled with
-no-alias-deps -w -49
, do not depend on any other module and every other module depends on them.Compilation flags
In order to avoid modifying the source files and to keep the existing workflow, the following compilation flags are added to
COMPFLAGS
.-open Ocamlcommon -open Ocamlbytecomp -open Ocamloptcomp -open Ocamtoplevel -open Ocamlopttoplevel
-no-alias-deps
.and
INCLUDES
gets modified by adding-I compilerlibs
as this is necessary so that map files above can be found.
Postprocessing of
.depend
As
ocamldep
does not have support for prefixing, we postprocess its output to add the prefixes.The postprocessing is done by a
sed
script which is itself automatically generated. Concretely, when setting up compilation rules for each module a scriptcompilerlibs/<library name>.sed
is generated that takes care of rewriting, say,bytecomp/meta.cmi
tobytecomp/ocamlbytecomp__meta.cmi
in.depend
.The same script used to post-process
.depend
forcompiler-libs
is used fortools
,debugger
,ocamldoc
, etc.Backwards compatibility
Switching to a prefixed version of
compiler-libs
will break all existing code using it. The fix is fairly simple (add some-open
flags), but we would like to do better. For that, in addition to the prefixing the libraries as above, we generate, for each module, a compatibility shim of the form:These modules are also linked in the corresponding libraries, so that users can continue to use the unprefixed modules (with a deprecation warning). As
compiler-libs
does not have an official policy of backwards-compatibility, this shim could be removed within one release of the compiler.Topdirs
There is one module in
compiler-libs
which could be considered to be part of the "public API" of the compiler:Topdirs
. This module aids interaction with the toplevel, and is used in different ways (byfindlib
, the different toplevels, the debugger, user.ocamlinit
scripts, etc.) Some of those uses we control and we could update to use a "prefixed"Topdirs
, but others are not within our control and chaning the name of the compilation unit would be breaking change.For this reason,
Topdirs
andOpttopdirs
(its analog forocamlnat
) are left unprefixed, preserving backwards compatibility.A variable
DO_NOT_PREFIX
inMakefile.compilerlibs
to used to control which modules are left unprefixed.ocamldoc for compiler-libs
ocamldoc
does not handle prefixed libraries very well; in this PR the documentation generated forcompiler-libs
is still good thanks to the compatibility shims. But once those shims are gone, the documentation forcompiler-libs
will not work anymore. I see a couple of options:Stdlib
inocamldoc
tocompiler-libs
compiler-libs
Performance
As the build system does more work now, it is important that performance is not overly affected.
Below are some informal timings for
make -j1
(starting from clean repository, just after./configure
) in my machineThat is, the compatibility shims in this PR add about 15% compilation when compiling from scratch with
-j1
(the worst case), which is reasonable since there are double the number of files to compile and as many number of calls to an external script to generate the shims in the first place. This price is only paid when compiling from scratch, as afterwards the shims are already generated and recompiled incrementally, as needed. It doesn't seem too bad for a feature that will be removed in the short-ish term.Outstanding naming issues
Ocamlcommon
vsOcaml_common
? I chose the first one, as it matches the library name.+compiler-libs
, as before. But we may want to take advantage of the situation to change to per-library directories? This would break backwards compatibility, though.