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

Delete the deprecated Bigarray.*.map_file functions #2263

Merged
merged 2 commits into from Feb 26, 2019

Conversation

Projects
None yet
5 participants
@diml
Copy link
Member

commented Feb 25, 2019

These functions have been deprecated since 4.06.0, which was released in November 2017. That seems like enough notice to delete them.

@diml diml force-pushed the diml:delete-Bigarray.X.map_file branch from 46f8afd to b0d2151 Feb 25, 2019

Show resolved Hide resolved otherlibs/bigarray/Makefile Outdated
@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

Assuming testsuite pass, etc., this LGTM - up to you if you want the extra commit updating the otherlibs Makefile.

I wouldn't put this in 4.08, personally - I think it's fine to announce that the functions are effectively not present (but available with a workaround for the belligerent), and then definitely deleted in 4.09.

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

https://gist.github.com/4d8edbcf47138253509505166a57e68d

Here's the results of a quick map_file grep over opam source of the latest versions of packages using grep -r map_file *|grep Bigarray (so this doesnt cover cases where Bigarray has been opened previously)

Looks like a fair few packages still using the old form. The full extent will become more obvious once we get a working ocaml-migrate-parsetree with 4.08 and can unblock the bulk opam builds for 4.08+beta2

Show resolved Hide resolved Changes Outdated
@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

We can learn a valuable lesson from all this: when deprecating a feature, it is important to specify when the feature will effectively go away.

@diml diml force-pushed the diml:delete-Bigarray.X.map_file branch from 5dc1a14 to 66c0415 Feb 25, 2019

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2019

@avsm what do you conclude from this grepping?

In 4.08, the Bigarray.*.map_file functions are not accessible anyway. As @dra27 suggests, there is an ugly workaround to access them (that I don't recommend using), so in any cases released packages will need to be updated in order to be compatible with 4.08.

So personally I still don't see the harm of deleting this function in 4.08. At least it makes things clear.

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

We can learn a valuable lesson from all this: when deprecating a feature, it is important to specify when the feature will effectively go away.

Indeed, that would be extremely useful to specify in the original deprecation warning too.

what do you conclude from this grepping?

I think several of the packages that currently use it will need to support compiling with ocaml<4.06.0 (such as opam), frustrating though this is. Would it be possible to have a stdlib shim that will expose the right map_file function for the current compiler, and patch packages to use that instead?

To be clear, I think that this PR is the right thing to do in this case, but a compat shim for pre<4.06>=4.08 compilers would make life vastly easier for package maintainers.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

I suppose. So the proposed plan is:

  • merge this in trunk and 4.08
  • provide a package Bigarray_with_map_file that has the same API as Bigarray in 4.07

This is similar to what @damiendoligez suggested, except that the compatibility code for the old map_file functions goes in the external package, leaving the compiler in a better state right now.

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

How about calling the package Unix_with_map_file instead, so that it forces people to move to the foo_of_genarray (Unix.map_file) mode now. Then when they want to drop <4.06 they can just replace Unix_with_map_file with Unix.map_file

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@diml was suggesting a third-party package with the Bigarray interface, so naming it Unix sounds weird. Maybe the third-party package could expose only the map_file function? Then @avsm's naming scheme would work, but something simple like Mmap.genarray_of_file or Mmap.file could also make sense.

avsm added a commit to avsm/ocaml-cstruct that referenced this pull request Feb 26, 2019

`Cstruct_unix` now uses the post-OCaml 4.06 `Unix.map_file` instead o…
…f the deprecated Bigarray `map_file` that was removed in OCaml 4.08 (see ocaml/ocaml#2263)
@avsm

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Yes just exposing only the Unix.map_file function is what I intended, so your names sound better.

@avsm avsm referenced this pull request Feb 26, 2019

Merged

prepare 3.5.0 release #224

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Just adding an echo for the shim adding Unix.map_file, rather than the other way around - it seems a double-win to do it that way, as the shim never affects newer versions of OCaml and it's forcing adoption of the new API in old versions rather than providing a way of accessing the old API in new versions.

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

I wish we were more systematic in proposing compatibility shims right away, or interacting early with people who already do the work of building those shims (eg. stdcompat).

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Ok, that makes sense. However, I suggest to call the module Mmap_shims. If we give it a nice short name, it won't feel like a transient package.

Delete the deprecated Bigarray.*.map_file functions
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>

@diml diml force-pushed the diml:delete-Bigarray.X.map_file branch from 66c0415 to c089fde Feb 26, 2019

@gasche

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

Do we actually need the package to be transient? It's fine if users depend on it indefinitely.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Ok, so there are no objection to merging this in 4.08 then. I moved the changelog entry to 4.08 as a result.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@gasche it's just that it doesn't bring anything compared to Unix.map_file.

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

There is one reason to call it just Mmap -- if it's just a single module with the map_file function, then it's easier for us to implement it in Mirage in the future without a full dependency on Unix.

An Mmap_xen module for instance could take advantage of virtual_modules in Dune and implement the map_file interface now that Bigarray is Unix-independent. This is why I'm in favour of breaking up the monolithic Unix module where practical, as it makes modular use of Unix facilities significantly easier.

(edit: I realise there is still a dependency on Unix.fd in the interface -- we work around that with a -dontlink unix hack right now which isn't ideal... but at least it doesn't pull in the full Unix.cmxa)

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Ok, that makes sense.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

I created the package: https://github.com/diml/mmap

@avsm if you want to integrate it in the mirage suite, I'm happy to transfer the project to mirage.

@anmonteiro

This comment has been minimized.

Copy link

commented Feb 26, 2019

I can help do this for https://github.com/mirage/mirage-platform as I recently submitted a PR for 4.08 integration (mirage/mirage-platform#206).

Once the next beta is out I'll take care of it.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@dra27 could you approve this PR?

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@diml thanks - if you add me as admin on there i'll do a repo transfer to mirage/

@dra27 dra27 merged commit 8d8df4e into ocaml:trunk Feb 26, 2019

2 checks passed

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

dra27 approved these changes Feb 26, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I've cherry-picked it to 4.08 as well

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

Thanks!

@avsm

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

https://github.com/mirage/mmap now available. I'll release it to opam after setting up CI

avsm added a commit to avsm/opam-repository that referenced this pull request Feb 26, 2019

[new release] cstruct-async, ppx_cstruct, cstruct, cstruct-unix and c…
…struct-lwt (3.5.0)

CHANGES:

- Remove trailing spaces in hexdump output (mirage/ocaml-cstruct#219 @emillon)
- Add `Cstruct.rev` to allocate a reversed cstruct (mirage/ocaml-cstruct#221 @emillon)
- `Cstruct_unix` now uses the post-OCaml 4.06 `Unix.map_file`
  instead of the deprecated Bigarray `map_file` that was removed
  in OCaml 4.08 (@avsm, see ocaml/ocaml#2263)
- Remove unnecessary `(wrapped false)` in the build system (@avsm)
- Correct ocamldoc to the right `cstruct-ppx` package pointer (@avsm)
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.