Search path fixes, v2

[abbradar]

Things done

• Tested using sandboxing (nix-build --option build-use-chroot true or nix.useChroot on NixOS)
• Built on platform(s)
  • NixOS
  • OS X
  • Linux
• 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/)
• Fits CONTRIBUTING.md.

---

This is continuation of my attempts at having outputs correctly used tree-wise. We now have a more specific function makeSearchPathOutput, defined via getOutput -- a new function for getting output from a package with necessary checks and fallbacks. This function is very useful on its own, so I've run a few regexps tree-wise to use it instead of ad-hoc a.needed or a.out or a. What I don't like is that as of now ideally this function should be used anywhere to abstract from splitted/unsplitted packages, and that would be a slowdown and also clunky-looking. However it's better than ad-hoc fixes anyway and this is a price for multiple outputs -- a little one for its benefits ~_^.

cc @vcunat @edolstra

[mention-bot]

By analyzing the blame information on this pull request, we identified @vcunat, @bnikolic and @dezgeg to be potential reviewers

[7c6f434c]

Looks nice

[abbradar]

I had an idea right now to instead define out, lib, dev and bin everywhere, aliasing them as needed for packages that are not splitted. What do you think?

[jcumming]

This is just the tool I was looking for to fix the torbrowser derivation..

[abbradar]

Notice that you can do it already -- just use makeLibraryPath, makeBinPath or makeSearchPathOutputs "needed/path" ["neededOutput"] (the latter will be replaced in this PR -- ping me to include an update).

EDIT: That is, if you aren't talking about getOutput.

[cstrahan]

This looks pretty great. I'd like to see this and #14766 merged.

[abbradar]

This looks OK to merge for me, but first I'd like to gather feedback on an idea briefly described in #14694 (comment). In more details:

Right now we are slowly moving through splitting more and more packages. Also, we have a lot of packages that refer to other packages' outputs explicitly for various reasons, like FOO_PATH = "${foo}/bin/foo. This would break when foo is split (well, in this case it would not break after #14766 but then it would break ${foo}/include). So, to simplify splitting packages in future new code would likely be written to abstract over split outputs, as it is already written now; for example, FOO_PATH = "${foo.dev or foo.out or foo}/include". Of course writing this manually is too cumbersome, so this PR introduces lib.getOutput which covers all the details you need to consider (i.e. outputUnspecified, checking out and so on). However, writing lib.getOutput "dev" foo everywhere is cumbersome too, and so (arguably) is lib.getDev foo (or stdenv.lib.getDev foo!).

What I propose instead is to add several most popular attributes to all packages by default. They would point to needed outputs in case the package was really split, and would be an alias if it wasn't. I think the most used ones are lib, dev and bin. Of course there are more, but those are 95% of cases (subjectively -- I don't have numbers) of all "explicit usage" (i.e. not in environmentPackages, buildInputs or other places which can implement their own choosing logic). This would simplify described problem to just foo.dev without pulling lib and using a function.

What do you think? I'm willing to try to implement that if people are interested.

[cstrahan]

@abbradar I don't have any strong opinions. On the surface, that sounds nice though.

/cc @edolstra -- I figure you might have opinions wrt #14694 (comment).

[vcunat]

Why such a complex code? I'd write

if pkg.outputUnspecified or false
  then pkg.${output} or pkg.out or pkg
  else pkg

[abbradar]

Oh, that's because I'm constantly forgetting that I can do pkg.${foo} in Nix!

[vcunat]

I had an idea right now to instead define out, lib, dev and bin everywhere, aliasing them as needed for packages that are not splitted.

I had thought of that briefly before. It seems the best way I know of to go in long term. To do that properly, these would have to react to vars like outputDev, implying that the process of selecting defaults should be done during evaluation and not during build. The complexity of that change is what had dissuaded me from doing it straight away. (The branch was running for years even without adding such changes.)

[vcunat]

This PR looks good to me. (Mainly the general stuff; I only skimmed the package-specific changes.) I suppose we should go with it (or most of it) until someone implements that bigger plan outlined in my previous comment.

[abbradar]

Hm, I think that we indeed can merge it as is -- get{Dev,Lib,Bin} are quite regex-able functions, so when (if) we implement the plan we can just go over the code, replace them where found and drop get??? from the library.

Rebased and updated as per @vcunat's comments (and replaced all the new makeLibraryPathOutputs usages).

[edolstra]

Adding sbin to search paths is obsolete, since we automatically move sbin/* to bin/.

[abbradar]

Hm, I saw several places in code where sbin was accounted for. Do we always move it, or are there exceptions?

[vcunat]

I don't know of a possible reason not to move it. The occurrences are most likely just legacy.

[vcunat]

Note that dontMoveSbin is unused in nixpkgs.

[abbradar]

I thought of looking through all our sbin uses and replace them, but we have too much of them: grep -r 'sbin' | wc -l shows 835 occurrences. I think removing them is out of scope of this PR, although I can remove those that I touched if desired.

[vcunat]

It would be more fitting for a separate mass-replace commit. And I'd still probably leave the automatic sbin -> bin symlinks.

[abbradar]

Yeah, I thought of just removing things like mentions in PATH.

[abbradar]

I'm getting a mass rebuild now which shouldn't be the case. How do we usually debug this? .drvs are not very nice to diff...

[abbradar]

Ouch, it accidentally uncovered a bug in gcc wrapper D:
Specifically, old code in pkgs/build-support/cc-wrapper/default.nix:

  binutils_bin = if nativeTools then "" else binutils.bin or binutils;

This incorrectly resolved to binutils.dev (because .bin doesn't exist). It resolves to binutils.out now.

[abbradar]

I'd say problems like these is why we need to encourage everyone to specify exactly what they mean instead of just a package and get the "get several outputs to all derivations as aliases" plan finished sooner (so that keystroke laziness would be less an issue). ${foo => include/foo.h} would help, too (although not in this case).

[vcunat]

.drvs are not very nice to diff...

Just a few hours ago I used kdiff3 on derivations and it handled them quite well, wrapping and highlighting the differing stuff. (It helped me to find the root of #14965.)

[abbradar]

In the end I used meld because that was the first thing showed up in DDG. I guess I need to find how to do this properly in Emacs...

So, what else should be done before merging this? I'm building hello now, just in case.

[vcunat]

I suppose we can stage it. The risk of mass breakage seems low enough.

[abbradar]

hello built successfully, so it should be okay I think. We have more things that we might want to stage simultaneously:
#14909 (simple enough)
#14907 (sounds ready, and given that my PR does little in terms of actual build changes can be the primary test subject)

EDIT: by "does little" I meant "little" in terms of what could break: I suppose that if it worked before with incorrect path to binutils it wouldn't be worse now or would fail with distinguishable error messages.

EDIT2: Simples! It hasn't been broken because .dev has a symlink for /bin.

[vcunat]

Sounds fine.

[abbradar]

Closed by 5f19542