- Notifications
You must be signed in to change notification settings - Fork1.1k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Deprecate Bigarray.*.map_file and add Unix.map_file#997
Conversation
This module contains the types that are used by the compiler toproduce optimized code for bigarrays.
and replace Bigarray by CamlinternalBigarray in bytecomp/typeopt.ml
xrefMPR6193 |
Can't easily check the details, but there are some tests which should academically be moved from lib-bigarray tests to lib-unix. Academic now, but better done here. |
The following fixes the C# Dynlink tests: diff --git a/testsuite/tests/lib-dynlink-csharp/Makefile b/testsuite/tests/lib-dynlink-csharp/Makefileindex 1b0b1da..5647ce3 100644--- a/testsuite/tests/lib-dynlink-csharp/Makefile+++ b/testsuite/tests/lib-dynlink-csharp/Makefile@@ -19,7 +19,7 @@ CSC=$(CSC_COMMAND) $(CSC_FLAGS) COMPFLAGS=-I $(OTOPDIR)/otherlibs/bigarray -I $(OTOPDIR)/otherlibs/dynlink \ -I $(OTOPDIR)/byterun-LD_PATH=$(TOPDIR)/otherlibs/bigarray:$(TOPDIR)/otherlibs/dynlink+LD_PATH=$(TOPDIR)/otherlibs/win32unix:$(TOPDIR)/otherlibs/bigarray:$(TOPDIR)/otherlibs/dynlink default: @$(SET_LD_PATH) $(MAKE) alldiff --git a/testsuite/tests/lib-dynlink-csharp/bytecode.reference b/testsuite/tests/lib-dynlink-csharp/bytecode.referenceindex 8be606c..1c61c15 100644--- a/testsuite/tests/lib-dynlink-csharp/bytecode.reference+++ b/testsuite/tests/lib-dynlink-csharp/bytecode.reference@@ -1,5 +1,6 @@ Now starting the OCaml engine. Main is running.+Loading ../../../otherlibs/win32unix/unix.cma Loading ../../../otherlibs/bigarray/bigarray.cma Loading plugin.cmo I'm the plugin.diff --git a/testsuite/tests/lib-dynlink-csharp/main.ml b/testsuite/tests/lib-dynlink-csharp/main.mlindex d30c150..7c8030a 100755--- a/testsuite/tests/lib-dynlink-csharp/main.ml+++ b/testsuite/tests/lib-dynlink-csharp/main.ml@@ -5,19 +5,21 @@ let load s = with Dynlink.Error e -> print_endline (Dynlink.error_message e)+(* Callback must be linked to load Unix dynamically *)+let _ = Callback.register+module CamlinternalBigarray = CamlinternalBigarray+ let () = ignore (Hashtbl.hash 42.0); print_endline "Main is running."; Dynlink.init (); Dynlink.allow_unsafe_modules true;- let s1,s2 =- if Dynlink.is_native then- "../../../otherlibs/bigarray/bigarray.cmxs",- "plugin.cmxs"- else- "../../../otherlibs/bigarray/bigarray.cma",- "plugin.cmo"+ let s1,s2,s3 =+ Dynlink.adapt_filename "../../../otherlibs/win32unix/unix.cma",+ Dynlink.adapt_filename "../../../otherlibs/bigarray/bigarray.cma",+ Dynlink.adapt_filename "plugin.cmo" in load s1; load s2;+ load s3; print_endline "OK."diff --git a/testsuite/tests/lib-dynlink-csharp/native.reference b/testsuite/tests/lib-dynlink-csharp/native.referenceindex ff18be9..cfb612d 100644--- a/testsuite/tests/lib-dynlink-csharp/native.reference+++ b/testsuite/tests/lib-dynlink-csharp/native.reference@@ -1,5 +1,6 @@ Now starting the OCaml engine. Main is running.+Loading ../../../otherlibs/win32unix/unix.cmxs Loading ../../../otherlibs/bigarray/bigarray.cmxs Loading plugin.cmxs I'm the plugin. The |
| underlying system calls. [Invalid_argument] or [Failure] may be | ||
| raised in cases where argument validation fails. *) | ||
| [@@deprecated "Use Unix.map_file instead"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For the sake of uniformity, please use@@ocaml.deprecated rather than@@deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should also mention here the fact that the deprecated function raisesSys_error (so that people looking at removing the deprecated function know they have to be careful about the change).
I added@dra27's patch (thanks), moved the @dra27 I though about having only a .mli for |
Is being unable to alias an internal module ever likely to be a problem? Just thinking whether it's a problem that dynlinking bigarray got a little bit harder (I don't think it is a problem, especially given its final destination of being in the stdlib, but it's a user-visible change which requires an internal module to be named in code, unless there's a better way?) |
For third-party libraries, dynlinking through |
In any case, when you use dynlink you should also use -linkall. At least you should link a linkall version of libraries that are expected to be used in plugins, so the stdlib should always be fully linked. |
@diml: Really? It potentially links a lot of code (in the test case, it increases the DLL size by 64%). @gasche: I guess in this particular case, findlib could pull in a dummy .cmx file which would ensure that CamlinternalBigarray is linked, it just feels a bit weird (weirder, anyway, than having to reference Callback in order to ensure that Unix can be loaded dynamically). |
@dra27 yes, otherwise how do you know in advance what to link in? Except if you know all your plugins before linking your program, there isn't much of a choice. And even then, -linkall is a simpler approach. |
I've preferred in the past to have the available stdlib modules specified (which can be enforced by linking a dummy module), it just seems odd to be doing that with an internal module... but maybe that's just me 😄 |
and deprecate the Bigarray.*.map_file functions.
(Path by David Allsopp)
FTR it's not that uncommon. For instance you need |
I am looking at why Camomile fails to build with 4.05 right now, and I arrived at a build failure because it depends on |
Yes, it introduces such a dependency (I was bitten by this recently). Moreover, even if we had the In any case I think thissmall amount of breakage is well worth it in order to (eventually) integrate |
This new dependency (of bigarrays on unix) is going to break most packages using bigarrays. Maybe we're doing it wrong. What I mean by that is: breaking only the packages that use Bigarray.map_file would be much less breakage overall. Let me think about that. |
Why not just duplicate the definition of |
Where do you think is the |
Inhttps://github.com/ocaml/ocaml/blob/trunk/otherlibs/bigarray/mmap_unix.c andhttps://github.com/ocaml/ocaml/blob/trunk/otherlibs/bigarray/mmap_win32.c, respectively ( |
That's right. My point is that there is no easy way to make that C implementation available both to Unix and to Bigarray without introducing dependencies of one of the libraries on the other. |
Xavier Leroy (2017/02/25 02:05 -0800): That's right. My point is that there is no easy way to make that C implementation available both to Unix and to Bigarray without introducing dependencies of one of the libraries on the other. Would it make sense to have the C function in a dedicated C source filewhose correspoindiong object file would be included in both libraries? |
Thanks for the explanation, now I understand the problem. Couldn't we move the platform-independent |
Are we still discussing this for inclusion in 4.05? Making such design choices so late in the process seems risky to me. |
If we agree on what to do by the end of the week, say, I'm confident that@diml and/or I could implement it in time for 4.05. The implementation (of either solution) is neither very hard nor very risky. But I'm fine with reverting in 4.05 and shoot for 4.06. |
I prefer@xavierleroy's alternate plan - it feels more like moving in the correct direction and leaving compatibility libraries well outside the scope of the compiler itself (cf. findlib's bytes package, etc.). Given that this is still about deprecating something, it (still) doesn't feel too risky to be doing this for 4.05 to me, but perhaps I still have my cowboy boots on! |
… continuedThis is a follow-up to GPR#997 with a different implementation that creates fewer dependencies.We add to the runtime a `byterun/bigarray.c` file that contains the bigarray creation functions that used to be in `otherlibs/bigarray/bigarray_stubs.c`. In the latter file we keep all primitives needed to implement the Bigarray interface. The functions in the new `byterun/bigarray.c` make it possible to create bigarrays both from the bigarray library and from the unix library.The header file `bigarray.h` moves to `byterun/caml/bigarray.h` accordingly.The `map_file` implementations move to `otherlibs/unix/mmap.c` and `otherlibs/win32unix/mmap.c`. Some bigarray allocation code shared between the two implementations is put in `otherlibs/unix/mmap_ba.c`.Through a couple of `#ifdef`, the `map_file` implementations can also be compiled from within `otherlibs/bigarray` with the same semantics they had in 4.04.As a consequence, the bigarray library still contains a standalone, Unix-independent implementation of `Bigarray.*.map_file`; the only difference with 4.04 is that it is marked deprecated.Current status: compiled and lightly tested under Unix. Win32 implementation neither compiled nor tested.
A proposal in GPR#1077. |
This reverts commit5ed7200.GPR#997 introduced a hard dependency of Bigarray on Unix, while therepreviously only was a type-level dependency. This break some programs,such as Camomile, that linked bigarray.cma but not unix.cma.A solution is being worked out in GPRocaml#1077 to remove the dependency,but I would like to go forward with opam package testing on the 4.05branch, and getting the details right for GPR#1077 requires some care,so I wouldn't feel comfortable rushing to merge it.I had to handle the following conflicts:otherlibs/unix/unix.mli ("@SInCE 4.05.0" was added infaab91a)testsuite/tests/lib-bigarray-file/mapfile.ml (changed by5839c98; I kept and adapted the new version)testsuite/tests/lib-bigarray-file/mapfile.reference
This reverts commit5ed7200.GPR#997 introduced a hard dependency of Bigarray on Unix, while therepreviously only was a type-level dependency. This break some programs,such as Camomile, that linked bigarray.cma but not unix.cma.A solution is being worked out in GPRocaml#1077 to remove the dependency,but I would like to go forward with opam package testing on the 4.05branch, and getting the details right for GPR#1077 requires some care,so I wouldn't feel comfortable rushing to merge it.I had to handle the following conflicts:otherlibs/unix/unix.mli ("@SInCE 4.05.0" was added infaab91a)testsuite/tests/lib-bigarray-file/mapfile.ml (changed by5839c98; I kept and adapted the new version)testsuite/tests/lib-bigarray-file/mapfile.reference
… continuedThis is a follow-up to GPR#997 with a different implementation that creates fewer dependencies.We add to the runtime a `byterun/bigarray.c` file that contains the bigarray creation functions that used to be in `otherlibs/bigarray/bigarray_stubs.c`. In the latter file we keep all primitives needed to implement the Bigarray interface. The functions in the new `byterun/bigarray.c` make it possible to create bigarrays both from the bigarray library and from the unix library.The header file `bigarray.h` moves to `byterun/caml/bigarray.h` accordingly.The `map_file` implementations move to `otherlibs/unix/mmap.c` and `otherlibs/win32unix/mmap.c`. Some bigarray allocation code shared between the two implementations is put in `otherlibs/unix/mmap_ba.c`.Through a couple of `#ifdef`, the `map_file` implementations can also be compiled from within `otherlibs/bigarray` with the same semantics they had in 4.04.As a consequence, the bigarray library still contains a standalone, Unix-independent implementation of `Bigarray.*.map_file`; the only difference with 4.04 is that it is marked deprecated.Current status: compiled and lightly tested under Unix. Win32 implementation neither compiled nor tested.
Revert "Deprecate Bigarray.*.map_file and add Unix.map_file (#997)"This revert is intended to unblock the OPAM package testing of 4.05.
… continuedThis is a follow-up to GPR#997 with a different implementation that creates fewer dependencies.We add to the runtime a `byterun/bigarray.c` file that contains the bigarray creation functions that used to be in `otherlibs/bigarray/bigarray_stubs.c`. In the latter file we keep all primitives needed to implement the Bigarray interface. The functions in the new `byterun/bigarray.c` make it possible to create bigarrays both from the bigarray library and from the unix library.The header file `bigarray.h` moves to `byterun/caml/bigarray.h` accordingly.The `map_file` implementations move to `otherlibs/unix/mmap.c` and `otherlibs/win32unix/mmap.c`. Some bigarray allocation code shared between the two implementations is put in `otherlibs/unix/mmap_ba.c`.Through a couple of `#ifdef`, the `map_file` implementations can also be compiled from within `otherlibs/bigarray` with the same semantics they had in 4.04.As a consequence, the bigarray library still contains a standalone, Unix-independent implementation of `Bigarray.*.map_file`; the only difference with 4.04 is that it is marked deprecated.Current status: compiled and lightly tested under Unix. Win32 implementation neither compiled nor tested.
map_file is missing from unixLabels.ml |
Just to make sure: is it on purpose that no camlinternalBigarray.mli has |
It's because it was the same as the |
Right, camlinternalBigarray contains only type declarations, so the .ml and the .mli would be identical. |
Would it create problem to have only the .mli, then? (As we do e.g. for Parsetree.) |
With a .mli and no .ml, there is no .cmx and a chance of getting a warning "No cmx found". In this particular case, the chance is pretty low, I think. |
In general I try to avoid mli only modules as they are not very well supported; you can't alias them and if you write something such as |
To break the circular dependency between Bigarray and Unix, a CamlinternalBigarray module was added to the stdlib. This module defines all the types used by the compiler to produce optimized code for bigarrays.Thanks to David Allsopp for fixing Windows tests.
This reverts commit5ed7200.GPR#997 introduced a hard dependency of Bigarray on Unix, while therepreviously only was a type-level dependency. This break some programs,such as Camomile, that linked bigarray.cma but not unix.cma.A solution is being worked out in GPRocaml#1077 to remove the dependency,but I would like to go forward with opam package testing on the 4.05branch, and getting the details right for GPR#1077 requires some care,so I wouldn't feel comfortable rushing to merge it.I had to handle the following conflicts:otherlibs/unix/unix.mli ("@SInCE 4.05.0" was added infaab91a)testsuite/tests/lib-bigarray-file/mapfile.ml (changed by5839c98; I kept and adapted the new version)testsuite/tests/lib-bigarray-file/mapfile.reference
Revert "Deprecate Bigarray.*.map_file and add Unix.map_file (ocaml#997)"This revert is intended to unblock the OPAM package testing of 4.05.
…hrough functor arguments (ocaml#997)
b11eea1 flambda-backend: Introduce Import_info (ocaml#1036)bc5b135 flambda-backend: Fix `ocamlobjinfo` on flambda2 .cmx files (ocaml#1029)c8babbd flambda-backend: Compilation_unit optimisations (ocaml#1035)e8d3e22 flambda-backend: Use 4.14.0 opam switch for building (includes upgrading ocamlformat to 0.24.1) (ocaml#1030)eb14a86 flambda-backend: Port PR81 from ocaml-jst (ocaml#1024)131bc12 flambda-backend: Merge ocaml-jst 2022-12-13 (ocaml#1022)06c189a flambda-backend: Make stack allocation the default (ocaml#1013)98debd5 flambda-backend: Initial support for value slots not of value kind (ocaml#946)deb1714 flambda-backend: Add is_last flag to closinfo words (ocaml#938)d07fce1 flambda-backend: Disable poll insertion in Configure (ocaml#967)0f1ce0e flambda-backend: Regenerate ocaml/configure autoconf 2.69 (instead of 2.71) (ocaml#1012)27132d8 flambda-backend: Fix for spurious typing error related to expanding through functor arguments (ocaml#997)724fb68 flambda-backend: Use `Compilation_unit.t` instead of `Ident.t` for globals (ocaml#871)396d5b8 flambda-backend: Add a test for frametable setup in natdynlinked libraries (ocaml#983)b73ab12 flambda-backend: Fix invocation of `caml_shared_startup` in native dynlink (ocaml#980)7c7d75a flambda-backend: Fix split_default_wrapper which did not trigger anymore with flambda2 (ocaml#970)8fb75bd flambda-backend: Portocaml#11727 andocaml#11732 (ocaml#965)fdb7987 flambda-backend: Fix include functor issue after 4.14 merge. (ocaml#948)9745cdb flambda-backend: Print -dprofile/-dtimings output to stdout like 4.12 (ocaml#943)5f51f21 flambda-backend: Merge pull requestocaml#932 from mshinwell/4.14-upgrade841687d flambda-backend: Run make alldepend in ocaml/ (ocaml#936)72a7658 flambda-backend: Remove reformatting changes only in dynlink/dune (preserving PR889 and adjusting to minimise diff)6d758cd flambda-backend: Revert whitespace changes in dune files, to match upstreamc86bf6e flambda-backend: Remove duplicate tests for polling971dbeb flambda-backend: Testsuite fixes32f8356 flambda-backend: Topeval fix for symbols patchbefea01 flambda-backend: Compilation fixes / rectify merge faultsa84543f flambda-backend: Merge ocaml-jst8e65056 flambda-backend: Merge ocaml-jst4d70045 flambda-backend: Remove filename from system frametable (amd64) (ocaml#920)5e57b7d flambda-backend: Bugfix for runtime frame_descr logic for C frames (ocaml#918)6423d5e flambda-backend: Merge pull requestocaml#914 from mshinwell/merge-ocaml-jst-2022-10-24ead605c flambda-backend: Add a missing Extract_exception (ocaml#916)c8f1481 flambda-backend: Resolve conflicts and add specialise/specialised attributes to Builtin_attributescf4d0d3 flambda-backend: Merge fixes (ocaml#21)c2f742f flambda-backend: Re-enable some tests for Flambda2 (ocaml#881)3d38d13 flambda-backend: Long frames in frametable (ocaml#797)85aec7b flambda-backend: Add loop attribute to Builtin_attributesc0f16e3 flambda-backend: Compilation fixes90dea23 flambda-backend: Merge flambda-backend/main5acc6ea flambda-backend: Fixes after mergee501946 flambda-backend: Merge ocaml-jst115083b flambda-backend: Merge ocaml-jst9943b2e flambda-backend: Revert "Revert "Transform tail-recursive functions into recursive continuations (ocaml#893)"" (ocaml#909)ce339f1 flambda-backend: Fix alloc modes and call kinds for overapplications (ocaml#902)e6a317c flambda-backend: Revert "Transform tail-recursive functions into recursive continuations (ocaml#893)"853c488 flambda-backend: Transform tail-recursive functions into recursive continuations (ocaml#893)5a977e4 flambda-backend: Fix missing End_region primitives on switch arms (ocaml#898)7fa7f9d flambda-backend: Add missing dependencies to Dune files (ocaml#889)3cd36f0 flambda-backend: Have Lambda `Pgetglobal` and `Psetglobal` take `Compilation_unit.t` (ocaml#896)7565915 flambda-backend: [@poll error] attribute (ocaml#745)9eb9448 flambda-backend: Backport the main safepoints PRs (ocaml#740)689bdda flambda-backend: Add strict mode for ocamldep (ocaml#892)git-subtree-dir: ocamlgit-subtree-split:b11eea1
As discussed during the last developer meeting, I looked at deprecating the
Bigarray.*.map_filefunctions in favour ofUnix.map_file. This will allow to moveBigarrayto the stdlib later.There were two problems:
map_filerefers to bothUnix.file_descrand the bigarray typesmap_fileneeds things from bigarray.cOnce the
Bigarray.*.map_filefunctions are removed these won't be a problem but in the meantime I did the following to break the circular dependency:Added
CamlinternalBigarrayto the stdliband moved the bigarray types there. Given that these types are interpreted in a special way by the compiler, it seems to me that it make sense to have them in a special place anyway.
I looked at making them predefined types, but this would have added a lot of code to
typing/predef.mland a lot of types and constructors to the default environment. Moreover we already do something similar for format strings withCamlinternalFormatBasics.Kept the stubs for map_file in otherlibs/bigarray
Moving them to otherlibs/unix would have required quite a lot of refactoring which seemed a bit silly to me given that this is only temporary. Instead I did something dirty and added a
map_file_implreference in Unix that is initialised by the Bigarray module.Other changes
I changed
caml_ba_map_fileto raiseUnix_errorinstead ofSys_error. This seems more natural now that the function is in the Unix module. The deprecatedBigarray.*.map_filefunctions still convert this exception to aSys_errorone for backward compatibility.Future
Once the
Bigarray.*.map_fileare removed and Bigarray is moved to the stdlib, there will still be an issue: the bigarray finalizer callscaml_ba_unmap_file, which should naturally go into the Unix library at the side ofcaml_ba_map_file. At this point we can either:caml_ba_unmap_fileand initialize it incaml_ba_map_filecaml_ba_arrayto speficy a custom destroyer