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 makefiles in otherlibs/systhreads #785

Merged
merged 2 commits into from Sep 13, 2016

Conversation

Projects
None yet
5 participants
@shindere
Contributor

shindere commented Aug 29, 2016

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.

Show outdated Hide outdated 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 $^

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Why is linkall used under Windows and not Unix?

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Why is linkall used under Windows and not Unix?

This comment has been minimized.

@shindere

shindere Aug 30, 2016

Contributor

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!

@shindere

shindere Aug 30, 2016

Contributor

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!

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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.

This comment has been minimized.

@shindere

shindere Aug 30, 2016

Contributor

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.

@shindere

shindere Aug 30, 2016

Contributor

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.

Show outdated Hide outdated otherlibs/systhreads/Makefile
st_stubs_n.$(O): st_stubs.c $(HEADER)
$(NATIVECC) -I../../asmrun -I../../byterun $(NATIVECCCOMPOPTS) \
$(SHAREDCCCOMPOPTS) -DNATIVE_CODE -DTARGET_$(ARCH) \
-DMODEL_$(MODEL) -DSYS_$(SYSTEM) -c st_stubs.c

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

I'd suggest using $< as in the rule above. And also $(ROOTDIR) instead of ../...

Wouldn't it be better to copy the source file before compiling, instead of moving the resulting object file? I'm thinking about parallel make.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

I'd suggest using $< as in the rule above. And also $(ROOTDIR) instead of ../...

Wouldn't it be better to copy the source file before compiling, instead of moving the resulting object file? I'm thinking about parallel make.

This comment has been minimized.

@shindere

shindere Aug 30, 2016

Contributor

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

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)

  • $(CAMLOPT) -shared -o $@ -I . $< -linkall

+st_stubs_b.$(O): st_stubs.c $(HEADER)

  • $(BYTECC) -I../../byterun $(BYTECCCOMPOPTS) $(SHAREDCCCOMPOPTS) -c $<
  • mv st_stubs.$(O) $@

+st_stubs_n.$(O): st_stubs.c $(HEADER)

  • $(NATIVECC) -I../../asmrun -I../../byterun $(NATIVECCCOMPOPTS) \
  • $(SHAREDCCCOMPOPTS) -DNATIVE_CODE -DTARGET_$(ARCH) \
    
  • -DMODEL_$(MODEL) -DSYS_$(SYSTEM) -c st_stubs.c
    

I'd suggest using $< as in the rule above.

Done, thanks.

And also $(ROOTDIR) instead of ../...

Done, too. I actually made sure ../.. was not used in any place and
systematically replaced it with $(ROOTDIR).

Wouldn't it be better to copy the source file before compiling,
instead of moving the resulting object file? I'm thinking about
parallel make.

I noticed the issue, too. Again my plan was to address it later but if
you prefere it can be part of this commit, too.

Regarding the way to address it, the idea I had in mind was not
copy-based. The approach I'd suggest would be to introduce three files:
st_stubs.c for all the common stuff and st_stubs_b.c and st_stubs_n.c
for what is specific to bytecode resp. nativecode. Even if e.g. the
bytecode version just includes the common file. And in the nativecode
version one could #define the macros rather than passing them to the
compiler in the Makefile. What do you think?

Oh and by the way, that was another difference between the build
systems: not the same macros were defined under Unix and Windows.

@shindere

shindere Aug 30, 2016

Contributor

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

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)

  • $(CAMLOPT) -shared -o $@ -I . $< -linkall

+st_stubs_b.$(O): st_stubs.c $(HEADER)

  • $(BYTECC) -I../../byterun $(BYTECCCOMPOPTS) $(SHAREDCCCOMPOPTS) -c $<
  • mv st_stubs.$(O) $@

+st_stubs_n.$(O): st_stubs.c $(HEADER)

  • $(NATIVECC) -I../../asmrun -I../../byterun $(NATIVECCCOMPOPTS) \
  • $(SHAREDCCCOMPOPTS) -DNATIVE_CODE -DTARGET_$(ARCH) \
    
  • -DMODEL_$(MODEL) -DSYS_$(SYSTEM) -c st_stubs.c
    

