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

remove Str module #663

Closed
hannesm opened this Issue Nov 10, 2016 · 7 comments

Comments

Projects
None yet
5 participants
@hannesm
Member

hannesm commented Nov 10, 2016

The OCaml distribution provides, as part of otherlibs, a module called Str which has some regular expression implementation. There are parts of this implementation written in C, and it seems to be kind of out of fashion (a very imperative implementation).

Luckily there are alternatives, on the one side astring, and if you need full regular expressions there is the fine re library written purely in OCaml.

The mirage.runtime ocamlfind package, which is part of every MirageOS unikernel, embeds next to logs also astring. For simple string processing (such as split), there is no need to use the big regular expression hammer anymore.

I removed the strstubs from being part of the OCaml runtime shipped for MirageOS unikernels. In case your unikernel uses Str, it will be a link-time error -- this is how Travis found the occurence of Str in the dhcp example. Please consider the following PRs:

Have an informational look into mirage/mirage-dev#202 to convince yourself that Travis managed to compile mirage-skeleton with the new OCaml runtimes.

This is a breaking change and will be properly documented with the MirageOS3 release notes.

@yomimono

This comment has been minimized.

Member

yomimono commented Nov 11, 2016

I don't know how widely Str is used or how expected its presence is. I'm in favor of not encouraging people to use libraries that are of poor quality (which is why I merged mirage/mirage-skeleton#195 ), but I also can't easily run opam list --depends-on str to find out how many packages we make it difficult to use in Mirage unikernels with this change.

@dbuenzli

This comment has been minimized.

Contributor

dbuenzli commented Nov 11, 2016

but I also can't easily run opam list --depends-on str

If you do an opam pin odig --dev-repo this will install a tool call metagen, a call to metagen --raw will output on stdout all the $(opam config var lib)/$pkg/META files odig found in the switch (it won't report META.$pkg ones though; I don't know if mirage packages do that kind of thing).

Something like metagen --raw | grep '[" ]str[" ]' should give a good idea of the usage on the installed base.

@samoht

This comment has been minimized.

Member

samoht commented Nov 11, 2016

Deprecating the use of Str in Mirage libraries is certainly a good idea.

@yomimono

This comment has been minimized.

Member

yomimono commented Nov 11, 2016

thanks @dbuenzli ! While opam attempts to make the installed universe as large as possible, I'll note that just grepping around in repositories for packages tagged with org:mirage yielded a few users, including ctypes .

@yallop

This comment has been minimized.

Member

yallop commented Nov 11, 2016

I think ctypes only uses Str during configuration and code generation, not at run time, although that distinction isn't currently made in the META file.

@yomimono

This comment has been minimized.

Member

yomimono commented Nov 11, 2016

Thanks @yallop; that's the case for nearly all other Str users too AFAICT, but I wasn't familiar enough with ctypes to see that straight away.

@hannesm

This comment has been minimized.

Member

hannesm commented Nov 12, 2016

after 24 hours without anyone against it, I merged it.

some libraries use Str in their build system atm, there is also imaplet-lwt and an example in tls.lwt which use Str. Since these are not MirageOS unikernels, they can be fixed independently without further breakage.

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