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.

Sign up for GitHub

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

Jump to bottom

otherlibs: Merge win32unix into unix #11160

Merged
merged 2 commits into from
Apr 10, 2022
Merged

otherlibs: Merge win32unix into unix #11160

merged 2 commits into from
Apr 10, 2022

Conversation

shindere
Copy link
Contributor

@shindere shindere commented Apr 4, 2022

Build the unix library in the otherlibs/unix directory no matter whether
it's the unix or win32 flavour.

This simplifies the compiler's build system a bit, see for instance
how the recipe for the depend target in debugger/Makefile becomes
more straightforward.

The unixsupport.h header files of the two directories have been merged.

The declaration of the unix_freeze_buffer funciton has been removed
since it is mentionned nowhere else in the code base.

This PR goes through precheck successfully, se Build #679.

@dra27
Copy link
Member

dra27 commented Apr 4, 2022

I will aim to review this in the morning, but there aren’t enough emoji to express how happy this PR makes me!

@shindere
Copy link
Contributor Author

shindere commented Apr 5, 2022 via email

nojb
nojb reviewed Apr 5, 2022
View reviewed changes
otherlibs/unix/Makefile

CAMLOBJS=unix.cmo unixLabels.cmo

HEADERS=unixsupport.h socketaddr.h

include ../Makefile.otherlibs.common

unix.ml: unix_$(UNIX_OR_WIN32).ml
cp $< $@
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought unix.ml was created by configure fromunix.ml.in below. Did I miss something?

@shindere
Copy link
Contributor Author

shindere commented Apr 5, 2022 via email

@shindere shindere force-pushed the win32unix branch 2 times, most recently from a879d38 to 0f8b494 Compare April 5, 2022 12:55
@shindere
Copy link
Contributor Author

shindere commented Apr 5, 2022

Just rebased on trunk, after the merge of #10697.

@MisterDA: owuld you please mind checking that the rebase is correct with
respect to your work?

dra27
dra27 reviewed Apr 5, 2022
View reviewed changes
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.

This is great, @shindere!

In many instances, the C files could in future be further merged (the use of char_os and so forth means that many of the differences aren't as #ifdef-y as one might fear). Partly with this longer-term goal in mind, and partly to reduce the diff slightly, could it be that we have select.c as the "POSIX" file and select_win32.c as the Windows specific one, etc.?

I also double-checked in precheck that the tests being skipped are the same (which they are) - so the change hasn't affected what the testsuite is running.

.gitignore Outdated
Comment on lines 143 to 145
/otherlibs/threads/marshal.mli
/otherlibs/threads/stdlib.mli
/otherlibs/threads/unix.mli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not part of this PR, but if I open a PR with it will create a conflict - these 3 lines should have been deleted in 4.09!

Makefile Outdated
@@ -83,7 +83,7 @@ TOPINCLUDES=$(addprefix -I otherlibs/,$(filter-out %threads,$(OTHERLIBRARIES)))
ifeq "$(UNIX_OR_WIN32)" "unix"
EXTRAPATH=
else
EXTRAPATH = PATH="otherlibs/win32unix:$(PATH)"
EXTRAPATH = PATH="otherlibs/unix:$(PATH)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this has ever been a necessary thing to do, right since it was added between 4.04 and 4.05 - the only purpose of this would be to add DLLs to PATH, but the loader never searches PATH for them. Rather than continuing to update it, how about we just remove EXTRAPATH completely?

I have quickly double-checked - it's certainly working as expected on trunk without this (even setting CAML_LD_LIBRARY_PATH to point to an incompatible stub library, for example) and it even worked that way in 4.05 when it was added! The -I otherlibs/unix should cause the toplevel to prefer otherlibs/unix/dllunix.dll in the tree, unless I really need more coffee...

Makefile Outdated
@@ -809,8 +809,7 @@ clean::
$(MAKE) -C runtime clean
rm -f stdlib/libcamlrun.a stdlib/libcamlrun.lib

otherlibs_all := dynlink \
str systhreads unix win32unix
otherlibs_all := dynlink str systhreads unix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated in otherlibs/Makefile - worth introducing OTHERLIBS_ALL in Makefile.common??

Makefile.config.in Outdated
@@ -181,7 +181,7 @@ PTHREAD_CAML_LIBS=$(addprefix -cclib ,$(PTHREAD_LIBS))
PTHREAD_CFLAGS=@PTHREAD_CFLAGS@

UNIX_OR_WIN32=@unix_or_win32@
UNIXLIB=@unixlib@
UNIXLIB=unix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be moved to the "deprecated" part at the bottom - it's no longer used in the build system and anything which was relying on it (for whatever reason) is going to break!

api_docgen/Makefile.common Outdated
Comment on lines 26 to 27
otherlibs/str otherlibs/unix otherlibs/dynlink \
otherlibs/systhreads)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming we do have a common definition OTHERLIBS_ALL in Makefile.common, this could then become:

