You should ensure that all those libraries define CAML_NAME_SPACE (as both asmrun and byterun do) to ensure no accidental use of the old names in future. I would:

1. Remove the #define CAML_NAME_SPACE in otherlibs/str/strstubs.c
2. Move the -DCAML_NAME_SPACE from EXTRACFLAGS in otherlibs/bigarray/Makefile to CFLAGS in otherlibs/Makefile.

Unrelated to your changes (but also worth fixing here), -DCAML_NAME_SPACE is also missing from CFLAGS in both asmrun/Makefile.nt and byterun/Makefile.nt

David Allsopp (2016/11/06 05:41 -0800):

You should ensure that all those libraries define CAML_NAME_SPACE
(as both asmrun and byterun do) to ensure no accidental use of the old
names in future.

I agree. it's just that I found this PR big enough and thought that
these changes could diserve another PR.

Unrelated to your changes (but also worth fixing here),
-DCAML_NAME_SPACE is also missing from CFLAGS in both
asmrun/Makefile.nt and byterun/Makefile.nt

Yes, it is actually while trying to merge the build systems in these
directories that I came across this issue. So making sure
CAML_NAME_SPACE is defined everywhere it should (in all the directories
where it makes sense and for all the build systems) is planned for
another PR, unless there is a wish that this goes in this PR as well.

Sébastien.

To me, all that's OK to have in this one PR - it fits under the heading of removing the use of compatibility macros. Those changes do belong in a separate commit, though, yes.

David Allsopp (2016/11/07 02:00 -0800):

To me, all that's OK to have in this one PR - it fits under the
heading of removing the use of compatibility macros. Those changes do
belong in a separate commit, though, yes!

The PR has been updated. Reviews welcome! Thanks!

Making CAML_NAME_SPACE always defined is breaking some of the testsuite (which can sensibly be fixed too).

However, I think the way it's being applied may be a mistake - doesn't adding it at configure also force all third party C libraries to use the caml_ versions?

David Allsopp (2016/11/07 05:41 -0800):

Making CAML_NAME_SPACE always defined is breaking some of the
testsuite (which can sensibly be fixed too).

The PR has been updated to also fix the testsuite, thanks!

However, I think the way it's being applied may be a mistake - doesn't
adding it at configure also force all third party C libraries to use
the caml_ versions?

Indeed! Many thanks for having raised this point!
PR #911 is an attempt to address it. This PR depends on it so it is
probably better not to merge this one until either #911 gets
merged or another solution can be found.

This PR has been rebased on the latest trunk and should now be ready to be
merged. Given that it should not introduce any user-visible change,
no entry has been added to Changes.

The change in configure introduced by this PR has been minimized thanks
to a suggestion made in private by @damiendoligez. Many thanks to him
for this useful feedback.

The PR should be ready for integration, now.

Gabriel Scherer (2016/11/16 13:41 -0800):

I reviewed all the identifier changes, so I can attest that they just
turn foo into caml_foo, except for mlraise => caml_raise that
(is correct but) caught me off-guard.

Thanks a lot for having taken the time to review everything! Such a
commit is big and kind of uninteresting so it's especially easy to make
mistakes, and perhaps especially boring to review and hard to see such
errors. So I really appreciate all your reviewing work! Thanks!

@@ -16,6 +16,7 @@
#define CAML_INTERNALS

#include <caml/mlvalues.h>
+#include <caml/memory.h>

I don't understand what this change is for.

Will check as soon as possible and let you know, thanks!

Gabriel Scherer (2016/11/16 13:41 -0800):

@@ -16,6 +16,7 @@
#define CAML_INTERNALS

#include <caml/mlvalues.h>
+#include <caml/memory.h>

I don't understand what this change is for.

That was to fix a warning about the implicit declaration of
caml_stat_free but it is indeed not related to this PR.

So I removed this change from the PR and will included it in a PR
dedicated to fixing this kind of warnings which will be submitted soon.

Thanks again for the careful review!

Many thanks for the merge.

Unfortunately this PR broke the Cygwin builds.

It's my fault, I took it for granted that code that compiles on Linux
would also work on Cygwin and thus I didn't test -- wrong assumption.

I am working on this right now.

What's the best way to proceed from here? Shall I submit a new PR?

I just pushed the fix-pr-892 branch on my public git repository
(it works on both Cygwin and Cygwin 64 now).

Perhaps it's enough that somebody with write access to ocaml's
repository merges or cherry-picks it on trunk?

Done, thanks!

Gabriel Scherer (2016/11/17 03:57 -0800):

Done, thanks!

Thanks, and sorry for the noise / burden.