ocamlPackages tree-wide: Move buildInputs that should be nativeBuildInputs #161344

ulrikstrid · 2022-02-22T10:04:03Z

Motivation for this change

To make cross compilation better we need to move a bunch of stuff to nativeBuildInputs.
Another good reason is that we should get much smaller closures when building ocaml projects.
This PR supersedes #145448 I think.

Things done

symphorien

Thanks for the PR. How did you test it?

It would be nice to add strictDeps = true on every derivation which you fix. This allows to test if the partition between buildInputs and nativeBuildInputs is correct without cross compiling. This is the one true way to know if ppx libs belong to buildInputs or to nativeBuildInputs. In case strictDeps do not work at all in problematic cases (everything involving gtk3 is problematic for example), you can leave it to false, of course. But such a PR would bitrot very fast if we cannot set strictDeps to true on a large proportion of them.

symphorien · 2022-02-22T17:49:10Z

pkgs/development/ocaml-modules/bolt/default.nix

@@ -18,7 +18,8 @@ stdenv.mkDerivation rec {
    sha256 = "1c807wrpxra9sbb34lajhimwra28ldxv04m570567lh2b04n38zy";
  };

-  buildInputs = [ ocaml findlib ocamlbuild which camlp4 ];
+  nativeBuildInputs = [ ocaml findlib ocamlbuild which ];
+  buildInputs = [ camlp4 ];


I suspect this belongs to nativeBuildInput, as it's a preprocessor.

Me and @anmonteiro discussed this and we believe camlp4 will be built and ran while building like ppxes. I would love to be proved wrong and be able to move more things to nativeBuildInputs

Now that strictDeps is set, it only builds with camlp4 in nativeBuildInputs. That answers the question :)

you didn't fix this

ulrikstrid · 2022-02-22T19:06:44Z

I've been running nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD" and building LIGO statically in a separate project using this patch. It's not fully getting everything but I found a bunch of stuff that I fixed and amended.
I also did a patch to set strictDeps in buildDunePackage just to test it out, but I could add it to all the packages that I've touched in this PR.

symphorien · 2022-02-22T19:22:35Z

adding strictDeps = true to buildDunePackage is a good idea if there are not too many exceptions.
Yes it would be better to set it to all packages touched by this PR, otherwise there is no way to check that what you changed is correct, and it would bitrot rapidly.

ulrikstrid · 2022-02-22T19:59:56Z

@symphorien I have now added strictDeps = true; to all the packages I dared setting it for (skipped the SDL and gnome packages)

symphorien

nix-review is happy
I'll merge over the weekend unless someone complains.

ulrikstrid · 2022-02-26T10:04:03Z

@symphorien can I ask you to open a issue about the checkInputs failure we're seeing? And also add your guess about how to solve it?

If you do I could spend some time trying to solve it never week. 🙏

symphorien · 2022-02-26T11:45:29Z

Issue is #161570 and PR is #161575

* wip * check if still builds * remove file * update melange

vbgl · 2022-03-01T12:17:15Z

This broke ocaml-ng.ocamlPackages_4_07.lwt.

symphorien · 2022-03-01T14:05:02Z

fixed in #162370

vbgl · 2022-03-01T16:11:04Z

Thanks for the quick fix.
Bisecting the failure of ocaml-ng.ocamlPackages_4_07.ppx_let led me here again.

vbgl · 2022-03-01T16:20:21Z

This also broke ocaml-ng.ocamlPackages_4_07.ppx_deriving.

symphorien · 2022-03-01T21:39:51Z

I don't know what makes ppx_let fail, can you have a look @ulrikstrid ?

ulrikstrid · 2022-03-02T07:37:26Z

Trying to reproduce the error on with ocaml-ng.ocamlPackages_4_07.ppx_let but all I'm seeing is that ppxlib needs dune 2 for version 0.8.1. Is this what you are seeing as well @vbgl @symphorien ?

Also, I asked in the followup PR if any of you know a good command to build all/most packages across ocaml versions so that I can catch these earlier.

vbgl · 2022-03-03T10:37:10Z

This broke apron: it builds fine but is not installed properly:

nix-shell -p ocaml-ng.ocamlPackages_4_12.findlib ocaml-ng.ocamlPackages_4_12.apron --run 'ocamlfind query -r apron'

