Skip to content

Remove Stream, Genlex, Pervasives & the legacy bigarray library#10896

Merged
nojb merged 6 commits intoocaml:trunkfrom
nojb:remove_deprecated_cont
Jan 20, 2022
Merged

Remove Stream, Genlex, Pervasives & the legacy bigarray library#10896
nojb merged 6 commits intoocaml:trunkfrom
nojb:remove_deprecated_cont

Conversation

@nojb
Copy link
Copy Markdown
Contributor

@nojb nojb commented Jan 13, 2022

What it says on the tin. Note that Stream and Genlex are available as an external library https://github.com/ocaml/camlp-streams.

Comment on lines -276 to -278
AC_ARG_ENABLE([bigarray-lib],
[AS_HELP_STRING([--disable-bigarray-lib],
[do not build the legacy separate bigarray library])])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe you might be asked to keep the option around and make it error gracefully instead.
e.g. #10893 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you 🙂

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

@nojb nojb force-pushed the remove_deprecated_cont branch 2 times, most recently from 7f3a53d to 08c2111 Compare January 14, 2022 05:31
Copy link
Copy Markdown
Member

@avsm avsm left a comment

Choose a reason for hiding this comment

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

Looks good to me, aside from a couple of minor suggestions.

Changes Outdated
(Nicolás Ojeda Bär, review by Damien Doligez)

* #10896: Remove Stream, Genlex and Pervasives. Also remove legacy bigarray
library.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it worth mentioning here that the Bigarray module is now included in the distribution, and that this only refers to the bigarray ocamlfind library? It's a little confusing potentially since the previous sentence refers to modules.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, reworded for clarity.

configure.ac Outdated
[AS_HELP_STRING([--disable-bigarray-lib],
[do not build the legacy separate bigarray library])])
AC_ARG_ENABLE([bigarray-lib], [],
[AC_MSG_ERROR([The bigarray-lib option was deleted in OCaml 5.00.])],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
[AC_MSG_ERROR([The bigarray-lib option was deleted in OCaml 5.00.])],
[AC_MSG_ERROR([The bigarray-lib option was deleted in OCaml 5.00, as it is now part of the standard library])],

(to make OS porters live's easier)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, reworded to clarify the sentence.

@dra27
Copy link
Copy Markdown
Member

dra27 commented Jan 14, 2022

A note to check (just as I've been looking at base packages elsewhere) that we need to determine if OCaml 5.00 should be installing the base-bigarray opam package or not (I'm not certain without checking what the precise expectation of packages which depend on that is in opam-repository).

@nojb nojb force-pushed the remove_deprecated_cont branch from 7694e27 to e3306f3 Compare January 20, 2022 13:55
@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Jan 20, 2022

This is ready from my side, and already has one official approval, so am planning to merge this soon (either today or tomorrow) to have a maximum amount of time to investigate breakage in the ecosystem.

@nojb nojb merged commit f4b6dce into ocaml:trunk Jan 20, 2022
@nojb nojb deleted the remove_deprecated_cont branch January 20, 2022 17:02
@dbuenzli
Copy link
Copy Markdown
Contributor

@nojb I suspect this fixes #10833 ?

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Jan 20, 2022

@nojb I suspect this fixes #10833 ?

Yes!

@c-cube
Copy link
Copy Markdown
Contributor

c-cube commented Jan 20, 2022

RIP Stream, you won't be missed 🤪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants