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

Janestreet #18572

Merged
merged 45 commits into from
Sep 23, 2016
Merged

Janestreet #18572

merged 45 commits into from
Sep 23, 2016

Conversation

maurer
Copy link
Member

@maurer maurer commented Sep 14, 2016

Motivation for this change

OCaml has switched its primary macro system in OCaml 4.02 to PPX (from camlp4).
Jane Street, who produces one of the OCaml basis libraries, took this opportunity to change their build system, rip out all of their camlp4 stuff, and replace it with PPX.
New libraries and programs which depend on the new PPX style macros are being released, so we need a way to support them.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

To deal with older compilers and packages, I did a few things in addition to packaging the new code:

  • Each package which has transitioned to PPX has been renamed to packagename_p4, allowing its last camlp4 release to be referenced explicitly
  • Each package which transitioned to PPX had an if statement added which will diversion to packagename_p4 if the compiler is too old, so OCaml 4.0.0, 3.1, etc will still be able to access e.g. "sexplib" rather than "sexplib_p4"
  • Existing packages which referenced a transitioning package were pointed at the _p4 version.

@mention-bot
Copy link

@maurer, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ts468 and @ericbmerritt to be potential reviewers

@maurer
Copy link
Member Author

maurer commented Sep 14, 2016

@vbgl Tagging you as another potential reviewer, feel free to ignore if you're not interested.

@maurer maurer mentioned this pull request Sep 14, 2016
7 tasks
propagatedBuildInputs = [ ppx_core ppx_tools ppx_type_conv sexplib];

meta = with stdenv.lib; {
description = "A ppx rewriter that defines an extension node whose value is its source position";
Copy link
Member

Choose a reason for hiding this comment

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

This description doesn't seem to correspond to the ppx

@thufschmitt
Copy link
Member

Wow, that's a huge work !

I added a few minor annotations, otherwise it's a big 👍

@aske
Copy link
Member

aske commented Sep 14, 2016

Right now I'm working on moving ocaml packages from all-packages.nix to make it look more like current haskell system; would be nice to see this PR merged first so I can deal with merge conflicts sooner ( the opposite way is fine too :) ).

@joachifm
Copy link
Contributor

Is the darwin failure reported by travis legit and if so is it a regression?

@vbgl
Copy link
Contributor

vbgl commented Sep 16, 2016

I can reproduce the error on darwin: ppx_core won’t build with the following message.

+ xargs stat -f '%d:%i:%m' < .compiler-libs.common.files > .compiler-libs.common.stats
stat: cannot read file system information for '%d:%i:%m': No such file or directory

I have no idea where that comes from.

@maurer
Copy link
Member Author

maurer commented Sep 16, 2016

I don't have easy access to a darwin machine, but ppx_core as a piece of software does work on OSX outside of this packagaing.

@joachifm It's not a regression since ppx_core is not in nixpkgs yet, but should probably be fixed before merging since I know the packages I'm adding/updating work on OSX when installed via OPAM.

@maurer
Copy link
Member Author

maurer commented Sep 16, 2016

https://github.com/janestreet/js-build-tools/blob/7ff35bb6451f241bb1a643f73f73d9d55fd36d94/ocamlbuild_goodies/jane_street_ocamlbuild_goodies.ml#L66-L74

It looks like they are expecting some BSD-like version of stat for which -f means something else, that is normally present on OSX. With Darwin + nix, it may be getting a more Linuxish stat.

I can't test on Darwin easily, but does someone who can want to test removing | "Darwin" from that top match and see if it fixes things? @vbgl

@vbgl
Copy link
Contributor

vbgl commented Sep 17, 2016

Good catch. The following patch fixes the build.

--- a/ocamlbuild_goodies/jane_street_ocamlbuild_goodies.ml
+++ b/ocamlbuild_goodies/jane_street_ocamlbuild_goodies.ml
@@ -65,7 +65,7 @@ let track_external_deps = function

     let stat, md5sum =
       match run_and_read "uname" |> String.trim with
-      | "Darwin" ->
+      | "FreeBSD" | "NetBSD" ->
         (S [A "stat"; A "-f"; A "%d:%i:%m"],
          A "md5")
       | _ ->

@aske aske mentioned this pull request Sep 19, 2016
7 tasks
On darwin, js-build-tools expects an OSX style userland and stat binary.
However, under nix we use the nix version of stat, which speaks linux-style
flags.

This patch removes js-build-tools special casing so that it speaks to stat
in linux style.
@maurer maurer mentioned this pull request Sep 21, 2016
@maurer
Copy link
Member Author

maurer commented Sep 22, 2016

I've applied the patch to fix the darwin build. Anything else to be done?

@joachifm joachifm merged commit aa92278 into NixOS:master Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants