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

build with dune #31

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

olafhering
Copy link

also make use of stdlib-shims, but this is of course optional IMHO.

@olafhering
Copy link
Author

Not sure if an opam environment requires an additional dependency to some sort of pkg-config

@bobot
Copy link
Contributor

bobot commented May 18, 2020

conf-pkg-config indeed exists in opam. If @xavierleroy is interested in this MR, it could be improved by updating the Makefile so that the usual make, make install works. Moreover the META files in the repository can be removed. Finally if camlzip is a deprecated name (or the new one?) it could be specified in the dune file.

dune Show resolved Hide resolved
@EduardoRFS
Copy link

@olafhering this is an invalid opam package, leading your opam file to be invalid, it should be dune-configurator instead https://github.com/olafhering/ocaml-camlzip/blob/dune-master/dune-project#L21

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Olaf Hering <olaf@aepfle.de>
dune Show resolved Hide resolved
@xavierleroy
Copy link
Owner

I'm not convinced that there is any benefit to switching Camlzip to Dune. Maybe you could enlighten me? At any rate, the two ocamlfind package names "zip" and "camlzip" are here to stay.

@bobot
Copy link
Contributor

bobot commented Jun 3, 2021

Mature packages that doesn't change much have less incentive to move to dune, and adding problems because of the transition would be sad, but:

  • It reduces maintenance cost; changes like Update META file to use plugin convention #33 would not be needed since dune handles change in conventions of OCaml librairies
  • It is easier to test and develop modifications of camlzip for other projects which use dune because it is sufficient to add camlzip as a subdirectory for dune to use this version

To reduce breaking changes:

  • if moving to wrapped is wanted: (wrapped (transition <message>)) can be used to generate the unwrapped version alongside the wrapped version
  • I don't know which is the new library and the old one, in any case deprecated-library-name can be used to indicate an alias and generate the needed META file.

@EduardoRFS
Copy link

The main benefit that I see is having the wrapper, currently camlzip fails to build with zlib.

@xavierleroy
Copy link
Owner

currently camlzip fails to build with zlib.

I'm not aware of this issue. Can you elaborate?

@EduardoRFS
Copy link

EduardoRFS commented Jun 4, 2021

@xavierleroy module name conflict, camlzip has a module called zlib, zlib is a library called zlib, so it will fail at link time because camlzip is not wrapped. Of course making it wrapped is also a breaking change on the API but we can have both for now, wrapped and non wrapped.

http://opam.ocaml.org/packages/zlib

@NathanReb
Copy link

I recently worked on an update of this port for opam-monorepo. I believe it addresses some of the original issues that were mentioned here. I'd be happy to submit a PR if you are interested.

The port is available here: https://github.com/dune-universe/camlzip/tree/dune-universe-v1.11.
The strategy is described in the first section of the README there.

@bobot
Copy link
Contributor

bobot commented Mar 11, 2022

For the deprecated names, neither deprecated-library-names and deprecated_package_names were satisfying?

@NathanReb
Copy link

I didn't use these features because the dune-universe ports aim is to strictly stick to build system changes and remain as close as possible to the upstream version. I assumed they would trigger warnings which is what I wanted to avoid. If I assumed wrong, I'd be happy to use them instead!

It also seems that @xavierleroy has no intention to deprecate either library names.

If the maintainers wish to switch to dune, I'm more than happy to open a PR and adapt it however they see fit, including deprecating one of the lib name or wrapping the library to avoid modules clashes!

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.

None yet

8 participants