-
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
Merge otherlibs/dynlink/Makefile into the root Makefile #13285
Conversation
4283bd0
to
ab3f2ee
Compare
Fixed, thanks!
|
f7f9459
to
f3d3ae3
Compare
0f433f6
to
6a215ce
Compare
A few updates: The PR is now ready to be reviewed, after a bit of work, To elaborate on the change of checksums, this is actually due Also, thePR now begins with a commit that simplifies the The commit which had been added to copy |
Before this commit, the test was adding dynlink.cmx?a to the all_modules variable but this is actually not necessary because the "include dynlink" directive already adds them as appropriate to the libraries variable. This has been spotted by @dra27 as it led to a test failure when -linkall was (incorrectly) used because this led to some modules being linked twice.
This commit introduces build variables foo_{COMMON,BYTECODE,NATIVE}_LINKFLAGS where foo is the name of a program to build. These variables are initialised with empty strings (no flags) and can then be overridden by each program which needs to. Such variables allow more fine-grained control over flags so that they do not propagate when put in target-specific variables. It may be the case that these variables become unnecessary once we use a version of GNU make which is recent enough to support the private keyword.
This variable lists the other libraries that get built by the recursive makefiles. At this point it has the same content as the OTHERLIBRARIES variable but, when the other libraries will be built by the root Makefile their names will be removed from this variable. It is thus expected that this variable becomes empty and can be removed once all the other libraries get built by the root Makefile.
otherlibs/dynlink/dynlink_platform_intf.mli is a copy of the corresponding otherlibs/dynlink/dynlink_platform_intf.ml file. Before this commit, the copy was done during the build. With this commit the copy (more likely a symlink) is done by configure as is already done for other files that need to be duplicated.
6a215ce
to
4c6a5d5
Compare
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.
Reviewed offline (outcome in the most recent force push) - LGTM, thanks, @shindere
Plenty of thanks @ëra27 for your super supportive review.
|
This PR continues the build system unification work started in #11243, #11248, #11268, #11420, #11675, #12198, #12321, #12586, #12616, #12706 and #13048.
The present PR merges
otherlibs/dynlink/Makefile
into the rootMakefile
and consists of four commits. The three first ones have the property of not altering the installed artefacts at all. The only commit that does so is the last one, which is the core of the PR, and the only thing it changes to the installed artifacts is the checksums, which is due to the fact that the compilation of the OCaml sources is done from the top-level of the source tree rather than from theotherlibs/dynlink
directory as before.The first commit introduces the private
OTHERLIBS
build variable to keep track of which other libraries are still built byotherlibs/Makefile
. This variable is initially equal toOTHERLIBRARIES
(the list of other libraries to build) but this list will become shorter andshohrter as more and more other libraries will be built directly by the root Makefile. Once all the other libraries are built by the root Makefile this variable is expected to become empty and to finally vanish.The second commit delegates some file copying which was previously done during the build to the configure script.
THe third (and last preparatory) commit extends the general framework defined in
Makefile.common
to make sure the.cmx
files of a native library get copied to the directory where its.cmxa
file resides, as this is necessary when linking with dynlink's native version. As the copy happens also for all the other libraries built by the framework, theclean
rule ofcompilerlibs
had to be adjusted to clean the newly copied artefacts.Finally, the PR's last commit is its core, which is the actual merge of
otherlibs/dynlink/Makefile
into the rootMakefile
. As usually, this goes hand in hand withotherlibs/dynlink/.depend
being merged into the root.depend
.