Merge makefiles in otherlibs/systhreads by shindere · Pull Request #785 · ocaml/ocaml

shindere · 2016-08-29T15:36:36Z

The initial project was to submit one PR merging all the makefiles in
all subdirectories of otherlibs. However it turned out that just the merge
in the systhreads directory is rather non trivial and may deserve a separate
review.

@alainfrisch you may want to review it since it seems you have worked on it.

alainfrisch · 2016-08-30T07:28:31Z

otherlibs/systhreads/Makefile

+ifeq "$(UNIX_OR_WIN32)" "unix"
+	$(MKLIB) -o $(LIBNAME) -ocamlc '$(CAMLC)' -cclib -lunix $(PTHREAD_CAML_LINK) $^
+else # Windows
+	$(MKLIB) -o $(LIBNAME) -ocamlc "$(CAMLC)" -linkall $^


Why is linkall used under Windows and not Unix?

Alain Frisch (2016/08/30 00:28 -0700):

-threads.cma: $(THREAD_OBJS)

$(MKLIB) -ocamlc '$(CAMLC)' -o threads $(THREAD_OBJS) \

-cclib -lunix $(PTHREAD_CAML_LINK)
+$(LIBNAME).cma: $(THREADS_BCOBJS)
+ifeq "$(UNIX_OR_WIN32)" "unix"

$(MKLIB) -o $(LIBNAME) -ocamlc '$(CAMLC)' -cclib -lunix $(PTHREAD_CAML_LINK) $^
+else # Windows

$(MKLIB) -o $(LIBNAME) -ocamlc "$(CAMLC)" -linkall $^

Why is linkall used under Windows and not Unix?

To be honest, I don't know. It's just the way things were done before so
I tried to be as conservative as possible for this commit, in order to
separate changes from each other. But if you think it would be better to
add linkall under Linux as well I'm perfectly fine and can certainly do
so, so please just let me know.

Thanks a lot for the review!

Merging makefiles has the nice benefit of making such differences apparent, and one should then really either remove or document them. But indeed, this can come later.

Alain Frisch (2016/08/30 01:08 -0700):

-threads.cma: $(THREAD_OBJS)

$(MKLIB) -ocamlc '$(CAMLC)' -o threads $(THREAD_OBJS) \

-cclib -lunix $(PTHREAD_CAML_LINK)
+$(LIBNAME).cma: $(THREADS_BCOBJS)
+ifeq "$(UNIX_OR_WIN32)" "unix"

$(MKLIB) -o $(LIBNAME) -ocamlc '$(CAMLC)' -cclib -lunix $(PTHREAD_CAML_LINK) $^
+else # Windows

$(MKLIB) -o $(LIBNAME) -ocamlc "$(CAMLC)" -linkall $^

Merging makefiles has the nice benefit of making such differences
apparent, and one should then really either remove or document them.
But indeed, this can come later.

I agree. For this specific example, it's just that it' was not obvious
to me what to do.

There are other places where I got rid of the differences, e.g. IIRC not
all the files were installed properly on Windows, I think it was the
.mli files which were not installed.

In the same vein, the warning-related flags were also different and I
took the most strict ones.

alainfrisch · 2016-08-30T08:40:34Z

Generally speaking, I'd suggest to add TODO comments to make it explicit when you discover "unexplained" differences between the two versions during the merge. Otherwise, people would easily assume that an explicit conditional in a Makefile is on purpose and not consider investigating the reason for the difference later.

shindere · 2016-08-30T08:49:56Z

That's a good idea and I will do that, thanks a lot.

alainfrisch · 2016-08-30T21:43:27Z

otherlibs/systhreads/Makefile


-libthreads.a: $(BYTECODE_C_OBJS)
-	$(MKLIB) -o threads $(BYTECODE_C_OBJS) $(PTHREAD_LINK)
+allopt: lib$(LIBNAME).$(A) $(LIBNAME).cmxa $(LIBNAME).cmxs $(CMIFILES)


This should be lib$(LIBNAME)nat.$(A), no?

Alain Frisch (2016/08/30 14:43 -0700):

-libthreads.a: $(BYTECODE_C_OBJS)

