search

rdicosmo / parmap

Parmap is a minimalistic library allowing to exploit multicore architecture for OCaml programs with minimal modifications.
http://rdicosmo.github.io/parmap/
Other
94 stars 20 forks source link

modernization effort #88

Closed UnixJunkie closed 5 years ago

UnixJunkie commented 5 years ago

this is compatible with OCaml 4.02.3 and 4.08.0

sorry for the many commits into a single PR. However, each commit should be quite simple and self-explanatory.

I regenerated the configure file; I can revert this if needed.

Cc: @JuliaLawall @glondu

PS: it feels _sogood to remove ocamlbuild's boilerplate...

UnixJunkie commented 5 years ago

If this is accepted, I will try to switch the build to dune in the next step.

JuliaLawall commented 5 years ago

On Mon, 19 Aug 2019, Francois Berenger wrote:

this is compatible with OCaml 4.02.3 and 4.08.0

Thanks!

julia

sorry for the many commits into a single PR. However, each commit should be quite simple and self-explanatory.

I regenerated the configure file; I can revert this if needed.

Cc: @JuliaLawall @glondu

PS: it feels so_good to remove ocamlbuild's boilerplate...

You can view, comment on, or merge this pull request online at:

  https://github.com/rdicosmo/parmap/pull/88

Commit Summary

  • rm compilation warning with 4.02.3

  • rely on the opam setcore package

  • rm bytearray things

  • rely on the opam bytearray package

  • update opam file

  • rely on the mmap opam package

  • no more need to generate config.h

  • no more need to generate config.h

  • rm trailing whitespaces

  • I don't think ocamldoc needs to look into parmap.ml

  • 4.08.0 compatiblity fix

  • I forgot to mention mmap in the META file

    File Changes

  • M META (3)

  • M Makefile.in (2)

  • M _tags (4)

  • D bytearray.ml (126)

  • D bytearray.mli (46)

  • D bytearray_stubs.c (73)

  • D config.h.in (63)

  • M configure (14)

  • M configure.ac (4)

  • M example/_tags (2)

  • D libparmap_stubs.clib (2)

  • D myocamlbuild.ml (17)

  • M opam (25)

  • M parmap.ml (18)

  • D setcore.ml (6)

  • D setcore_stubs.c (62)

  • M tests/_tags (4)

    Patch Links:

  • https://github.com/rdicosmo/parmap/pull/88.patch

  • https://github.com/rdicosmo/parmap/pull/88.diff

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute thethread.[AAD2ZGSYU4ROJETZQYEH7VDQFNTK3A5CNFSM4INNKRK2YY3PNVWWK3TUL52HS4DFUVE XG43VMWVGG33NNVSW45C7NFSM4HGELPEA.gif]

UnixJunkie commented 5 years ago

I pushed some more things, so that we don't have compilation warnings with 4.08.0 (but still compile with 4.02.3).

glondu commented 5 years ago

I am worried by all these new dependencies. Maintaining many tiny packages like those really hurts.

UnixJunkie commented 5 years ago

The new dependencies (setcore, bytearray, mmap) are all using dune. setcore and bytearray were externalized (by me) out of parmap. https://github.com/UnixJunkie/bytearray https://github.com/UnixJunkie/setcore They should be pretty low maintainance. They are now easier to maintain than when they were part of parmap (no more ocamlbuild, for example). Also, they might be useful to other programs (and setcore is already used by parany, by the way).

glondu commented 5 years ago

setcore and bytearray were externalized (by me) out of parmap

I am aware of that. That's partly why I complained.

They should be pretty low maintainance.

The maintenance cost is rather constant no matter how big the package is. Even if that cost is small, it feels more important for small packages. bytearray and setcore (and mmap, but that's not your fault) are ridiculously small. It feels less "profitable" to package them. (I'm talking with my Debian hat here.)

They are now easier to maintain than when they were part of parmap (no more ocamlbuild, for example).

This feels so wrong...

Also, they might be useful to other programs (and setcore is already used by parany, by the way).

If they are useful to other programs, maybe they should be made part of some bigger third-party library (extlib and/or extunix come to mind... maybe batteries?).

My fear is that we end up with tons of 1-line (or 1-function) packages like in the NPM world, which is not sustainable.

UnixJunkie commented 5 years ago

Concerning Debian, isn't there a tool which allows dune packages to be turned automatically into Debian packages? Because, if some people do packaging twice, it looks like the sustainable way to go.

glondu commented 5 years ago

Concerning Debian, isn't there a tool which allows dune packages to be turned automatically into Debian packages?

No, but creating the Debian package per se is not what takes most of the time and energy. It is reviewing, which cannot be automated; especially of licensing/copyright issues, which is done by people who process all Debian package additions (not just OCaml-related ones).

Because, if some people do packaging twice, it looks like the sustainable way to go.

No. I think making too many small packages is not sustainable, no matter how easy or cost-less it is to add/maintain them. Even in opam, where there is no reviewing, there is the cost of additional metadata in the repository, the additional burden on constraint solving, the additional burden put on downstream users that need to review their dependencies (and I expect any serious organization does that)...

UnixJunkie commented 5 years ago

If this PR is not accepted, this is fine with me. You can cherry pick from it what you like.

Of course, not accepting parts of this PR will make my future contributions much less likely, because this PR is meant to increase the maintainability of the software, so that working on it is more efficient for me and more enjoyable.

PS: package managers should just scale.

UnixJunkie commented 5 years ago

@glondu concerning setcore and bytearray being part of other libraries, feel free to ride that horse. This is open source, no one prevents you from doing it. Maybe setcore could interest extunix, maybe bytearray could interest the stdlib. Once there are merged in there (or anywhere else), I will happily remove those micro packages and stop relying on them.

rdicosmo commented 5 years ago

Looking at the discussion in this thread, I thing there are three different issues mixed up in a single PR:

  1. making the code compatible with recent OCaml
  2. changing the build system
  3. and extracting part of the functionality contained in Parmap's code into separate modules, introducing dependencies on external modules

The priority is to have a release of Parmap with the minimal changes needed to make it compile on recent OCaml now that the decision is taken, and push it to opam+Debian, so let's focus on this first, and leave the two other issues for further discussion/testing.

@UnixJunkie : I'll close this for the moment, may you create a new PR with the minimal changes needed for (1) ?

olafhering commented 4 years ago

@rdicosmo @UnixJunkie: Well, I would rather make 1 and 2 mandatory for a v1.0 release. configure.ac in 2019, really?

jvillard commented 4 years ago

autotools and configure.ac are still very much a standard in 2019; the OCaml compiler itself recently (and finally) migrated to it (https://github.com/ocaml/ocaml/pull/2139). I'm not sure what you are proposing to use instead @olafhering.

olafhering commented 4 years ago

Well, ocaml itself must be built with "UNIX tools" in some way, that hardly counts. Fortunately, dune itself does just depend on ocaml, which means no extra dependencies for packages that start to use it.

I think dune should be used at some point. Unfortunately I misinterpreted some comments and was under the impression that this conversion was already done with the proposed changes.

Well, I will take a look at how it needs to be done.

UnixJunkie commented 4 years ago

@olafhering a PR to switch to dune would be very welcome. I would be happy to review and test it. Note that the current PR was partly made to make this dune switching super easy.

UnixJunkie commented 4 years ago
  1. was recently made in master, accepting a hack using the configure step to generate some compatibility module. This was the included PR: https://github.com/rdicosmo/parmap/pull/83 Of course, after it was accepted, I had to fix some ocamlbuild files that also needed updating.