[WIP] Implement RFC0035 II: Electric Boogaloo

[Synthetica9]

Motivation for this change

Follow-up for #49398

TODO:

Should I split this up in two PR's? Part of this is good to go and should just go in the working tree straight away.

• The basic change
• Consistency check
• Check the entire tree is consistent (it is now!)
• Document the new check
• Docs for the new hasInfix library function
• Currently interacts really weirdly with Coq, and I have no idea why.
• Check everything still works.

cc @dtzWill

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 nox --run "nox-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)
• Fits CONTRIBUTING.md.

---

[Synthetica9]

Ofborg's current output is the problem I'm having with Coq, and I'm not sure what is even happening, so any help is welcome there

[Zimmi48]

cc @vbgl You might have an idea about the Coq-related issue in this PR.

[Zimmi48]

@Synthetica9 If I understand correctly, this allows the scheme name = "ocaml${ocaml.version}-${pname}-${version}" or name = "coq${coq.version}-${pname}-${version}", etc. Correct?

[Synthetica9]

@Synthetica9 If I understand correctly, this allows the scheme name = "ocaml${ocaml.version}-${pname}-${version}" or name = "coq${coq.version}-${pname}-${version}", etc. Correct?

Correct. The current check is whether both pname and version (if the attributes exist) appear in name in their entirety. So any prefix, postfix, whatever is allowed. This was the most forgiving check I could think of that still meant anything, and it found a few genuine problems (see: most of the commits in this PR).

[vbgl]

May I suggest that the issue is in your code rather than in the Coq package descriptions? It seems that the checks are run too early.

Please do remove the failing assertions (assert param != null). They break evaluation.

[zimbatm]

if all of this compiles, then let's get this merged in first

[vbgl]

Can we ensure that mkDerivation is lazy? We rely on this property in coqPackages.

[Synthetica9]

Can we ensure that mkDerivation is lazy? We rely on this property in coqPackages.

Only in the places altered in 0ab59bc37b21a13dc35b9d902d78f26af4bb56f2? Because I am currently testing a fix that should fix the issues caused by that. It seems to work fine (testing by compiling coqPackages_8_8)

  param = params."${coq.coq-version}" or null;
in

if param == null then null else
stdenv.mkDerivation rec {

[vbgl]

What would be the value of builtins.hasAttr "coqprime" coqPackages_8_5 with your fix? I expect “false”.

[Synthetica9]

@vbgl Everything should be okay now!

[vbgl]

Thanks for your understanding. LGTM.

[Synthetica9]

(Rebased to squash some commits, clarify a few messages, and solve merge conflicts)

[Synthetica9]

Currently building on my Hydra (private, so no link, sorry!)

Seems to work fine (ran out of disk space at about 50%, but no errors yet):

[Synthetica9]

@Mic92 Willing to give this another try?

[Synthetica9]

Okay, next order of business is doing refactors in the tree to make packages use this pattern. I think that should go in master, once this PR gets there, am I correct?

[timokau]

Usually. If it actually changes the resulting name it will cause rebuilds though. If the package is high enough in the dpendency tree, that should go to staging. So if ofBorg reports excessive (>100) rebuilds, rebase on staging.

[Synthetica9]

I was thinking of packages that define name = "${pname}-${version}", which shouldn't cause rebuilds at all, right?

[timokau]

Not sure, since the default builder will add pname as an environment variable for the build I think.

[dtzWill]

nixos-version was failing this check for me--but I might have missed some of the new goodness, and due to strange network problems it's not easy to confidently check against master right now. It's probably been fixed but if not thought I'd mention just in case :).

[Synthetica9]

See nlewo#1. Can't open a pr to staging atm (laptop's empty), but feel free to cherry-pick

…

On Wed, 14 Nov 2018, 14:58 Will Dietz, ***@***.***> wrote: nixos-version was failing this check for me--but I might have missed some of the new goodness, and due to strange network problems it's not easy to confidently check against master right now. It's probably been fixed but if not thought I'd mention just in case :). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#49769 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGv3pzeImtnhFlr-Tx4FDtCo1GVrHa6Xks5uvCF4gaJpZM4YNloU> .

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293/4