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.

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

Add bigarray to the stdlib #1685

Merged
merged 2 commits into from Apr 9, 2018

Conversation

Projects
None yet
7 participants
@diml
Copy link
Member

diml commented Mar 30, 2018

This PR is the next step in moving Bigarray to the Standard Library.

Overview

It moves the Bigarray module to the standard library without the map_file functions. The bigarray library is kept for compatibility purposes and simply extends Stdlib.Bigarray with the deprecared map_file functions. This way users have more time to update their code, especially the ones who want to keep compatibility with OCaml < 4.06.0.

Other relevant details:

  • otherlibs/bigarray/bigarray_stubs was appended at the end of byterun/bigarray.c
  • the CamlinternalBigarray module was removed since it is no longer needed

Limitation

Because all the cmi files are installed in the same directory, in the initial environment the module name Bigarray always resolves to Bigarray from the bigarray library rather than Stdlib.Bigarray. One has to use Stdlib.Bigarray explicitely in order to use bigarrays without depending on unix.

Reviewing

Because some code was moved around, the diff shows a lot of added and deleted code. It is easier to review the following files with these commands:

  • stdlib/bigarray.ml: patdiff <(git show trunk:otherlibs/bigarray/bigarray.ml) stdlib/bigarray.ml
  • stdlib/bigarray.mli: patdiff <(git show trunk:otherlibs/bigarray/bigarray.mli) stdlib/bigarray.mli
  • byterun/bigarray.c: patdiff <(git show trunk:byterun/bigarray.c; git show trunk:otherlibs/bigarray/bigarray_stubs.c) byterun/bigarray.c
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Mar 30, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Mar 31, 2018

Indeed, I removed the modifier

@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Apr 2, 2018

Assuming that repeatably fixes it, feel free to squash that commit into 97e616c

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 4, 2018

I am reviewing this.

@mshinwell mshinwell force-pushed the diml:move-bigarray-to-the-stdlib branch from e7767ec to d3cb662 Apr 9, 2018

@mshinwell mshinwell force-pushed the diml:move-bigarray-to-the-stdlib branch from d3cb662 to c264c55 Apr 9, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

This is going to produce an unrelated test failure which I have emailed @shindere about, but this patch is OK, and we should merge before 4.07. A bootstrap commit will be needed to remove some redundant primitives; I will do this.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

The only failure for a reasonable set of targets appears to be the one which @shindere is currently working on, and I need to follow this with a manual bootstrap commit anyway, so I am going to merge this now.

@mshinwell mshinwell merged commit 32da45a into ocaml:trunk Apr 9, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Apr 9, 2018

The bootstrap which completes this GPR is in e866ba3f6891a18ede3235f125babd5f7fd6d03a.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented Apr 9, 2018

This closes MPR6139

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 9, 2018

Thanks @dbuenzli, I'll add the mantis issue to the Changes entry.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@diml sorry for asking this question so late.

Am I correct that, as far as the source tree is concerned, the bigarray
files were not moved to the stdlib directory?

May I ask why and whether there is any plan to do so in a further PR?

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 18, 2018

They were moved, however the bigarray library is kept for compatibility. The Bigarray.*.map_file functions were deprecated in 4.06, so we just need to wait a reasonable amount of time before deleting the bigarray library entirely. I assume that deleting it in 4.08 should be ok.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 18, 2018

You should just use the Bigarray module from the stdlib, so you don't need any special cma. However, if you pass -I [...]/stdlib -I [...]/otherlibs/bigarray to the compiler, you need to use Stdlib.Bigarray explicitly in your code in order to refer to the bigarray module from the stdlib. After installing, you always need to use Stdlib.Bigarray. This is because the bigarray.cmi file in the search path has precedence over Stdlib.Bigarray. bigarray.cmi is from the bigarray library.

If you do want to use the deprecated bigarray library, then you need to to use bigarray.cma as before.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 18, 2018

Ok, this test should still work as bigarray.cma still exists at the same place.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 18, 2018

Because it's not part of the stdlib :)

Stdlib.Bigarray, like all other stdlib modules, is compiled as Stdlib__Bigarray and ends up part of stdlib.cma/cmxa. bigarray.cma is only the deprecated bigarray library, which simply extends Stdlib.Bigarray with the deprecated functions.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 18, 2018

Perhaps the tes will simply no longer need to load it and will continue
to work?

I believe so. It should even work right now

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

@diml and @alainfrisch :

I am assumiing that unix.cma and unix.cmxs were loaded only to support
bigarray and that they are thus not needed any longer either. Is that
correct?

I hope stopping to load them does not defeat the purpose of the test itself
but I don't think so. Would just appreciate this to be confirmed.

@diml

This comment has been minimized.

Copy link
Member Author

diml commented Apr 18, 2018

I admit I don't really know what this test is doing, but I assume that it should indeed load a few files. Maybe the plugin should be updated to test a different function

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented Apr 18, 2018

hannesm added a commit to hannesm/ocaml-freestanding that referenced this pull request Jun 20, 2018

OCaml 4.07 support
Relevant changes in 4.07
- unix.c now #include <sys/ioctl.h> (since 852b595ff3), but an empty one is fine
  (only use is in caml_nun_rows_fd in an #ifdef TIOCGWINSZ), see
  ocaml/ocaml#1431
  -> provide an empty ioctl via nolibc
- bigarray is now part of the stdlib
  ocaml/ocaml#1685
  -> libotherlibs.a is no longer needed (neither built nor linked)

@hannesm hannesm referenced this pull request Jun 20, 2018

Merged

OCaml 4.07 support #39

hannesm added a commit to hannesm/ocaml-freestanding that referenced this pull request Jul 14, 2018

OCaml 4.07 support
Relevant changes in 4.07
- unix.c now #include <sys/ioctl.h> (since 852b595ff3), but an empty one is fine
  (only use is in caml_nun_rows_fd in an #ifdef TIOCGWINSZ), see
  ocaml/ocaml#1431
  -> provide an empty ioctl via nolibc
- bigarray is now part of the stdlib
  ocaml/ocaml#1685
  -> libotherlibs.a is no longer needed (neither built nor linked)

To support these changes, an empty sys/ioctl.h is added to nolibc.  Also,
`ocaml-freestanding.pc.in` now contains in `Libs`: -L${libdir} -lasmrum -lnolibc
-lopenlibm (instead of ${libdir}/lib for each library).  `configure.sh` adds
-lotherlibs to these flags in the case that OCAML_GTE_4_07_0 is false.  The
`Makefile` treats libotherlibs.a as dependency only if OCAML_GTE_4_07_0 is false.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.