$(MKLIB) -o threads $(BYTECODE_C_OBJS) $(PTHREAD_LINK)
+allopt: lib$(LIBNAME).$(A) $(LIBNAME).cmxa $(LIBNAME).cmxs $(CMIFILES)

This should be lib$(LIBNAME)nat.$(A), no?

Well I'm getting confused. I believe that, at this level, the proposed
changes faithfully reflect the behaviour of the former Makefile.nt. Or
am I missing something here? Sorry if that's the case...

Yes, but it was probably wrong (and Makefile was right).

alainfrisch · 2016-08-30T22:12:58Z

Several simplifications could be added if we decide to drop the compilation of threads.cmxs and dllthreads.dll (both used to be built under Windows only). I'm in favor of doing it.

shindere · 2016-09-05T15:04:52Z

Alain Frisch (2016/08/30 14:55 -0700):

Dynamic linking with -lpthread is risky on many platforms, so

do not create a shared object for libthreadsnat.

-libthreadsnat.a: $(NATIVECODE_C_OBJS)

$(AR) rc libthreadsnat.a $(NATIVECODE_C_OBJS)

$(AR) rc $@ $^

Open question: what about using $(MKLIB) -custom? This brings it
closer to the Windows version (and possibly identical if one decide
not to build libthreadsnat.dll anymore).

Done.

Alain Frisch (2016/08/30 14:56 -0700):

Dynamic linking with -lpthread is risky on many platforms, so

do not create a shared object for libthreadsnat.

-libthreadsnat.a: $(NATIVECODE_C_OBJS)

$(AR) rc libthreadsnat.a $(NATIVECODE_C_OBJS)

$(AR) rc $@ $^
+else # Windows

$(MKLIB) -o $(LIBNAME)nat $^ $(LDOPTS)

Do we really want to build dllthreadsnat.dll? This looks like a dll
for bytecode, which seems useless here. So one could use $(MKLIB) -custom (which becomes identical to the Unix version).

So we discussed with @damiendoligez. He also thinks building the shared
objects is not useful. Hence the new version of the commit which does
not build them any longer.

Alain Frisch (2016/08/30 15:07 -0700):

INSTALL_STUBLIBDIR=$(DESTDIR)$(STUBLIBDIR)

install:
if test -f dllthreads.so; then \
cp dllthreads.so $(INSTALL_STUBLIBDIR)/dllthreads.so; fi
cp libthreads.a $(INSTALL_LIBDIR)/libthreads.a

cd $(INSTALL_LIBDIR); $(RANLIB) libthreads.a

if test -d $(INSTALL_LIBDIR)/threads; then :; \
else mkdir $(INSTALL_LIBDIR)/threads; fi
cp $(THREAD_OBJS:.cmo=.cmi) threads.cma $(INSTALL_LIBDIR)/threads

rm -f $(INSTALL_LIBDIR)/threads/stdlib.cma

cp thread.mli mutex.mli condition.mli event.mli threadUnix.mli \
 $(INSTALL_LIBDIR)
cp threads.h $(INSTALL_LIBDIR)/caml/threads.h

if test -f dllthreads$(EXT_DLL); then \
This would go away if we stop building dllthreads.dll (under Windows).

It has indeed been removed.

Alain Frisch (2016/08/30 15:13 -0700):

Several simplifications could be added if we decide to drop the
compilation of threads.cmxs and dllthreads.dll (both used to be built
under Windows only). I'm in favor of doing it.

Done. If you see other simplifications your comments are welcome.

(making make -j work in this directory seems to be a different matter
for another PR, to keep this one of a reasonable size)

Alain Frisch (2016/08/31 03:15 -0700):

-libthreads.a: $(BYTECODE_C_OBJS)

$(MKLIB) -o threads $(BYTECODE_C_OBJS) $(PTHREAD_LINK)
+allopt: lib$(LIBNAME).$(A) $(LIBNAME).cmxa $(LIBNAME).cmxs $(CMIFILES)

Yes, but it was probably wrong (and Makefile was right).

Fixed.

Alain Frisch (2016/08/31 03:20 -0700):

Note: I removed "-cclib -lunix" from the line above.

Indeed, if we link threads.cmxa, then we must also link unix.cmxa,

which itself will pass -lunix to the C linker. It seems more

