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

Add miscellaneous OCaml Packages #8291

Merged
merged 5 commits into from
Jun 27, 2015
Merged

Add miscellaneous OCaml Packages #8291

merged 5 commits into from
Jun 27, 2015

Conversation

maurer
Copy link
Member

@maurer maurer commented Jun 11, 2015

Miscellaneous OCaml packages, plus updates to some existing ones so they will build against ocaml 4.02.1

This was referenced Jun 11, 2015
@vbgl
Copy link
Contributor

vbgl commented Jun 11, 2015

Thanks. Here are some remarks.

You may want to add yourself as maintainer of the packages that you introduce.

Bitstring-2.0.4 is broken in OCaml-4.02 (for instance, run the test suite and see test_18 fail). See e.g., https://code.google.com/p/bitstring/source/detail?r=5997a66d9b078d75f821eedc9ba615c9df321e98 for discussion.

I don’t see the point in fixing individual packages to make them compatible with the broken ocamlbuild (which doesn’t know where camlp4 is). Let’s fix ocamlbuild first.

Please make sure that all your packages (e.g., piqi) have a version number as part of their names.

@maurer
Copy link
Member Author

maurer commented Jun 11, 2015

@vbgl:
I've added version numbers (oops) and myself as maintainer.

In the case of bitstring, would you reccomend that I make a package pointing at a git snapshot then? My end goal is to build a package which depends both on cohttp and bitstring, and the cohttp derivation currently has a minimum OCaml version of 4.02.

As far as fixing ocamlbuild goes, the OCaml maintainers think this is hard and have punted on it until sometime after the release of 4.02.2. Their suggested solution, putting camlp4 into the compiler directory, is not really nix-compatible, and would require me to modify deeper/more important packages (e.g. the ocaml package). I agree that it would be nice to have a fixed ocamlbuild, but that seems to leave the realm of packaging and go into working on OCaml's tooling.

@maurer
Copy link
Member Author

maurer commented Jun 11, 2015

@vbgl I went ahead and bumped bitstring to latest git. Let me know if you have another strategy for dealing with optcomp - I agree what I did is ugly, but doing it the right way would involve getting a consensus from the OCaml community first on what is for them a low priority issue (since basically only nix users have this issue).

@vbgl
Copy link
Contributor

vbgl commented Jun 11, 2015

I agree that it would be nice to have a fixed ocamlbuild, but that seems to leave the realm of packaging and go into working on OCaml's tooling.

My opinion is that the patch for optcomp does not really belong to the “realm of packaging”. I’ve just proposed a solution in PR #8296.

@maurer
Copy link
Member Author

maurer commented Jun 11, 2015

OK. I didn't know such a patch was already written - the bug seemed to make me think that there was something tricky about fixing ocamlbuild I was missing. I'll wait for #8296 to go in and then edit these commits to be in line with it. (Looks like I should just need to remove the ulex fix and revisit optcomp and see if it works once your ocamlbuild patch is in.)

@vbgl
Copy link
Contributor

vbgl commented Jun 19, 2015

@maurer, would you like to test the camlp4/ocamlbuild fix? If you confirm that it works, I’ll merge the corresponding PR and this one. Thanks for your contribution.

@maurer
Copy link
Member Author

maurer commented Jun 19, 2015