I'd suggest using $< as in the rule above.

Done, thanks.

And also $(ROOTDIR) instead of ../...

Done, too. I actually made sure ../.. was not used in any place and
systematically replaced it with $(ROOTDIR).

Wouldn't it be better to copy the source file before compiling,
instead of moving the resulting object file? I'm thinking about
parallel make.

I noticed the issue, too. Again my plan was to address it later but if
you prefere it can be part of this commit, too.

Regarding the way to address it, the idea I had in mind was not
copy-based. The approach I'd suggest would be to introduce three files:
st_stubs.c for all the common stuff and st_stubs_b.c and st_stubs_n.c
for what is specific to bytecode resp. nativecode. Even if e.g. the
bytecode version just includes the common file. And in the nativecode
version one could #define the macros rather than passing them to the
compiler in the Makefile. What do you think?

Oh and by the way, that was another difference between the build
systems: not the same macros were defined under Unix and Windows.

Show outdated Hide outdated otherlibs/systhreads/Makefile
ifeq "$(UNIX_OR_WIN32)" "unix"
$(CAMLOPT) -a -o $@ $^ -cclib -lthreadsnat $(PTHREAD_CAML_LINK)
else # Windows
$(MKLIB) -o $(LIBNAME)nat -ocamlopt "$(CAMLOPT)" -linkall $^ $(LINKOPTS)

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Wouldn't it work to the same as under unix here (i.e. use ocamlopt directly to build the lib)?

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Wouldn't it work to the same as under unix here (i.e. use ocamlopt directly to build the lib)?

This comment has been minimized.

@shindere

shindere Aug 30, 2016

Contributor

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

See remark above: force static linking of libthreadsnat.a

-threads.cmxa: $(THREAD_OBJS:.cmo=.cmx)

  • $(CAMLOPT) -a -o threads.cmxa $(THREAD_OBJS:.cmo=.cmx) \
  • -cclib -lthreadsnat $(PTHREAD_CAML_LINK)
    
    +$(LIBNAME).cmxa: $(THREADS_NCOBJS)
    +ifeq "$(UNIX_OR_WIN32)" "unix"
  • $(CAMLOPT) -a -o $@ $^ -cclib -lthreadsnat $(PTHREAD_CAML_LINK)
    +else # Windows
  • $(MKLIB) -o $(LIBNAME)nat -ocamlopt "$(CAMLOPT)" -linkall $^ $(LINKOPTS)

Wouldn't it work to the same as under unix here (i.e. use ocamlopt
directly to build the lib)?

It does indeed, thanks.

So I updated the PR accordingly and also added a TODO regarding the
linkall issue, so I think I have now taken into account all the comments
so far.

@shindere

shindere Aug 30, 2016

Contributor

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

See remark above: force static linking of libthreadsnat.a

-threads.cmxa: $(THREAD_OBJS:.cmo=.cmx)

  • $(CAMLOPT) -a -o threads.cmxa $(THREAD_OBJS:.cmo=.cmx) \
  • -cclib -lthreadsnat $(PTHREAD_CAML_LINK)
    
    +$(LIBNAME).cmxa: $(THREADS_NCOBJS)
    +ifeq "$(UNIX_OR_WIN32)" "unix"
  • $(CAMLOPT) -a -o $@ $^ -cclib -lthreadsnat $(PTHREAD_CAML_LINK)
    +else # Windows
  • $(MKLIB) -o $(LIBNAME)nat -ocamlopt "$(CAMLOPT)" -linkall $^ $(LINKOPTS)

Wouldn't it work to the same as under unix here (i.e. use ocamlopt
directly to build the lib)?

It does indeed, thanks.

So I updated the PR accordingly and also added a TODO regarding the
linkall issue, so I think I have now taken into account all the comments
so far.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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.

Contributor

alainfrisch commented Aug 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Aug 30, 2016

Contributor
Contributor

shindere commented Aug 30, 2016

Show outdated Hide outdated 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 comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

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...

@shindere

shindere Aug 31, 2016

Contributor

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...

This comment has been minimized.