modular to me this way. -- Alain

+$(LIBNAME).cmxs: $(LIBNAME).cmxa lib$(LIBNAME)nat.$(A)

Simplifying the build system is secondary, I agree. But even for
users, declaring once and for all (and documenting) that the
systhreads library does not provide a .cmxs file is better, in my
opinion, than providing the .cmxs file only on Windows.

@dra27 you may want to review this PR.

A few additional remarks:

It seems the use of -linkall under Windows has been there for a while,
and had been introduced in a subversion branch called natdynlink,
presumably by you @alainfrisch. Do you remeber what lead you to
introduce this?

In case you do not remember, we (@damiendoligez and I) propose to use
-linkall everywhere because the linked modules are not that big and, for
most of them (except perhaps events which is really small) it does not
seem to make sense to use one without the others.

I also removed two things:

The dependency of .cmx files on the ocamlopt comiler, because
it is not there in other libraries and because it had been added
almost 20 years ago, presumably for debugging purposes.
The line that removes threads/stdlib.cma, in agreement with
@damiendoligez.

alainfrisch · 2016-09-05T15:20:58Z

-linkall under Windows has been there for a while, and had been introduced in a subversion branch called natdynlink, presumably by you @alainfrisch. Do you remeber what lead you to introduce this?

No, unfortunately, I don't remember (that branch was the one were the notion of .cmxs plugins was added).

shindere · 2016-09-05T20:54:37Z

Alain Frisch (2016/09/05 08:21 -0700):

-linkall under Windows has been there for a while, and had been introduced in a subversion branch called natdynlink, presumably by you @alainfrisch. Do you remeber what lead you to introduce this?

No, unfortunately, I don't remember (that branch was the one were the
notion of .cmxs plugins was added).

Okay. So I can add the -linkall on the Unix side as suggested earlier.

This, however, leads to two other questions:

Even if -linkall is added, the commands on Unix and Windows
will still be different, because on the Unix side we have
-cclib -lunix $(PTHREAD_CAML_LINK)
which is not there on the Windows side.
(To be complete, $(PTHREAD_CAML_LINK) comes from configure, see
configure variable $pthread_caml_link.)

Can anyone see a way to unify these two commands?

If threads.cma is built using -linkall everywhere, shouldn't
threads.cmxa be built with -linkall, too?

shindere · 2016-09-08T06:57:36Z

The PR has been rebased on latest trunk and updated so that -linkall is used consistently. Moreover, comments have been added about those points that need further clarification. Can anybody see anything that still needs to be done before merging? Thanks.

alainfrisch · 2016-09-08T07:09:16Z

The CI build fails with:

../../boot/ocamlrun ../../ocamlopt -nostdlib -I ../../stdlib -I ../../otherlibs/unix -a -o -linkall threads.cmxa thread.cmx mutex.cmx condition.cmx event.cmx threadUnix.cmx -cclib -lthreadsnat -cclib -lpthread
Option -a cannot be used with .cmxa input files.

(-linkall should not be added between -o and its argument)

alainfrisch · 2016-09-08T07:11:03Z

otherlibs/systhreads/Makefile

-	$(CAMLOPT) -a -o threads.cmxa $(THREAD_OBJS:.cmo=.cmx) \
-	  -cclib -lthreadsnat $(PTHREAD_CAML_LINK)
+$(LIBNAME).cmxa: $(THREADS_NCOBJS)
+	$(CAMLOPT) -a -o -linkall $@ $^ -cclib -lthreadsnat $(PTHREAD_CAML_LINK)


This line causes the build failure.

Fixed. Sorry for the noise, thanks a lot for your prompt report, @alainfrisch!

alainfrisch · 2016-09-13T16:00:29Z

Thanks again for your hard work Sébastien!

I suggest that we group all these related changes into a single entry in the Changes file, if you agree.

shindere · 2016-09-13T16:03:31Z

Alain Frisch (2016/09/13 08:59 -0700):

Merged #785.

Thanks for merging @alainfrisch, and thanks even more for your patience
in reviewing.

Hope things will be easier with #808.

shindere · 2016-09-13T16:07:05Z

Alain Frisch (2016/09/13 09:00 -0700):

Thanks again for your hard work Sébastien!