ocamlfind: Package `apron' not found

ulrikstrid · 2022-03-03T12:41:50Z

I can't reproduce this on master

nix-shell -p ocaml-ng.ocamlPackages_4_12.findlib ocaml-ng.ocamlPackages_4_12.apron --run 'ocamlfind query -r apron'
/nix/store/mps7hcb0qp7cf4l5k582asxwxhdl08kx-ocaml-4.12.0/lib/ocaml
/nix/store/mps7hcb0qp7cf4l5k582asxwxhdl08kx-ocaml-4.12.0/lib/ocaml
/nix/store/hiisfrrljwbj99h4q0spqizzj2xgr7c5-ocaml4.12.0-mlgmpidl-1.2.12/lib/ocaml/4.12.0/site-lib/gmp
/nix/store/lf0cyn31apa7yi2f25w6dih528cjbn3d-ocaml4.12.0-apron-0.9.13-dev/lib/ocaml/4.12.0/site-lib/apron
/nix/store/lf0cyn31apa7yi2f25w6dih528cjbn3d-ocaml4.12.0-apron-0.9.13-dev/lib/ocaml/4.12.0/site-lib/apron

vbgl · 2022-03-03T12:46:41Z

You may need to set the NIX_PATH environment variable.

ulrikstrid · 2022-03-03T12:51:20Z

This would be the same thing right?

➜  nixpkgs git:(master)  nix-shell -I ./default.nix -p ocaml-ng.ocamlPackages_4_12.findlib ocaml-ng.ocamlPackages_4_12.apron
[nix-shell:~/dev/nixpkgs]$ ocamlfind query -r apron
/nix/store/mps7hcb0qp7cf4l5k582asxwxhdl08kx-ocaml-4.12.0/lib/ocaml
/nix/store/mps7hcb0qp7cf4l5k582asxwxhdl08kx-ocaml-4.12.0/lib/ocaml
/nix/store/hiisfrrljwbj99h4q0spqizzj2xgr7c5-ocaml4.12.0-mlgmpidl-1.2.12/lib/ocaml/4.12.0/site-lib/gmp
/nix/store/lf0cyn31apa7yi2f25w6dih528cjbn3d-ocaml4.12.0-apron-0.9.13-dev/lib/ocaml/4.12.0/site-lib/apron
/nix/store/lf0cyn31apa7yi2f25w6dih528cjbn3d-ocaml4.12.0-apron-0.9.13-dev/lib/ocaml/4.12.0/site-lib/apron

vbgl · 2022-03-03T13:06:35Z

Did you try: nix-shell -I nixpkgs=./default.nix …?

vbgl · 2022-03-03T13:07:02Z

Or -I ..?

ulrikstrid · 2022-03-03T13:29:48Z

nix-shell -I nixpkgs=./default.nix -p ocaml-ng.ocamlPackages_4_12.findlib ocaml-ng.ocamlPackages_4_12.apron show the error, let me investigate

ulrikstrid · 2022-03-03T14:10:01Z

Thanks for the quick fix. Bisecting the failure of ocaml-ng.ocamlPackages_4_07.ppx_let led me here again.

It almost looks like dune2 was somehow available for ppxlib even in a case where it shouldn't have been. I have a PR coming up fixing this, I just need to test it with a couple of other OCaml versions before submitting.

vbgl · 2022-03-03T14:18:18Z

ocamlPackages.elina seems broken too.

ulrikstrid · 2022-03-04T19:21:00Z

@vbgl elina and a bunch of other packages are fixed in #162745, would love a review when you have time

Et7f3 · 2023-01-24T00:03:31Z

pkgs/development/ocaml-modules/ppx_tools/default.nix

@@ -5,6 +5,7 @@ let param =
    version = "6.4";
    sha256 = "15v7yfv6gyp8lzlgwi9garz10wpg34dk4072jdv19n6v20zfg7n1";
    useDune2 = true;
+    nativeBuildInputs = [cppo];
    buildInputs = [cppo];


Is it a left over ?

Can't remember but might be that there's a library component of cppo that was also needed?

Ok I will do another round. I see other packages with the same issue.

github-actions bot added the 6.topic: ocaml label Feb 22, 2022

ulrikstrid force-pushed the ulrikstrid/tree-wide-native-inputs branch from f2fc6db to 70dbc54 Compare February 22, 2022 10:06

ulrikstrid requested review from vbgl, sternenseemann and symphorien February 22, 2022 10:08

ulrikstrid force-pushed the ulrikstrid/tree-wide-native-inputs branch from 70dbc54 to 2e9d67a Compare February 22, 2022 10:09

ulrikstrid requested review from alyssais and anmonteiro February 22, 2022 10:10

ulrikstrid force-pushed the ulrikstrid/tree-wide-native-inputs branch from 2e9d67a to a43a8a0 Compare February 22, 2022 10:27

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Feb 22, 2022

ulrikstrid force-pushed the ulrikstrid/tree-wide-native-inputs branch 14 times, most recently from 29fd001 to 14d80f6 Compare February 22, 2022 15:57

symphorien reviewed Feb 22, 2022

View reviewed changes

ulrikstrid force-pushed the ulrikstrid/tree-wide-native-inputs branch from 44ed833 to f3db7c9 Compare February 22, 2022 19:59

ulrikstrid requested a review from symphorien February 24, 2022 15:45

symphorien approved these changes Feb 25, 2022

View reviewed changes

symphorien merged commit b9ecdae into NixOS:master Feb 27, 2022

ulrikstrid deleted the ulrikstrid/tree-wide-native-inputs branch February 27, 2022 19:55

anmonteiro added a commit to nix-ocaml/nix-overlays that referenced this pull request Feb 28, 2022

Check if NixOS/nixpkgs#161344 builds (#183)

7774df9

* wip * check if still builds * remove file * update melange

symphorien mentioned this pull request Mar 1, 2022

ocaml-ng.ocamlPackages_4_07.lwt: fix build with strictDeps #162370

Merged

13 tasks

ulrikstrid mentioned this pull request Mar 1, 2022

ocamlPackages treewide: strictDeps all packages #162385

Merged

13 tasks

symphorien mentioned this pull request Mar 1, 2022

ocamlPackage.ppx_deriving: fix build with ocaml 4.07 #162421

Merged

13 tasks

ulrikstrid mentioned this pull request Mar 3, 2022

ocamlPackages.apron: set strictDeps = false to fix install failure #162628

Merged

13 tasks

This was referenced Mar 3, 2022

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

Merged

ocamlPackages: fix broken packages #162745

Merged

Et7f3 reviewed Jan 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ocamlPackages tree-wide: Move buildInputs that should be nativeBuildInputs #161344

ocamlPackages tree-wide: Move buildInputs that should be nativeBuildInputs #161344

ulrikstrid commented Feb 22, 2022 •

edited

Loading

symphorien left a comment

symphorien Feb 22, 2022

ulrikstrid Feb 22, 2022

symphorien Feb 22, 2022

symphorien Feb 23, 2022

ulrikstrid commented Feb 22, 2022

symphorien commented Feb 22, 2022

ulrikstrid commented Feb 22, 2022

symphorien left a comment

ulrikstrid commented Feb 26, 2022

symphorien commented Feb 26, 2022

vbgl commented Mar 1, 2022

symphorien commented Mar 1, 2022

vbgl commented Mar 1, 2022

vbgl commented Mar 1, 2022

symphorien commented Mar 1, 2022

ulrikstrid commented Mar 2, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

vbgl commented Mar 3, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 4, 2022

Et7f3 Jan 24, 2023

ulrikstrid Jan 24, 2023

Et7f3 Jan 24, 2023

ocamlPackages tree-wide: Move buildInputs that should be nativeBuildInputs #161344

ocamlPackages tree-wide: Move buildInputs that should be nativeBuildInputs #161344

Conversation

ulrikstrid commented Feb 22, 2022 • edited Loading

Motivation for this change

Things done

symphorien left a comment

Choose a reason for hiding this comment

symphorien Feb 22, 2022

Choose a reason for hiding this comment

ulrikstrid Feb 22, 2022

Choose a reason for hiding this comment

symphorien Feb 22, 2022

Choose a reason for hiding this comment

symphorien Feb 23, 2022

Choose a reason for hiding this comment

ulrikstrid commented Feb 22, 2022

symphorien commented Feb 22, 2022

ulrikstrid commented Feb 22, 2022

symphorien left a comment

Choose a reason for hiding this comment

ulrikstrid commented Feb 26, 2022

symphorien commented Feb 26, 2022

vbgl commented Mar 1, 2022

symphorien commented Mar 1, 2022

vbgl commented Mar 1, 2022

vbgl commented Mar 1, 2022

symphorien commented Mar 1, 2022

ulrikstrid commented Mar 2, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

vbgl commented Mar 3, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

ulrikstrid commented Mar 3, 2022

vbgl commented Mar 3, 2022

ulrikstrid commented Mar 4, 2022

Et7f3 Jan 24, 2023

Choose a reason for hiding this comment

ulrikstrid Jan 24, 2023

Choose a reason for hiding this comment

Et7f3 Jan 24, 2023

Choose a reason for hiding this comment

ulrikstrid commented Feb 22, 2022 •

edited

Loading