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

Merge otherlibs/dynlink/Makefile into the root Makefile #13285

Merged
merged 6 commits into from
Jul 19, 2024

Conversation

shindere
Copy link
Contributor

@shindere shindere commented Jul 10, 2024

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 root Makefile 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 the otherlibs/dynlink directory as before.

The first commit introduces the private OTHERLIBS build variable to keep track of which other libraries are still built by otherlibs/Makefile. This variable is initially equal to OTHERLIBRARIES (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, the clean rule of compilerlibs 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 root Makefile. As usually, this goes hand in hand with otherlibs/dynlink/.depend being merged into the root .depend.

@shindere shindere force-pushed the merge-dynlink-makefile branch 3 times, most recently from 4283bd0 to ab3f2ee Compare July 10, 2024 18:57
@shindere
Copy link
Contributor Author

shindere commented Jul 10, 2024 via email

@shindere shindere force-pushed the merge-dynlink-makefile branch 9 times, most recently from f7f9459 to f3d3ae3 Compare July 17, 2024 15:31
@shindere shindere force-pushed the merge-dynlink-makefile branch 3 times, most recently from 0f433f6 to 6a215ce Compare July 18, 2024 09:09
@shindere
Copy link
Contributor Author

A few updates:

The PR is now ready to be reviewed, after a bit of work,
part of which having been done offline with @dra27.

To elaborate on the change of checksums, this is actually due
to the fact that this PR changes the file names that are
stored in locations. Instead of having jsut file names as we
did before, we now have complete file paths relative to the
top level directory of the source tree. This can be seen in the
changes to the test reference files.

Also, thePR now begins with a commit that simplifies the C# test
by not mentionning the dynlink files explicitly. THey are actually
already linked so they were linked twice, which caused a problem when used
in conjunction with -linkall. This is @dra27's discovery.

The commit which had been added to copy .cmx files systematically has
finally been remove in favour of a more ad hoc solution, which is closer
tohwat was done before.

shindere added 6 commits July 18, 2024 16:11
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.
@shindere shindere force-pushed the merge-dynlink-makefile branch from 6a215ce to 4c6a5d5 Compare July 18, 2024 16:14
Copy link
Member

@dra27 dra27 left a 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

@dra27 dra27 merged commit b8ee117 into ocaml:trunk Jul 19, 2024
15 checks passed
@shindere
Copy link
Contributor Author

shindere commented Jul 19, 2024 via email

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