Well, sorry for the lack of rigor!

I suggest that we group all these related changes into a single entry
in the Changes file, if you agree.

Certainly!

ghost · 2017-01-27T15:57:25Z

I'm getting some linking errors with 4.05 as dllthreads.so is no longer installed. This seems to be caused by this PR. Is it intentional?

ghost · 2017-01-27T16:03:41Z

Note that ocamlrun is linked with -lpthread, so dynamic loading of threads.cma doesn't cause dynamic linking of -lpthread

dra27 · 2017-01-27T16:47:54Z

It looks like an accident - "both" platforms.

dra27 · 2017-01-27T16:50:00Z

It should be dllthreads.$(EXT_DLL) now - are you able to do a quick GPR for it?

dra27 · 2017-01-27T16:51:55Z

We really should alter the testsuite and CI so that the testsuite is run over an installed OCaml, not just a compiled one!

ghost · 2017-01-27T17:30:38Z

Agreed for the testsuite. I think the fix can be committed directly. Should dllthreads.$(EXT_DLL) be installed on Windows as well?

dra27 · 2017-01-27T17:45:54Z

Yes to everything!

ghost · 2017-01-27T17:50:40Z

Ok, fixed in 812eb68

shindere · 2017-01-28T10:36:30Z

Jérémie Dimino (2017/01/27 09:50 -0800):

Ok, fixed in 812eb68

Many thanks Jérémie and David for your work on this! Sorry for the error!! Agreed that the testsuit should work against an installed version of OCaml. Took note of this and will take it into account!

gasche · 2017-01-28T11:00:28Z

Being able to run the testsuite from the compiled checkout without installation is important to me. The reason is that I have to configure a source checkout carefully to be able to install it as an opam switch, and I don't always remember to do this when I just run a build to test something. Forcing me to install would either force me to reconfigure (and rebuild) or risk overwriting the currently-configured switch location.

Having both test-from-sources and test-from-install targets would of course be fine.

dra27 · 2017-01-28T11:13:47Z

@gasche - I completely agree, I think the ability to do both is important! Having done that, we could also configure the two Travis tests to use the both approaches (e.g. flambda installs, non-flambda doesn't).

@shindere - thanks, I'll have a look. Get well soon - hope that arm's mending!

shindere · 2017-01-28T11:19:39Z

Gabriel Scherer (2017/01/28 03:00 -0800):

Having both test-from-sources and test-from-install targets would of course be fine.

That's what I'd like to achieve. test-from-install seems especially important in the context of cross-compiling.

shindere · 2017-01-28T11:21:37Z

David Allsopp (2017/01/28 03:13 -0800):

@shindere - thanks, I'll have a look. Get well soon - hope that arm's mending!

Thanks. I'm a bit worried because it seems to evolve very slowly, if at all. Rather frustrating! :)

ghost · 2017-01-30T07:43:54Z

@shindere no poblem, get well soon. I'm glad we can now build all of JS packages against trunk on a daily basis. This should help as well with testing trunk

dra27 · 2017-02-01T10:49:48Z

@diml - actually, fbdddb2 suggests that maybe we should never push build system changes directly :)

ghost · 2017-02-01T11:08:49Z

@dra27, I disagree, I think it suggests that bash is a terrible tool to use in build systems :)

dra27 · 2017-02-01T11:15:20Z

:) After my recent battles with AppVeyor in #1024, I couldn't agree more with that! As soon as you want spaces in filenames and directories, the same becomes true for make!

* Make sure the PTHREAD_CAML_LINK variable is defined everywhere. * Merge makefiles in otherlibs/systhreads Before this commit, the .cma and .cmxa libraries were linked using -linkall under Windows but not under Unix. It was not possible to clarify why -linkall was useful, but given the small size of the involved modules and the fact that, for most modules, it does not seem to make much sense to use one without the others, it has been decided to use -linkall everywhere. This commit also stops building the .cmxs shared library under Windows, for consistency reasons (it was built only under Windows before).

Unexport some unprefixed global names

Basic Cfg instructions don't raise. Only terminators can raise. Includes extension of the validator for register allocation with structural checks.