Suggested change
otherlibs/str otherlibs/unix otherlibs/dynlink \
otherlibs/systhreads)
$(addprefix otherlibs/, $(OTHERLIBS_ALL)))

otherlibs/unix/Makefile Outdated
# Note: at the moment, we need to include the configuration makefile directly
# even before we can include Makefile.otherlibs.common
# because the UNIX_OR_WIND32 variable must be defined even before
# we can include Makefile.otherlibs.config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Makefile.otherlibs.common, not config

otherlibs/unix/Makefile Show resolved Hide resolved
otherlibs/unix/Makefile Show resolved Hide resolved
otherlibs/unix/unixsupport.h Outdated
Comment on lines 89 to 95
#ifdef _WIN32
extern wchar_t ** cstringvect(value arg, char * cmdname);
extern void cstringvect_free(wchar_t **);
#else
extern char ** cstringvect(value arg, char * cmdname);
extern void cstringvect_free(char **);
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if this used char_os instead:

Suggested change
#ifdef _WIN32
extern wchar_t ** cstringvect(value arg, char * cmdname);
extern void cstringvect_free(wchar_t **);
#else
extern char ** cstringvect(value arg, char * cmdname);
extern void cstringvect_free(char **);
#endif
extern char_os ** cstringvect(value arg, char * cmdname);
extern void cstringvect_free(char_os **);

however, that requires caml/misc.h and I'm not sure if there are any implications to adding that here? (I don't think there are??)

otherlibs/unix/unixsupport.h

#ifdef __cplusplus
}
#endif

#ifdef _WIN32
#define EXECV_CAST (const char_os * const *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, apropos above, caml/misc.h is obviously loaded before unixsupport.h, or this line from the win32unix's unixsupport.h would already be failing. Perhaps it should be made official by adding #include <caml/misc.h> here, though.

MisterDA
MisterDA approved these changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@MisterDA MisterDA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nice, I don't think my previous patches are affected in any (bad) way.

otherlibs/unix/unixsupport.h Outdated
Comment on lines 110 to 117
#ifdef _WIN32
/* Information stored in flags_fd, describing more precisely the socket
* and its status. The whole flags_fd is initialized to 0.
*/

/* Blocking or nonblocking. By default a filedescr is in blocking state */
#define FLAGS_FD_IS_BLOCKING (1<<0)
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section could be moved closer to the struct filedescr definition.

@MisterDA
Copy link
Contributor

MisterDA commented Apr 5, 2022

The CI says make alldepend should be run. Is that missing from the PR?

@dra27
Copy link
Member

dra27 commented Apr 5, 2022 •
edited

No, it needs a little tweak:

diff --git a/Makefile b/Makefile
index 2a9059c9f4..c59429e148 100644
--- a/Makefile
+++ b/Makefile
@@ -810,8 +810,8 @@ clean::
        rm -f stdlib/libcamlrun.a stdlib/libcamlrun.lib

 otherlibs_all := dynlink str systhreads unix
-subdirs := debugger lex ocamldoc ocamltest stdlib tools \
-  $(addprefix otherlibs/, $(otherlibs_all)) \
+subdirs := lex stdlib tools $(addprefix otherlibs/, $(otherlibs_all)) \
+  debugger ocamldoc ocamltest

 .PHONY: alldepend
 alldepend: depend

The point is that before computing the debugger's dependencies, you need otherlibs/unix's to have been computed in order to generate unix.ml. I'd recommend a workaround like the above rather than hammering in a dependency between debugger/Makefile and otherlibs/unix/Makefile just for alldepend (at least for now).

configure's also been regenerated with autoconf 2.71 rather than 2.69 (come on, Ubuntu 22.04!!)

@shindere
Copy link
Contributor Author

shindere commented Apr 5, 2022 via email

@lthls
Copy link
Contributor

lthls commented Apr 5, 2022

Also, if you have a look to the diff, it seems weird because it's about the dependency of a .cmx file. It wants to replace one .cmx prereq by a .cmi prereq, which I find mysterious. It makes no sense to me.

