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

ocamlPackages.base_0_12: Add dune_1 to build inputs for dune.configurator #162636

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

ulrikstrid
Copy link
Member

Motivation for this change

ppx_let was broken for OCaml 4.07 by #161344.
This change fixes the breakage by adding a force override to build with dune 2, This is useful when it's used in janestreet libs 0.12 where it's problematic.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ulrikstrid ulrikstrid requested a review from vbgl March 3, 2022 14:20
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 3, 2022
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 looks really wrong.

Here is a suspicious change in the other PR:
a13cdfe#diff-d4cd7e64992766eaf78d6937055c74118c78eb50eed2c992e948f5c4f05a0544R10

@ulrikstrid
Copy link
Member Author

Now I remember, the reason I added useDune2 = true there was because base started failing with this:

File "src/discover/dune", line 1, characters 41-58:
1 | (executables (names discover) (libraries dune.configurator)
                                             ^^^^^^^^^^^^^^^^^
Error: Library "dune.configurator" not found.
Hint: try: dune external-lib-deps --missing -p base -j 16 @@default
                          
error: builder for '/nix/store/gssfvk9hbms2l6fff2sv9y0721l0lwqc-ocaml4.07.1-base-0.12.2.drv' failed with exit code 1;
       last 10 log lines:
       > patching sources
       > configuring
       > no configure script, doing nothing
       > building
       > File "src/discover/dune", line 1, characters 41-58:
       > 1 | (executables (names discover) (libraries dune.configurator)
       >                                              ^^^^^^^^^^^^^^^^^
       > Error: Library "dune.configurator" not found.
       > Hint: try: dune external-lib-deps --missing -p base -j 16 @@default
       >
       For full logs, run 'nix log /nix/store/gssfvk9hbms2l6fff2sv9y0721l0lwqc-ocaml4.07.1-base-0.12.2.drv'.

@ulrikstrid
Copy link
Member Author

Okay, so the underlying issue is that no library can find dune.configurator while building on older OCaml versions (at least for 4.07). I'm not sure how to debug and/or fix this, any ideas? @vbgl @symphorien

@ulrikstrid
Copy link
Member Author

ulrikstrid commented Mar 3, 2022

To find this I ran the following:
find ./pkgs/development/ocaml-modules -maxdepth 1 -type d \( ! -name ocaml-modules \) -exec bash -c "./build.sh '{}'" \;
with build.sh as follows:

PACKAGE=$(basename "$1")

echo "$1"
echo "$PACKAGE"

{
echo "4.07 - $PACKAGE";
nix-shell -I nixpkgs=./default.nix \
    -p ocaml-ng.ocamlPackages_4_07.findlib \
    "ocaml-ng.ocamlPackages_4_07.$PACKAGE" \
    --run "echo \"query\" && ocamlfind query -r $PACKAGE";

echo "4.12 - $PACKAGE";
nix-shell -I nixpkgs=./default.nix \
    -p ocaml-ng.ocamlPackages_4_12.findlib \
    "ocaml-ng.ocamlPackages_4_12.$PACKAGE" \
    --run "echo \"query\" && ocamlfind query -r $PACKAGE";
} &> "./ocaml-logs/$PACKAGE.result.log"

There are probably smarter ways of doing this, but this is what I did at least.
Then you can grep for failures with grep -rnw './ocaml-logs' -e 'configurator' and grep -rnw './ocaml-logs' -e 'ocamlfind: Package'

@vbgl
Copy link
Contributor

vbgl commented Mar 4, 2022

dune.configuratoris provided by dune_1, not by dune-configurator.

@vbgl
Copy link
Contributor

vbgl commented Mar 4, 2022

Here is a possible patch (to replace this PR):

diff --git a/pkgs/development/ocaml-modules/janestreet/0.12.nix b/pkgs/development/ocaml-modules/janestreet/0.12.nix
index ec2f793caf9..c3b4b0c6d70 100644
--- a/pkgs/development/ocaml-modules/janestreet/0.12.nix
+++ b/pkgs/development/ocaml-modules/janestreet/0.12.nix
@@ -24,7 +24,7 @@ with self;
     hash = "0gl89zpgsf3n30nb6v5cns27g2bfg4rf3s2427gqvwbkr5gcf7ri";
     meta.description = "Full standard library replacement for OCaml";
     propagatedBuildInputs = [ sexplib0 ];
-    buildInputs = [ dune-configurator ];
+    buildInputs = [ dune_1 ];
   };
 
   stdio = janePackage {
@@ -208,7 +208,7 @@ with self;
     pname = "jst-config";
     hash = "0yxcz13vda1mdh9ah7qqxwfxpcqang5sgdssd8721rszbwqqaw93";
     meta.description = "Compile-time configuration for Jane Street libraries";
-    buildInputs = [ ppx_assert ];
+    buildInputs = [ dune_1 ppx_assert ];
   };
 
   ppx_optcomp = janePackage {
diff --git a/pkgs/development/ocaml-modules/janestreet/janePackage_0_12.nix b/pkgs/development/ocaml-modules/janestreet/janePackage_0_12.nix
index ebde7b240ef..4601341ed64 100644
--- a/pkgs/development/ocaml-modules/janestreet/janePackage_0_12.nix
+++ b/pkgs/development/ocaml-modules/janestreet/janePackage_0_12.nix
@@ -1,13 +1,13 @@
 { lib, fetchFromGitHub, buildDunePackage, defaultVersion ? "0.12.0" }:
 
-{ pname, version ? defaultVersion, hash, buildInputs ? [], ...}@args:
+{ pname, version ? defaultVersion, hash, ...}@args:
 
 buildDunePackage (args // {
-  inherit version buildInputs;
+  inherit version;
 
-  minimumOCamlVersion = "4.07";
+  minimalOCamlVersion = "4.07";
 
-  useDune2 = true;
+  useDune2 = false;
 
   src = fetchFromGitHub {
     owner = "janestreet";

@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ppx_let branch from fd7a278 to f463e53 Compare March 4, 2022 07:14
@ulrikstrid ulrikstrid changed the title ocamlPackages.ppx_let: ppxlib needs dune2 with strictDeps ocamlPackages.base_0_12: Add dune_1 to build inputs for dune.configurator Mar 4, 2022
@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ppx_let branch 2 times, most recently from eab18d4 to f9c984e Compare March 4, 2022 07:25
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Mar 4, 2022
@ulrikstrid
Copy link
Member Author

I came to about the some conclusion as your patch, I tried adding you as a co-author to the commit but I think it's basically your diff now, it fixes all the packages I could find that complained about dune.configurator in my tests.

@vbgl
Copy link
Contributor

vbgl commented Mar 4, 2022

While you are at it, you can fix the spelling of minimalOCamlVersion.

@ulrikstrid ulrikstrid force-pushed the ulrikstrid/fix-ppx_let branch from f9c984e to 9aba040 Compare March 4, 2022 08:15
@ulrikstrid
Copy link
Member Author

Done

@vbgl vbgl self-requested a review March 4, 2022 09:01
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.

Thanks.

@vbgl vbgl merged commit ffa377b into NixOS:master Mar 4, 2022
@ulrikstrid ulrikstrid deleted the ulrikstrid/fix-ppx_let branch March 4, 2022 10:00
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.

2 participants