@alainfrisch

alainfrisch Aug 31, 2016

Contributor

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

@alainfrisch

alainfrisch Aug 31, 2016

Contributor

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

Show outdated Hide outdated otherlibs/systhreads/Makefile
# 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)

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

The .cmxs file was not built on Unix previously, probably because "Dynamic linking with -lpthread is risky on many platforms" as another comment said. It used to be built under Windows, though. I'd rather not starting to build the .cmxs file under Unix, unless you have a good reason to do so. And I would use the opportunity to stop building it under Windows as well for the sake of uniformity across platforms.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

The .cmxs file was not built on Unix previously, probably because "Dynamic linking with -lpthread is risky on many platforms" as another comment said. It used to be built under Windows, though. I'd rather not starting to build the .cmxs file under Unix, unless you have a good reason to do so. And I would use the opportunity to stop building it under Windows as well for the sake of uniformity across platforms.

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

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

The .cmxs file was not built on Unix previously, probably because
"Dynamic linking with -lpthread is risky on many platforms" as another
comment said. It used to be built under Windows, though. I'd rather
not starting to build the .cmxs file under Unix, unless you have a
good reason to do so. And I would use the opportunity to stop
building it under Windows as well for the sake of uniformity across
platforms.

I think cmxs files are provided for other libraries, see for instance
the rules in otherlibs/Makefile. So my (open) question is: is it okay to
not be consistent in providing the .cmxs files?

I mean, as I understand it, there is a choice between either not
providing the .cmxs file for this library, for sake of obtaining a
simpler build system, or trying to provide the .cmxs file also for this
library, whenever possible (i.e. at least on Windows) because this is
more coherent with what happens with other libraries.

Is it correct to phrase the question that way?

I, personnally, have no strong opinion. It seems to me that the overall
coherence (providing .cmxs files whenever possible) could be considered as
more important than keeping the build system uniform, because it can
have an impact on users, whereas only OCaml developers are concerned
with the simplicity of the build system. But that beiing said I'm happy
to implement whatever the community finds best.

@shindere

shindere Aug 31, 2016

Contributor

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

The .cmxs file was not built on Unix previously, probably because
"Dynamic linking with -lpthread is risky on many platforms" as another
comment said. It used to be built under Windows, though. I'd rather
not starting to build the .cmxs file under Unix, unless you have a
good reason to do so. And I would use the opportunity to stop
building it under Windows as well for the sake of uniformity across
platforms.

I think cmxs files are provided for other libraries, see for instance
the rules in otherlibs/Makefile. So my (open) question is: is it okay to
not be consistent in providing the .cmxs files?

I mean, as I understand it, there is a choice between either not
providing the .cmxs file for this library, for sake of obtaining a
simpler build system, or trying to provide the .cmxs file also for this
library, whenever possible (i.e. at least on Windows) because this is
more coherent with what happens with other libraries.

Is it correct to phrase the question that way?

I, personnally, have no strong opinion. It seems to me that the overall
coherence (providing .cmxs files whenever possible) could be considered as
more important than keeping the build system uniform, because it can
have an impact on users, whereas only OCaml developers are concerned
with the simplicity of the build system. But that beiing said I'm happy
to implement whatever the community finds best.

This comment has been minimized.

@alainfrisch

alainfrisch Aug 31, 2016

Contributor

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.

@alainfrisch

alainfrisch Aug 31, 2016

Contributor

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.

Show outdated Hide outdated otherlibs/systhreads/Makefile
$(AR) rc libthreadsnat.a $(NATIVECODE_C_OBJS)
$(AR) rc $@ $^
else # Windows
$(MKLIB) -o $(LIBNAME)nat $^ $(LDOPTS)

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

I know this comes from the old Makefile.nt, but is LDOPTS really useful (and if so, why not under Unix)?

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

I know this comes from the old Makefile.nt, but is LDOPTS really useful (and if so, why not under Unix)?

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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).

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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).

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

Alain Frisch (2016/08/30 14:53 -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)

I know this comes from the old Makefile.nt, but is LDOPTS really
useful (and if so, why not under Unix)?