… for better alignment (ocaml#785) Co-authored-by: Sabine Schmaltz <[email protected]>

alainfrisch reviewed Aug 30, 2016
View reviewed changes

shindere force-pushed the merge-otherlibs-systhreads-makefiles branch 2 times, most recently from 38ed141 to 6c6dabe Compare August 30, 2016 08:30

shindere force-pushed the merge-otherlibs-systhreads-makefiles branch from 6c6dabe to 4fe3c52 Compare August 30, 2016 08:58

alainfrisch reviewed Aug 30, 2016
View reviewed changes

shindere force-pushed the merge-otherlibs-systhreads-makefiles branch 6 times, most recently from ae57e93 to 2d8ba8b Compare September 5, 2016 14:54

shindere force-pushed the merge-otherlibs-systhreads-makefiles branch from 2d8ba8b to 93b8624 Compare September 8, 2016 06:54

alainfrisch reviewed Sep 8, 2016
View reviewed changes

alainfrisch merged commit 5c4c41b into ocaml:trunk Sep 13, 2016

shindere deleted the merge-otherlibs-systhreads-makefiles branch September 14, 2016 09:31

shindere mentioned this pull request Sep 14, 2016

Allow parallel build of native and bytecode stubs of the systhreads library. #811

Merged

shindere mentioned this pull request Oct 21, 2016

Merge build systems in the stdlib directory #866

Merged

dra27 mentioned this pull request Nov 6, 2016

Fix bootstrapping flexlink #893

Merged

dra27 mentioned this pull request Feb 1, 2017

Extend the testsuite to test installations #1024

Closed

EduardoRFS pushed a commit to esy-ocaml/ocaml that referenced this pull request Dec 17, 2021

Merge pull request ocaml#785 from yallop/sanitize-all-exports

851148b

Unexport some unprefixed global names

stedolan pushed a commit to stedolan/ocaml that referenced this pull request Oct 25, 2022

CFG: rework terminators (ocaml#785)

ce1317b

Basic Cfg instructions don't raise. Only terminators can raise. Includes extension of the validator for register allocation with structural checks.

EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024

make code tags render in the same font size as surrounding body text,…

5bc789e

… for better alignment (ocaml#785) Co-authored-by: Sabine Schmaltz <[email protected]>

Conversation

shindere commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Aug 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shindere commented Aug 30, 2016 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Aug 30, 2016

Uh oh!

shindere commented Sep 5, 2016

Dynamic linking with -lpthread is risky on many platforms, so

do not create a shared object for libthreadsnat.

Dynamic linking with -lpthread is risky on many platforms, so

do not create a shared object for libthreadsnat.

Note: I removed "-cclib -lunix" from the line above.

Indeed, if we link threads.cmxa, then we must also link unix.cmxa,

which itself will pass -lunix to the C linker. It seems more

modular to me this way. -- Alain

Uh oh!

alainfrisch commented Sep 5, 2016

Uh oh!

shindere commented Sep 5, 2016

Uh oh!

shindere commented Sep 8, 2016 via email

Uh oh!

alainfrisch commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shindere Sep 8, 2016 via email

Choose a reason for hiding this comment

Uh oh!

alainfrisch commented Sep 13, 2016

Uh oh!

shindere commented Sep 13, 2016

Uh oh!

shindere commented Sep 13, 2016

Uh oh!

ghost commented Jan 27, 2017

Uh oh!

ghost commented Jan 27, 2017

Uh oh!

dra27 commented Jan 27, 2017

Uh oh!

dra27 commented Jan 27, 2017

Uh oh!

dra27 commented Jan 27, 2017

Uh oh!

ghost commented Jan 27, 2017

Uh oh!

dra27 commented Jan 27, 2017

Uh oh!

ghost commented Jan 27, 2017

Uh oh!

shindere commented Jan 28, 2017 via email

Uh oh!

gasche commented Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dra27 commented Jan 28, 2017

Uh oh!

shindere commented Jan 28, 2017 via email

Uh oh!

shindere commented Jan 28, 2017 via email

alainfrisch commented Aug 30, 2016 •

edited

Loading

alainfrisch commented Sep 8, 2016 •

edited

Loading

gasche commented Jan 28, 2017 •

edited

Loading

dra27 commented Feb 1, 2017 •

edited

Loading