I rebased my change on top of yours, removing the optcomp fix (since your ocamlbuild fix deals with it) and the ulex fix since you did that one too. Everything seems to build/run fine. (As checked by building #8293 against the new misc-ocaml).

@maurer
Copy link
Member Author

maurer commented Jun 19, 2015

Fixed the typo. I'm kind of surprised it built like that.

{stdenv, buildOcaml, fetchurl, herelib, camlp4}:

buildOcaml rec {
minimumSupportedOcamlVersion = "4.02";
Copy link
Contributor

Choose a reason for hiding this comment

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

You can relax this constraint to 4.00.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I was following the pattern I'd seen in the repo previously (plus, it had deps on 4.02-only herelib, so I couldn't test it with earlier than 4.02)

@maurer
Copy link
Member Author

maurer commented Jun 22, 2015

@vbgl: Anything else I need to do to get this merged?


buildInputs = [ocaml findlib];

configurePhase = "ocaml setup.ml -configure --destdir $prefix";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be --prefix rather than --destdir: look at where uuidtrip gets installed.

@vbgl
Copy link
Contributor

vbgl commented Jun 23, 2015

It would be nice to:

  1. fix the installation of uuidm;
  2. clean the description strings of the meta attribute of your packages;
  3. add doCheck = true to the packages that come with a test-suite;
  4. clean the git history.

Thanks a lot.

@maurer
Copy link
Member Author

maurer commented Jun 25, 2015

@vgbl I think I addressed all your concerns.
The bitstring 2.0.4 I've got in there passes tests when called from ocamlPackages_4_01 and fails with 4.02, so it seems like turning on tests in the first place would have alerted me to the bug you found.

By the way, are you doing all this checking by hand, or is there a set of linter scripts somewhere to help find these things (e.g. the misinstalled binary, the the typo'd maintainer field)

@vbgl
Copy link
Contributor

vbgl commented Jun 25, 2015

Nice.

The typo has been found by Travis CI; otherwise, it’s all manual review.

Is the file pkgs/development/ocaml-modules/bitstring/2.0.4.nix referenced somewhere? otherwise it’s just dead code.

@maurer
Copy link
Member Author

maurer commented Jun 25, 2015

It is not directly referenced, but I felt bad adding the git version of a package without the latest stable, even if stable is not the default.
I can remove it if you'd like.

@vbgl
Copy link
Contributor

vbgl commented Jun 25, 2015

If you want to keep the two versions of the package, with different defaults for different versions of OCaml, you can look at what is done for camlimages or sexplib.

@maurer
Copy link
Member Author

maurer commented Jun 25, 2015

Thanks, I've done just that (it now uses bitstring 2.0.4 for ocaml earlier than 4.02, and the git commit I selected for 4.02 and higher).

@vbgl
Copy link
Contributor

vbgl commented Jun 27, 2015

I know have the following error when fetching the source of bitstring_git:

fatal: missing blob object '41833a9ba6b4b3ded32d97c0a5a8934e003a44f4'
error: https://code.google.com/p/bitstring/ did not send all necessary objects

fatal: Server does not support shallow clients
fatal: The remote end hung up unexpectedly
Unable to checkout f1673f86fd358706beb7b5ce357eb4e0ff1d970a from https://code.google.com/p/bitstring/.

I don’t know what this means.

@maurer
Copy link
Member Author

maurer commented Jun 27, 2015

Looks like fetchgit started doing non deep clones by default, and google code does not support this. I already had it in my nix-store, so I didn't notice this. I think the change I made should fix it though.

@vbgl
Copy link
Contributor

vbgl commented Jun 27, 2015

Looks like the hash is wrong now. When you test, you can remove the sources that are already in the store using something like

nix-store --delete /nix/store/yl113xcfsy3bm73fz5cisishhyy6rxha-bitstring-f1673f8

I don’t really understand what’s going on, but the following works for me:

  src = fetchgit {
    url = "https://code.google.com/p/bitstring/";
    rev = "f1673f86fd35";
    sha256 = "1lh97qf1b7mq64pxkphr2w91ri5hfwg58cpjb2xd8a453c9jylw4";
  };

Same hash as before, the rev is a little bit shorter.

@maurer
Copy link
Member Author

maurer commented Jun 27, 2015

Reading the nix-prefetch-git script, it appears that what is going on here is:
1.) If you specify an incomplete revision, it has to clone the repo normally first, no --depth trickery, so the gcode bug isn't hit.
2.) If you do a deep clone of the repo, it assumes you wants the history, and so doesn't gc it, resulting in a changed hash.

I changed it to your version and have pushed. Hopefully there aren't too many hosts that don't support shallow cloning, or we may need to talk to the prefetch-git people about yet another option (e.g. deep clone, but still gc the repo before hashing).

vbgl added a commit that referenced this pull request Jun 27, 2015
Add miscellaneous OCaml Packages
@vbgl vbgl merged commit 742824b into NixOS:master Jun 27, 2015
@vbgl
Copy link
Contributor

vbgl commented Jun 27, 2015

Hurrah ! let’s move on.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9c0f1ee on maurer:misc-ocaml into ** on NixOS:master**.

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.

4 participants