No it's not useful, it's empty. Thanks!
Commit amended to not use it any longer.

@shindere

shindere Aug 31, 2016

Contributor

Alain Frisch (2016/08/30 14:53 -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)

I know this comes from the old Makefile.nt, but is LDOPTS really
useful (and if so, why not under Unix)?

No it's not useful, it's empty. Thanks!
Commit amended to not use it any longer.

Show outdated Hide outdated otherlibs/systhreads/Makefile
# 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 $@ $^

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Open question: what about using $(MKLIB) -custom? This brings it closer to the Windows version and possibly identical (see notes below).

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Open question: what about using $(MKLIB) -custom? This brings it closer to the Windows version and possibly identical (see notes below).

Show outdated Hide outdated otherlibs/systhreads/Makefile
cp dllthreads$(EXT_DLL) $(INSTALL_STUBLIBDIR); fi
cp libthreads.$(A) $(INSTALL_LIBDIR)
ifeq "$(UNIX_OR_WIN32)" "unix"
cd $(INSTALL_LIBDIR); $(RANLIB) libthreads.$(A)

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

It is fine to use $(RANLIB) under Windows to avoid the conditional (it is just defined as echo).

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

It is fine to use $(RANLIB) under Windows to avoid the conditional (it is just defined as echo).

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

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

  • 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 \
  • cp dllthreads$(EXT_DLL) $(INSTALL_STUBLIBDIR); fi
    
  • cp libthreads.$(A) $(INSTALL_LIBDIR)
    +ifeq "$(UNIX_OR_WIN32)" "unix"
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreads.$(A)

It is fine to use $(RANLIB) under Windows to avoid the conditional (it
is just defined as echo).

Indeed, thanks!

@shindere

shindere Aug 31, 2016

Contributor

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

  • 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 \
  • cp dllthreads$(EXT_DLL) $(INSTALL_STUBLIBDIR); fi
    
  • cp libthreads.$(A) $(INSTALL_LIBDIR)
    +ifeq "$(UNIX_OR_WIN32)" "unix"
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreads.$(A)

It is fine to use $(RANLIB) under Windows to avoid the conditional (it
is just defined as echo).

Indeed, thanks!

Show outdated Hide outdated otherlibs/systhreads/Makefile
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 comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

This would go away if we stop building dllthreads.dll (under Windows).

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

This would go away if we stop building dllthreads.dll (under Windows).

Show outdated Hide outdated otherlibs/systhreads/Makefile
endif
mkdir -p "$(THREADS_LIBDIR)"
cp $(CMIFILES) threads.cma "$(THREADS_LIBDIR)"
rm -f "$(THREADS_LIBDIR)/stdlib.cma"

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Do you understand what this is about (I know it was there before)?

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Do you understand what this is about (I know it was there before)?

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

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

  • 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 \
  • cp dllthreads$(EXT_DLL) $(INSTALL_STUBLIBDIR); fi
    
  • cp libthreads.$(A) $(INSTALL_LIBDIR)
    +ifeq "$(UNIX_OR_WIN32)" "unix"
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreads.$(A)
    +endif
  • mkdir -p "$(THREADS_LIBDIR)"
  • cp $(CMIFILES) threads.cma "$(THREADS_LIBDIR)"
  • rm -f "$(THREADS_LIBDIR)/stdlib.cma"

Do you understand what this is about (I know it was there before)?

Are you referring to one line in particular here?

@shindere

shindere Aug 31, 2016

Contributor

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

  • 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 \
  • cp dllthreads$(EXT_DLL) $(INSTALL_STUBLIBDIR); fi
    
  • cp libthreads.$(A) $(INSTALL_LIBDIR)
    +ifeq "$(UNIX_OR_WIN32)" "unix"
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreads.$(A)
    +endif
  • mkdir -p "$(THREADS_LIBDIR)"
  • cp $(CMIFILES) threads.cma "$(THREADS_LIBDIR)"
  • rm -f "$(THREADS_LIBDIR)/stdlib.cma"

Do you understand what this is about (I know it was there before)?

Are you referring to one line in particular here?

This comment has been minimized.

@alainfrisch