That's what @dra27 was referring to: if at the time you compute the dependencies in ocamldebug, otherlibs/unix/unix.ml doesn't exist, ocamldep cannot register dependencies to the .cmx file, only on the .cmi file (as unix.mli is present). In your local copy, I assume that you have built everything so the unix.ml file is here, and make alldepend produces the expected result, but on the CI there's only a minimal build (that doesn't include the Unix library), and the ocamldebug dependencies are compiled too early, so you get the result shown in the log.

@xavierleroy
Copy link
Contributor

I don't know why, but I was expecting a reorganization based on directories:

   otherlibs/unix
   otherlibs/unix/common
   otherlibs/unix/posix
   otherlibs/unix/win32

with unix.mli, unixsupport.h, Makefile, and the compiled libraries in otherlibs/unix, and most of the other files in subdirectories. Not sure it makes much of a difference, though.

@shindere
Copy link
Contributor Author

shindere commented Apr 6, 2022 via email

@shindere
Copy link
Contributor Author

shindere commented Apr 6, 2022 via email

@shindere
Copy link
Contributor Author

shindere commented Apr 6, 2022 via email

@shindere shindere force-pushed the win32unix branch 3 times, most recently from aeb4ee5 to b03cfed Compare April 6, 2022 13:34
@shindere
Copy link
Contributor Author

shindere commented Apr 6, 2022

The latest version of the PR is currently going through precheck on Inria's CI, see build #682.

dra27
dra27 approved these changes Apr 7, 2022
View reviewed changes
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.

Thanks for the changes @shindere - apart from possibly re-inserting one @ symbol, this looks go to go for me.

GitHub actions was checking the configure script - the regeneration check had failed in addition to the alldepend check.

No problem with not renaming the files further - of course, arguing with myself, having select_unix.c and select_win32.c also serves as a reminder that there are probably two files to update with any changes.

Anything which was relying on $(UNIXLIB) in Makefile.config would could have been using it to differentiate between unix and win32unix (just like our testsuite), but I think it's unlikely to have been being used that way "in the wild".

For ocamltest, let's leave this to another PR. I had not realised that windows and not-windows were applying to Cygwin - the windows action is not presently used in any tests and a quick look at all the uses of not-windows suggests that most, if not all, of them are incorrect (i.e. not-windows is excluding Cygwin where Cygwin should be being tested). I have always preferred to explain Cygwin as being Unix - i.e. it's Unix which just happens to be running on Windows, so maybe we can get away with just changing windows to mean msvc+mingw. Anyway, for the future.

No problem to wait for fixing otherlibs/unix/Makefile until otherlibs/Makefile.otherlibs.common is eliminated.

otherlibs/Makefile
# $1: target name to dispatch to all otherlibs/*/Makefile
define dispatch_
$1:
@for lib in $$(OTHERLIBRARIES); do \
for lib in $$(OTHERLIBRARIES); do \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I don't think we need to lose that @ here - each one of the recursive calls to make generates the correct Entering directory message and the loop is also careful to exit on any error. Of course, it would be even nicer if it were generated using $(for but that's not important right now ;)

@shindere
Copy link
Contributor Author

shindere commented Apr 7, 2022 via email

@dra27
Copy link
Member

dra27 commented Apr 8, 2022

I don't mind with the @ - mostly, I prefer shorter build logs, but not enough to be insisting!

@shindere
.gitignore cleanup
776f336 
Delete three files that should have been removed in 4.09.

Suggestion by David Allsopp.
@shindere
otherlibs: Merge win32unix into unix
988349d
@shindere shindere merged commit 3f459df into ocaml:trunk Apr 10, 2022
@shindere shindere deleted the win32unix branch April 10, 2022 09:15
shindere added a commit that referenced this pull request Apr 11, 2022
@shindere
Fix otherlibs/Makefile (follow-up to #11160).
146766c 
In this file, the clean targets depend on the OTHERLIBRARIES variable.

However, this variable is defined in `Makefile.config`, which is not
included when make is called for cleaning, so that the clean does
actually not happen (OTHERLIBRARIES being empty in that case).
@shindere
Copy link
Contributor Author

I just pushed 146766c to fix the bootstrap.

The problem is explained both in the commit message and as a comment in the source code. Apologies for the noise.

@dra27 dra27 mentioned this pull request May 3, 2022
Merged
@shindere
Copy link
Contributor Author

shindere commented Oct 11, 2022 via email

@shindere
Copy link
Contributor Author

shindere commented Oct 11, 2022 via email

@dra27 dra27 mentioned this pull request Dec 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nojb nojb nojb left review comments

@MisterDA MisterDA MisterDA approved these changes

@dra27 dra27 dra27 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@shindere @dra27 @MisterDA @lthls @xavierleroy @nojb