MPR#7679: make sure .a files are erased before calling ar rc#1494
MPR#7679: make sure .a files are erased before calling ar rc#1494xavierleroy merged 3 commits intoocaml:trunkfrom
.a files are erased before calling ar rc#1494Conversation
Otherwise leftover .a files from an earlier compilation may contain unwanted modules.
The Caml definition in the #ml comment block is better. We can safely assume "ar" supports the "s" option because this is "ar" from the GNU binutils.
dra27
left a comment
There was a problem hiding this comment.
I don't know if the MSVC linker does the same thing, but would it be worth deleting the .lib file in their definitions of MKLIB too?
|
|
||
| ### How to build a static library | ||
| MKLIB=rm -f $(1); $(TOOLPREF)ar rc $(1) $(2); $(RANLIB) $(1) | ||
| MKLIB=rm -f $(1) && $(TOOLPREF)ar rcs $(1) $(2) |
There was a problem hiding this comment.
These two have been switched from using RANLIB to using the 's' option of ar, but the configure script still calls ranlib expressly.
There was a problem hiding this comment.
As mentioned in my commit message, Mingw uses the GNU version of ar for which I'm certain the s flag is supported. In contrast the ar provided by old Unix platforms could lack the s flag and could require an explicit ranlib call.
There was a problem hiding this comment.
Sorry, I missed the commit message thanks to GitHub's tedious interface!
What bothers me with this is twofold - a difference here is "annoying" (or at least a special case) when the 4 Windows ports get moved into the configure script with autoconf. It's also a bit weird that both RANLIB and ar -s are used at various points in the build system, though I don't expect that can ever cause a problem (I'd be more inclined to have altered the #ml branch to be calling RANLIBCMD and add the optimisation to use GNU ar's -s switch everywhere properly at a later date)
config/Makefile.mingw
Outdated
| MKLIB=rm -f $(1); $(TOOLPREF)ar rc $(1) $(2); $(RANLIB) $(1) | ||
| MKLIB=rm -f $(1) && $(TOOLPREF)ar rcs $(1) $(2) | ||
| #ml let mklib out files opts = | ||
| #ml Printf.sprintf "rm -f %s && %sar rcs %s %s %s" |
There was a problem hiding this comment.
This obviously wasn't changed as part of this GPR, but it would be better if mingw/msvc were using the del command, rather than rm (or, better yet, of the deletion were done more expressly in the compiler with a Sys.remove call)
There was a problem hiding this comment.
Good point! Actually, this mklib definition is used in only one place, namely tools/ocamlmklib.ml line 287, and the library file to be created is deleted before calling the mklib command. So, we can simplify the corresponding #ml configuration lines to not call rm nor del.
The other place in OCaml where a static library is built is utils/ccomp.ml, function create_archive. There, the destination file is also deleted first, and moreover mklib is not used, rather the use of ar+ranlib or of link /lib is hardcoded.
I couldn't find documentation for |
The lines `#ml let mklib ...` are used only in utils/ocamlmklib.ml, where the destination .a file is always erased before `mklib` is invoked. Hence there is no need for a `rm` (or `del`) shell command in `mklib`.
|
The latest commit removes the useless calls to Requesting a formal accept/reject review. |
|
alainfrisch
left a comment
There was a problem hiding this comment.
LGTM.
It is my understanding that MKLIB is used only in byterun/Makefile and asmrun/Makefile. Is that right?
(There are other variables called MKLIB in other Makefiles, but they refer to ocamlmklib. It would be best to rename them, but this is unrelated to this PR.)
config/Makefile.mingw64
Outdated
| MKLIB=rm -f $(1); $(TOOLPREF)ar rc $(1) $(2); $(RANLIB) $(1) | ||
| MKLIB=rm -f $(1) && $(TOOLPREF)ar rcs $(1) $(2) | ||
| #ml let mklib out files opts = | ||
| #ml Printf.sprintf "rm -f %s && %sar rcs %s %s %s" |
There was a problem hiding this comment.
This rm -rf also wants to go (as in the .mingw Makefile)
There was a problem hiding this comment.
It is gone in the current version (commit 897c1f5, pushed yesterday). Not sure why Github is making you review an outdated version!
…ml#1494) Otherwise leftover .a files from an earlier compilation may contain unwanted modules, as shown in MPR#7679. However, ocamlmklib always erases the destination .a file before calling the `mkdll` function defined in `#ml` blocks of the configuration makefile. Hence there is no need for a `rm` (or `del`) shell command in `mklib`. Finally, in the config/Makefile.mingw* files, we can safely assume "ar" supports the "s" option because this is "ar" from the GNU binutils, so no need to call ranlib.
Otherwise, as shown by MPR#7679, leftover
.afiles from an earlier compilation may contain unwanted modules.Also: replace
;by&&when chaining several shell commands, it's safer this way.The second commit freshens up the Mingw32 and Mingw64 configurations. They did the correct file-remove dance in MKLIB already, but the
#mlblock contained a different, and better, definition than the MKLIB shell command.