Fix derivations setting createFindlibDestdir in nix-shell

[Zimmi48]

Motivation for this change

Fix #121991

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• 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 nixpkgs-review --run "nixpkgs-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)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[sternenseemann]

I think it'd be preferrable to inject this feature into the installPhase (by appending to the preCheck hook maybe?) rather than doing something in the setup hook at all times and ignoring it when it fails.

This is especially an issue since you can get a nix-shell which can write to /nix/store

[Zimmi48]

by appending to the preCheck hook maybe?

There is also a preInstall hook. Is there any reason you didn't suggest this one?

[sternenseemann]

Typo, I would suggest preInstall of course.

[Zimmi48]

Here is a new version based on this idea that seems to work as well. I didn't know how to append to the preInstall hook, so I just replaced it.

[r-rmcgibbo]

Result of nixpkgs-review pr 121992 at d0bcd0ea run on aarch64-linux 1

17 packages marked as broken and skipped:

• coccinelle
• coq_8_5
• coq_8_6
• cubicle
• haxe_3_2
• haxe_3_4
• libvmi
• monotoneViz
• obliv-c
• opa
• ...

63 packages skipped due to time constraints:

• abella
• acgtk
• alt-ergo
• beluga
• coq_8_10
• coq_8_12
• coq_8_13
• coq_8_7
• coq_8_8
• coq_8_9
• ...

53 packages built successfully:

• coq (coqPackages.coq ,coq_8_11)
• coqPackages.Cheerios
• coqPackages.CoLoR
• coqPackages.ITree
• coqPackages.InfSeqExt
• coqPackages.QuickChick
• coqPackages.StructTact
• coqPackages.Verdi
• coqPackages.bignums
• coqPackages.contribs.zorns-lemma
• coqPackages.coq-bits
• coqPackages.coq-elpi
• coqPackages.coq-ext-lib
• coqPackages.coqeal
• coqPackages.coqhammer
• coqPackages.coqprime
• coqPackages.coqtail-math
• coqPackages.coquelicot
• coqPackages.corn
• coqPackages.dpdgraph
• coqPackages.equations
• coqPackages.flocq
• coqPackages.fourcolor
• coqPackages.gappalib
• coqPackages.hierarchy-builder
• coqPackages.interval
• coqPackages.iris
• coqPackages.math-classes
• coqPackages.mathcomp
• coqPackages.mathcomp-abel
• coqPackages.mathcomp-algebra
• coqPackages.mathcomp-analysis
• coqPackages.mathcomp-bigenough
• coqPackages.mathcomp-character
• coqPackages.mathcomp-field
• coqPackages.mathcomp-fingroup
• coqPackages.mathcomp-finmap
• coqPackages.mathcomp-real-closed
• coqPackages.mathcomp-solvable
• coqPackages.ssreflect (coqPackages.mathcomp-ssreflect)
• coqPackages.metalib
• coqPackages.multinomials
• coqPackages.odd-order
• coqPackages.paco
• coqPackages.paramcoq
• coqPackages.simple-io
• coqPackages.stdpp
• coqPackages.tlc
• dune_2
• framac
• hevea
• opaline
• why3

[Zimmi48]

@sternenseemann Does the new version look good to you?

[sternenseemann]

I've thought about it and I think overriding the preInstall hook may gives us trouble. For one I don't really like it because it becomes harder to use preInstall and overriding is probably compromised. More importantly though I think a lot of ocaml packages don't run the preInstall hook due to installPhases` like this:

  installPhase = "ocaml setup.ml -install";

I think the easiest could be to remove createFindlibDestdir altogether and insert the mkdir calls manually everywhere where it is needed. We don't really need to keep this around since the ecosystem seems to be converging on dune anyways. What do you think? I'm somewhat inclined to sacrifice myself and do the chore as well.

[Zimmi48]

I didn't really consider this option because I thought it would be a huge task. Checking now with grep, I see that there are still 116 occurrences. That's a lot but indeed not that many compared to the 812 occurrences of buildDunePackage. So if you are up to the task, I'd be happy with the solution you propose.

[sternenseemann]

You can follow my progress at #124504, taking a break now…

[sternenseemann]

As @symphorien pointed out, alternatively we could also add a new phase via the setup hook like autoreconfHook does:

nixpkgs/pkgs/build-support/setup-hooks/autoreconf.sh

Lines 1 to 7 in 1834bc8

	preConfigurePhases+=" autoreconfPhase"
	autoreconfPhase() {
	runHook preAutoreconf
	autoreconf ${autoreconfFlags:---install --force --verbose}
	runHook postAutoreconf
	}

[Zimmi48]

Given that your PR is ready, and that this feature was becoming less and less useful with the general move toward Dune, I wouldn't bother doing differently now.

[sternenseemann]

@vbgl what do you think?

[vbgl]

I acknowledge that there is a problem that should be fixed.

This PR has a QnD fix that looks OK but causes a massive rebuild so we have to double-check before merging…

PR 124504 seems much better (a lot of welcome cleaning, a thorough understanding of how to fix the issue) but the change is large thus tedious to review.

[stale]

I marked this as stale due to inactivity. → More info

[sternenseemann]

Something in a similar spirit was incidentally implemented in #133008: a3ab43d.