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

dune: 1.9.2 -> 1.10.0 #62256

Merged
merged 4 commits into from Jul 5, 2019
Merged

dune: 1.9.2 -> 1.10.0 #62256

merged 4 commits into from Jul 5, 2019

Conversation

marsam
Copy link
Contributor

@marsam marsam commented May 30, 2019

Motivation for this change

Changelog: https://github.com/ocaml/dune/releases/tag/1.10.0

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Mic92
Copy link
Member

Mic92 commented Jun 3, 2019

cc @Zimmi48 for review

@vbgl
Copy link
Contributor

vbgl commented Jun 4, 2019

@GrahamcOfBorg build virt-top

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

virt-top does not build because of a broken dependency: camomile

Copy link
Contributor

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

This breaks virt-top.

@Zimmi48

This comment has been minimized.

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

This seems to be related to this changelog entries:

Improve the behavior when a strict subset of the targets of a rule is already in the source tree for projects using the dune language < 1.10

With lang dune >= 1.10, rules in standard mode are no longer allowed to produce targets that are present in the source tree. This has been a warning for long enough

FTR camomile is not the only package broken by this change: for instance, sqlexpr is as well.

@Mic92
Copy link
Member

Mic92 commented Jun 4, 2019

One way to move this forward is to introduce a dune = dune_1_10; and a dune_1_9 and then use the old dune for packages that require it.

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

FTR these are the packages that are broken by this update:

  • ocamlPackages.camomile
  • ocamlPackages.sqlexpr
  • ocamlPackages.lwt_camlp4

Depends on ocamlPackages.camomile:

  • liquidsoap
  • ocamlPackages.zed
  • ocamlPackages.ocaml_gettext
  • ocamlPackages.javalib
  • libguestfs
  • libguestfs-with-appliance
  • ocamlPackages.lambdaTerm
  • ocamlPackages.sawja
  • virt-top
  • python37Packages.guestfs
  • python27Packages.guestfs
  • ocamlPackages.utop
  • vagrant
  • ocamlPackages.bap
  • ocamlPackages.reason
  • libbap
  • python27Packages.bap
  • python37Packages.bap

Depends on ocamlPackages.sqlexpr:

  • ocamlPackages.ppx_sqlexpr

Depends on ocamlPackages.lwt_camlp4:

  • ocamlPackages.eliom
  • ocamlPackages.ocsigen-start
  • ocamlPackages.ocsigen-toolkit

and the following patch is enough to have lwt_camlp4 and its dependencies build:

diff --git a/pkgs/development/ocaml-modules/lwt/camlp4.nix b/pkgs/development/ocaml-modules/lwt/camlp4.nix
index 53f0435f462..5367ef3f9f2 100644
--- a/pkgs/development/ocaml-modules/lwt/camlp4.nix
+++ b/pkgs/development/ocaml-modules/lwt/camlp4.nix
@@ -15,6 +15,8 @@ buildDunePackage rec {
 
   propagatedBuildInputs = [ camlp4 ];
 
+  preBuild = "rm META.lwt_camlp4";
+
   meta = {
     description = "Camlp4 syntax extension for Lwt (deprecated)";
     license = lib.licenses.lgpl21;
@@ -22,4 +24,3 @@ buildDunePackage rec {
     maintainers = [ lib.maintainers.vbgl ];
   };
 }

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

The following patch fixes the build for camomile and all the packages that depend on it (including virt-top):

diff --git a/pkgs/development/ocaml-modules/camomile/default.nix b/pkgs/development/ocaml-modules/camomile/default.nix
index f21e7643afe..815acf78914 100644
--- a/pkgs/development/ocaml-modules/camomile/default.nix
+++ b/pkgs/development/ocaml-modules/camomile/default.nix
@@ -1,14 +1,14 @@
-{ stdenv, fetchFromGitHub, buildDunePackage, cppo }:
+{ stdenv, fetchurl, fetchFromGitHub, buildDunePackage, cppo }:
 
 buildDunePackage rec {
   pname = "camomile";
-       version = "1.0.1";
+       version = "git-pr-76";
 
        src = fetchFromGitHub {
                owner = "yoriyuki";
                repo = pname;
-               rev = version;
-               sha256 = "1pfxr9kzkpd5bsdqrpxasfxkawwkg4cpx3m1h6203sxi7qv1z3fn";
+               rev = "59e350d7988b90b068d4075512d1ad6002fdea4c";
+               sha256 = "1zlrf2ij70x83rncbsbjw0f4l4sxjylkhhx6ayg2n57zqwhmb999";
        };
 
        buildInputs = [ cppo ];

This is basically updating to the version of camomile of yoriyuki/Camomile#76.

@vbgl
Copy link
Contributor

vbgl commented Jun 4, 2019

At this point you should ask upstream (camomile) to release a new version.

@Mic92
Copy link
Member

Mic92 commented Jun 4, 2019

They have not even merged the patch. fetchpatch might be also acceptable in the meantime.

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

Sure. I intended to do so as soon as upstream merges the PR.

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

@Mic92 I first tried a solution with fetchpatch but it doesn't work because it is a patch for an unreleased version which has moved from jbuild to dune, which is not the case of the last released version.

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 4, 2019

The last issue is with sqlexpr whose compilation fail like this:

  Multiple rules generated for _build/default/META.sqlexpr:
  - file present in source tree
  - <internal location>
  Hint: rm -f META.sqlexpr

but removing the META file like I did for lwt_camlp4 breaks ppx_sqlexpr:

  File "tests/ppx/jbuild", line 12, characters 17-24:
  12 |   (libraries    (sqlexpr oUnit))
                        ^^^^^^^
  Error: Library "sqlexpr" not found.

Zimmi48 referenced this pull request in yoriyuki/Camomile Jun 4, 2019
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@Zimmi48
Copy link
Member

Zimmi48 commented Jun 5, 2019

From talking to @diml on yoriyuki/Camomile@505202b, it would seem that it could be a Dune bug that we have three packages using jbuilder that do not build with the new version of Dune:

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 5, 2019

According to @diml, another fix for this is just to use the jbuilder binary instead of the dune binary for projects that still use jbuild files.

@marsam
Copy link
Contributor Author

marsam commented Jun 14, 2019

I've replaced jbuilder in camomile, and removed the META.* files in sqlexpr and lwt_camlp4 which seems to be enough

@GrahamcOfBorg build ocamlPackages.camomile ocamlPackages.sqlexpr ocamlPackages.lwt_camlp4 virt-top

@Zimmi48
Copy link
Member

Zimmi48 commented Jun 14, 2019

@marsam Have you tried building ppx_sqlexpr after removing the META file in sqlexpr?

@marsam
Copy link
Contributor Author

marsam commented Jun 14, 2019

@ofborg ofborg bot requested a review from vbgl June 14, 2019 15:22
@Zimmi48
Copy link
Member

Zimmi48 commented Jul 5, 2019

@vbgl Isn't this PR ready to be merged?

@vbgl vbgl merged commit 88c2586 into NixOS:master Jul 5, 2019
@Mic92
Copy link
Member

Mic92 commented Aug 1, 2019

Camomile got a fix merged now: yoriyuki/Camomile#76

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.

None yet

4 participants