alainfrisch Aug 31, 2016

Contributor

Yes:

rm -f "$(THREADS_LIBDIR)/stdlib.cma"

I don't know what is this stdlib.cma under $(THREADS_LIBDIR).

@alainfrisch

alainfrisch Aug 31, 2016

Contributor

Yes:

rm -f "$(THREADS_LIBDIR)/stdlib.cma"

I don't know what is this stdlib.cma under $(THREADS_LIBDIR).

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

Alain Frisch (2016/08/31 08:46 -0700):

Yes:

rm -f "$(THREADS_LIBDIR)/stdlib.cma"

I don't know what is this stdlib.cma under $(THREADS_LIBDIR).

Me neither. Is it possible that, for older versions of OCaml, the stdlib
came in two flavours depending on whether one compiled with or without
threads? And then when newer versions arrived they added this command to
remove the thread-safe version of the stdlib which became obsolete?

Is that possible?

Perhaps this command can now be removed?

@shindere

shindere Aug 31, 2016

Contributor

Alain Frisch (2016/08/31 08:46 -0700):

Yes:

rm -f "$(THREADS_LIBDIR)/stdlib.cma"

I don't know what is this stdlib.cma under $(THREADS_LIBDIR).

Me neither. Is it possible that, for older versions of OCaml, the stdlib
came in two flavours depending on whether one compiled with or without
threads? And then when newer versions arrived they added this command to
remove the thread-safe version of the stdlib which became obsolete?

Is that possible?

Perhaps this command can now be removed?

Show outdated Hide outdated otherlibs/systhreads/Makefile
BYTECODE_C_OBJS=st_stubs_b.$(O)
NATIVECODE_C_OBJS=st_stubs_n.$(O)
THREADS_SOURCES := thread.ml mutex.ml condition.ml event.ml threadUnix.ml

This comment has been minimized.

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Why are you using := (and also below)?

@alainfrisch

alainfrisch Aug 30, 2016

Contributor

Why are you using := (and also below)?

This comment has been minimized.

@shindere

shindere Aug 31, 2016

Contributor

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

COMPFLAGS=-w +33..39 -warn-error A -g -bin-annot -safe-string
ifeq "$(FLAMBDA)" "true"
OPTCOMPFLAGS=-O3
else
OPTCOMPFLAGS=
endif

-BYTECODE_C_OBJS=st_stubs_b.o
-NATIVECODE_C_OBJS=st_stubs_n.o
+LIBNAME=threads
+
+ifeq "$(UNIX_OR_WIN32)" "unix"
+HEADER = st_posix.h
+else # Windows
+HEADER = st_win32.h
+endif
+
+BYTECODE_C_OBJS=st_stubs_b.$(O)
+NATIVECODE_C_OBJS=st_stubs_n.$(O)
+
+THREADS_SOURCES := thread.ml mutex.ml condition.ml event.ml threadUnix.ml

Why are you using := (and also below)?

It's true that it may be better to be consistent.
That being said, I believe it is legitimate to use := in these places
and it would even be better to use it in other places, too.

See section 6.2 The Two Flavors of Variables in the make manual.

For the moment I replaced := by = everywhere, but I think in the long
run it may be better to use := where it makes sense, i.e. in a lot of
places, IMO.

@shindere

shindere Aug 31, 2016

Contributor

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

COMPFLAGS=-w +33..39 -warn-error A -g -bin-annot -safe-string
ifeq "$(FLAMBDA)" "true"
OPTCOMPFLAGS=-O3
else
OPTCOMPFLAGS=
endif

-BYTECODE_C_OBJS=st_stubs_b.o
-NATIVECODE_C_OBJS=st_stubs_n.o
+LIBNAME=threads
+
+ifeq "$(UNIX_OR_WIN32)" "unix"
+HEADER = st_posix.h
+else # Windows
+HEADER = st_win32.h
+endif
+
+BYTECODE_C_OBJS=st_stubs_b.$(O)
+NATIVECODE_C_OBJS=st_stubs_n.$(O)
+
+THREADS_SOURCES := thread.ml mutex.ml condition.ml event.ml threadUnix.ml

Why are you using := (and also below)?

It's true that it may be better to be consistent.
That being said, I believe it is legitimate to use := in these places
and it would even be better to use it in other places, too.

See section 6.2 The Two Flavors of Variables in the make manual.

For the moment I replaced := by = everywhere, but I think in the long
run it may be better to use := where it makes sense, i.e. in a lot of
places, IMO.

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 30, 2016

Contributor

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.

Contributor

alainfrisch commented Aug 30, 2016

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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Sep 5, 2016

Contributor

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.
Contributor

shindere commented Sep 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Sep 5, 2016

Contributor

-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).

Contributor

alainfrisch commented Sep 5, 2016

-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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Sep 5, 2016

Contributor

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:

  1. 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?

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

shindere commented Sep 5, 2016

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:

  1. 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?

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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Sep 8, 2016

Contributor
Contributor

shindere commented Sep 8, 2016

@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Sep 8, 2016

Contributor

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)

Contributor

alainfrisch commented Sep 8, 2016

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)

Show outdated Hide outdated 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 comment has been minimized.

@alainfrisch

alainfrisch Sep 8, 2016

Contributor

This line causes the build failure.

@alainfrisch

alainfrisch Sep 8, 2016

Contributor

This line causes the build failure.

This comment has been minimized.

@shindere

shindere Sep 8, 2016

Contributor
@shindere

shindere via email Sep 8, 2016

Contributor
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Sep 9, 2016

Contributor

AppVeyor fails with

cp libthreads.lib C:/Program Files/OCaml/lib
cp: target 'Files/OCaml/lib' is not a directory
Contributor

alainfrisch commented Sep 9, 2016

AppVeyor fails with

cp libthreads.lib C:/Program Files/OCaml/lib
cp: target 'Files/OCaml/lib' is not a directory
Show outdated Hide outdated otherlibs/systhreads/Makefile
cp thread.mli mutex.mli condition.mli event.mli threadUnix.mli \
$(INSTALL_LIBDIR)
cp threads.h $(INSTALL_LIBDIR)/caml/threads.h
cp libthreads.$(A) $(INSTALL_LIBDIR)

This comment has been minimized.

@alainfrisch

alainfrisch Sep 9, 2016

Contributor

Missing quotes around the target directory, I guess.

@alainfrisch

alainfrisch Sep 9, 2016

Contributor

Missing quotes around the target directory, I guess.

This comment has been minimized.

@shindere

shindere Sep 12, 2016

Contributor

Alain Frisch (2016/09/09 06:18 -0700):

AppVeyor fails with

cp libthreads.lib C:/Program Files/OCaml/lib
cp: target 'Files/OCaml/lib' is not a directory

Alain Frisch (2016/09/09 06:19 -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
  • cp libthreads.$(A) $(INSTALL_LIBDIR)

Missing quotes around the target directory, I guess.

Alain Frisch (2016/09/09 06:20 -0700):

installopt:

  • cp libthreadsnat.a $(INSTALL_LIBDIR)/libthreadsnat.a
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreadsnat.a
  • cp $(THREAD_OBJS:.cmo=.cmx) threads.cmxa threads.a \
  •  $(INSTALL_LIBDIR)/threads
    
  • cd $(INSTALL_LIBDIR)/threads && $(RANLIB) threads.a
  • cp libthreadsnat.$(A) "$(INSTALL_LIBDIR)"
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreadsnat.$(A)
  • cp $(THREADS_NCOBJS) threads.cmxa threads.$(A) $(THREADS_LIBDIR)

Missing quotes as well (also on cd lines above and below).

Indeed, sorry about that and thanks a lot for having reported it.

This should now be fixed and the PR has been rebased on the latest
trunk.

@shindere

shindere Sep 12, 2016

Contributor

Alain Frisch (2016/09/09 06:18 -0700):

AppVeyor fails with

cp libthreads.lib C:/Program Files/OCaml/lib
cp: target 'Files/OCaml/lib' is not a directory

Alain Frisch (2016/09/09 06:19 -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
  • cp libthreads.$(A) $(INSTALL_LIBDIR)

Missing quotes around the target directory, I guess.

Alain Frisch (2016/09/09 06:20 -0700):

installopt:

  • cp libthreadsnat.a $(INSTALL_LIBDIR)/libthreadsnat.a
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreadsnat.a
  • cp $(THREAD_OBJS:.cmo=.cmx) threads.cmxa threads.a \
  •  $(INSTALL_LIBDIR)/threads
    
  • cd $(INSTALL_LIBDIR)/threads && $(RANLIB) threads.a
  • cp libthreadsnat.$(A) "$(INSTALL_LIBDIR)"
  • cd $(INSTALL_LIBDIR); $(RANLIB) libthreadsnat.$(A)
  • cp $(THREADS_NCOBJS) threads.cmxa threads.$(A) $(THREADS_LIBDIR)

Missing quotes as well (also on cd lines above and below).

Indeed, sorry about that and thanks a lot for having reported it.

This should now be fixed and the PR has been rebased on the latest
trunk.

Show outdated Hide outdated otherlibs/systhreads/Makefile
cd $(INSTALL_LIBDIR)/threads && $(RANLIB) threads.a
cp libthreadsnat.$(A) "$(INSTALL_LIBDIR)"
cd $(INSTALL_LIBDIR); $(RANLIB) libthreadsnat.$(A)
cp $(THREADS_NCOBJS) threads.cmxa threads.$(A) $(THREADS_LIBDIR)

This comment has been minimized.

@alainfrisch

alainfrisch Sep 9, 2016

Contributor

Missing quotes as well (also on cd lines above and below).

@alainfrisch

alainfrisch Sep 9, 2016

Contributor

Missing quotes as well (also on cd lines above and below).

shindere added some commits Sep 8, 2016

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).

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Sep 13, 2016

Contributor

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.

Contributor

alainfrisch commented Sep 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Sep 13, 2016

Contributor

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.

Contributor

shindere commented Sep 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Sep 13, 2016

Contributor

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!

Contributor

shindere commented Sep 13, 2016

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!

@shindere shindere deleted the shindere:merge-otherlibs-systhreads-makefiles branch Sep 14, 2016

@dra27 dra27 referenced this pull request Nov 6, 2016

Merged

Fix bootstrapping flexlink #893

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Jan 27, 2017

Member

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?

Member

diml commented Jan 27, 2017

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?

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Jan 27, 2017

Member

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

Member

diml commented Jan 27, 2017

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

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 27, 2017

Contributor

It looks like an accident - "both" platforms.

Contributor

dra27 commented Jan 27, 2017

It looks like an accident - "both" platforms.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 27, 2017

Contributor

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

Contributor

dra27 commented Jan 27, 2017

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

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 27, 2017

Contributor

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

Contributor

dra27 commented Jan 27, 2017

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

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Jan 27, 2017

Member

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

Member

diml commented Jan 27, 2017

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

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 27, 2017

Contributor

Yes to everything!

Contributor

dra27 commented Jan 27, 2017

Yes to everything!

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Jan 27, 2017

Member

Ok, fixed in 812eb68

Member

diml commented Jan 27, 2017

Ok, fixed in 812eb68

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jan 28, 2017

Contributor
Contributor

shindere commented Jan 28, 2017

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 28, 2017

Member

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.

Member

gasche commented Jan 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 28, 2017

Contributor

@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!

Contributor

dra27 commented Jan 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jan 28, 2017

Contributor
Contributor

shindere commented Jan 28, 2017

@shindere

This comment has been minimized.

Show comment
Hide comment
@shindere

shindere Jan 28, 2017

Contributor
Contributor

shindere commented Jan 28, 2017

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Jan 30, 2017

Member

@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

Member

diml commented Jan 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 1, 2017

Contributor

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

Contributor

dra27 commented Feb 1, 2017

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

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Feb 1, 2017

Member

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

Member

diml commented Feb 1, 2017

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

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Feb 1, 2017

Contributor

:) 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!

Contributor

dra27 commented Feb 1, 2017

:) 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!

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge makefiles in otherlibs/systhreads (#785)
* 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment