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

[RFC/WIP] multiple-outputs: Change the order of outputs to have binaries in the primary output #14766

Merged
merged 27 commits into from Aug 30, 2016

Conversation

@dezgeg
Contributor

dezgeg commented Apr 16, 2016

This series reorders the outputs of each multiple-output splitted package in nixpkgs to have the output that is installed into the user environment be the first one (though I made a single exception: glibc has its libraries first to keep usages like ${stdenv.cc.libc}/lib/ld-linux-x86-64.so.2 working). Previously the dev output was typically the primary one, although there were some exceptions like the kernel package. To accommodate this, mkDerivation was changed to use the dev output by default in packages passed to buildInputs and friends.

The rationale for doing this is to avoid breakage to users' scripts and systemd services that reference binaries like ${pkgs.perl}/bin/perl (e.g. https://github.com/NixOS/nixos-org-configurations/blob/master/delft/common.nix#L62). Currently, those usages will just silently produce a nonsensical path, which IMHO is a significant regression to the pre-multiple-outputs state.

Also this should fix some of our in-tree systemd units which suffer from the same issue and avoid the need for changes like 3e38a71

As a part of doing this, I manually eyeballed all usages of ${pkgs.multiout-package} . I collected this list of packages where the implicit dev is currently incorrectly used in place of the correct output: https://github.com/dezgeg/nixpkgs/commits/bad-multiout-refs. So this PR fixes some of them as a side-effect (though not all, there are other things wrong as well).

Note: I'm still compiling most of this (at least stdenv builds...). It's certainly possible that chooseDevOutputs needs to be used in more places...

cc @edolstra @vcunat @ttuegel @joachifm @lethalman

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Apr 16, 2016

Member

Hmm, yes, probably. At least in principle, though I haven't thought through many potential consequences yet. I'm not certain how fast I get to this properly. I suppose I'd first solve issues like #14682, as that seems some easily fixable sloppy programming (of mine).

Member

vcunat commented Apr 16, 2016

Hmm, yes, probably. At least in principle, though I haven't thought through many potential consequences yet. I'm not certain how fast I get to this properly. I suppose I'd first solve issues like #14682, as that seems some easily fixable sloppy programming (of mine).

@cstrahan

This comment has been minimized.

Show comment
Hide comment
@cstrahan

cstrahan Apr 16, 2016

Contributor

The general idea here looks good to me. The changes to multiple-outputs.sh look good (particularly with respect to the defaulting of $propagaterOutput), and the exception for output order for glibc seems reasonable given how pervasive the current typical use case is. Hopefully this will make things a little easier on newcomers; it could be my own bias from my familiarity with the old status quo, but I think "${foo}/bin/foo" and "${foo.dev}/include" is more intuitive than "${foo.bin}/bin/foo" and "${foo}/include".

👍

Contributor

cstrahan commented Apr 16, 2016

The general idea here looks good to me. The changes to multiple-outputs.sh look good (particularly with respect to the defaulting of $propagaterOutput), and the exception for output order for glibc seems reasonable given how pervasive the current typical use case is. Hopefully this will make things a little easier on newcomers; it could be my own bias from my familiarity with the old status quo, but I think "${foo}/bin/foo" and "${foo.dev}/include" is more intuitive than "${foo.bin}/bin/foo" and "${foo}/include".

👍

@cstrahan

This comment has been minimized.

Show comment
Hide comment
@cstrahan

cstrahan Apr 16, 2016

Contributor

I forgot to mention, I also approve of having buildInputs in the stdenv default to using the dev output, expecting it to follow the convention of propagating the other requisite outputs.

I'd like to see this get pushed along, assuming no one comes along with a reasonable objection.

Contributor

cstrahan commented Apr 16, 2016

I forgot to mention, I also approve of having buildInputs in the stdenv default to using the dev output, expecting it to follow the convention of propagating the other requisite outputs.

I'd like to see this get pushed along, assuming no one comes along with a reasonable objection.

@abbradar

This comment has been minimized.

Show comment
Hide comment
@abbradar

abbradar Apr 17, 2016

Member

I don't have a strong opinion either way, but to new users this would probably be more consistent with Debian and Red Hat-based distributions' -devel packages.

Member

abbradar commented Apr 17, 2016

I don't have a strong opinion either way, but to new users this would probably be more consistent with Debian and Red Hat-based distributions' -devel packages.

@joachifm

This comment has been minimized.

Show comment
Hide comment
@joachifm

joachifm Apr 17, 2016

Contributor

Agreed, this sounds like a good idea, so better to just deal with any bugs.

Contributor

joachifm commented Apr 17, 2016

Agreed, this sounds like a good idea, so better to just deal with any bugs.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Apr 17, 2016

Contributor

I built closures.smallContainer.x86_64-linux and the minimal installation CD successfully (way better than I expected...). There are some minor evaluation errors exposed by this series (e.g. GHC using a version of gmp that is not split) that I need to deal with.

Contributor

dezgeg commented Apr 17, 2016

I built closures.smallContainer.x86_64-linux and the minimal installation CD successfully (way better than I expected...). There are some minor evaluation errors exposed by this series (e.g. GHC using a version of gmp that is not split) that I need to deal with.

@cstrahan cstrahan referenced this pull request Apr 17, 2016

Closed

Search path fixes, v2 #14694

1 of 7 tasks complete
@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Apr 18, 2016

Member

Doesn't this break

buildInputs = [ foo ];

since you would now have to write

buildInputs = [ foo.dev ];

? That was the whole reason for putting dev first.

Member

edolstra commented Apr 18, 2016

Doesn't this break

buildInputs = [ foo ];

since you would now have to write

buildInputs = [ foo.dev ];

? That was the whole reason for putting dev first.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Apr 18, 2016

Contributor

@edolstra: No, that is handled by 090c967

Contributor

dezgeg commented Apr 18, 2016

@edolstra: No, that is handled by 090c967

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Apr 18, 2016

Member

Okay, I see there is some special-casing for this in mkDerivation. But that seems confusing to me, i.e. foo will mean foo.dev in some contexts but not others.

Member

edolstra commented Apr 18, 2016

Okay, I see there is some special-casing for this in mkDerivation. But that seems confusing to me, i.e. foo will mean foo.dev in some contexts but not others.

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Apr 18, 2016

Member

Well, the whole point is to try guessing what output was meant when none was specified... obviously the best guess depends on context.

Member

vcunat commented Apr 18, 2016

Well, the whole point is to try guessing what output was meant when none was specified... obviously the best guess depends on context.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Apr 18, 2016

Contributor

Yes, that is pretty much inevitable. Even currently (as in before this PR) environment.systemPackages = [ pkgs.curl ]; doesn't refer to curl.dev.

Also cases like services.httpd.package = pkgs.apacheHttp_2_4; - that probably shouldn't refer to the dev output either.

Contributor

dezgeg commented Apr 18, 2016

Yes, that is pretty much inevitable. Even currently (as in before this PR) environment.systemPackages = [ pkgs.curl ]; doesn't refer to curl.dev.

Also cases like services.httpd.package = pkgs.apacheHttp_2_4; - that probably shouldn't refer to the dev output either.

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Apr 18, 2016

Member

The last one is correct, I believe. The users of services.httpd.package should choose the particular output, as they might need different on different places.

Member

vcunat commented Apr 18, 2016

The last one is correct, I believe. The users of services.httpd.package should choose the particular output, as they might need different on different places.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Apr 22, 2016

Contributor

#14902 seems another victim of silent breakage by accidentally using the dev output.

Contributor

dezgeg commented Apr 22, 2016

#14902 seems another victim of silent breakage by accidentally using the dev output.

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Apr 26, 2016

Member

I don't think there can be any argument against the commits that make output choice explicit (they make majority of the diff). These shouldn't even change hashes and could go to master directly (before conflicts can arise).

Member

vcunat commented Apr 26, 2016

I don't think there can be any argument against the commits that make output choice explicit (they make majority of the diff). These shouldn't even change hashes and could go to master directly (before conflicts can arise).

@trishume

This comment has been minimized.

Show comment
Hide comment
@trishume

trishume May 1, 2016

Contributor

I ran into some issues with this except with libraries defaulting to dev instead of out, which perhaps makes more sense than binaries. Example issue caused by this problem: #15124

Contributor

trishume commented May 1, 2016

I ran into some issues with this except with libraries defaulting to dev instead of out, which perhaps makes more sense than binaries. Example issue caused by this problem: #15124

vcunat added a commit to vcunat/nixpkgs that referenced this pull request May 19, 2016

Merge: make dev output references explicit
This is a rebase of most commits from #14766,
resolving conflicts and a few other evaluation problems.

vcunat added a commit to vcunat/nixpkgs that referenced this pull request May 20, 2016

Merge: make dev output references explicit
This is a rebase of most commits from #14766,
resolving conflicts and a few other evaluation problems.

vcunat added a commit that referenced this pull request May 22, 2016

Merge: make dev output references explicit
This is a rebase of most commits from #14766,
resolving conflicts and a few other evaluation problems.
@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Aug 29, 2016

Member

I suppose we can fixup various details after staging this.

I wonder whether to re-consider the default setting of meta.outputsToInstall, but probably not.

Member

vcunat commented Aug 29, 2016

I suppose we can fixup various details after staging this.

I wonder whether to re-consider the default setting of meta.outputsToInstall, but probably not.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Aug 29, 2016

Contributor

Right, I would just like a confirmation from @edolstra if he agrees with this or not. Also @domenkozar might have something to say about one more mass-rebuild thing to 16.09 ;)

Contributor

dezgeg commented Aug 29, 2016

Right, I would just like a confirmation from @edolstra if he agrees with this or not. Also @domenkozar might have something to say about one more mass-rebuild thing to 16.09 ;)

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Aug 29, 2016

Member

No objections for mass-rebuild if it's done before Thursday and it works (doesn't break too many packages and all tests pass).

Member

domenkozar commented Aug 29, 2016

No objections for mass-rebuild if it's done before Thursday and it works (doesn't break too many packages and all tests pass).

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Aug 29, 2016

Member

Looks good to me.

Member

edolstra commented Aug 29, 2016

Looks good to me.

@lethalman

This comment has been minimized.

Show comment
Hide comment
@lethalman

lethalman Aug 29, 2016

Contributor

Good. Can you please update release notes about this? Also the dev / devEnv change of ruby.

Contributor

lethalman commented Aug 29, 2016

Good. Can you please update release notes about this? Also the dev / devEnv change of ruby.

@lethalman

This comment has been minimized.

Show comment
Hide comment
@lethalman

lethalman Aug 29, 2016

Contributor

There are some more places where .dev needs to be used I think, like for nss nspr gcc glibc glib readline ncurses gio and all these libraries that sometimes are used in build flags of packages. Searching for "-I" might help.

Contributor

lethalman commented Aug 29, 2016

There are some more places where .dev needs to be used I think, like for nss nspr gcc glibc glib readline ncurses gio and all these libraries that sometimes are used in build flags of packages. Searching for "-I" might help.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Aug 30, 2016

Contributor

I had already done most of those changes in master (e.g. 4db1657) via some regexps just to be able to spot breakages (e.g. d801e8a) but now that you mention, I indeed managed to miss some, thanks.

Should now be in a condition where I can stage this in the morning after some overnight build testing.

Contributor

dezgeg commented Aug 30, 2016

I had already done most of those changes in master (e.g. 4db1657) via some regexps just to be able to spot breakages (e.g. d801e8a) but now that you mention, I indeed managed to miss some, thanks.

Should now be in a condition where I can stage this in the morning after some overnight build testing.

@dezgeg

This comment has been minimized.

Show comment
Hide comment
@dezgeg

dezgeg Aug 30, 2016

Contributor

Oh, and cc @zimbatm for the Ruby change (ba6d94e). Does this rename sound good, or would some other name be better?

Reason for changing is that the name dev is used in the multiple-outputs stuff and causes problem there.

Also, any documentation references to update? Didn't notice any but the name dev is not very greppable.

Contributor

dezgeg commented Aug 30, 2016

Oh, and cc @zimbatm for the Ruby change (ba6d94e). Does this rename sound good, or would some other name be better?

Reason for changing is that the name dev is used in the multiple-outputs stuff and causes problem there.

Also, any documentation references to update? Didn't notice any but the name dev is not very greppable.

@zimbatm

This comment has been minimized.

Show comment
Hide comment
@zimbatm

zimbatm Aug 30, 2016

Member

Looks good. I don't think ruby.dev isn't documented right now and I'm considering dropping it in the future.

Member

zimbatm commented Aug 30, 2016

Looks good. I don't think ruby.dev isn't documented right now and I'm considering dropping it in the future.

@dezgeg dezgeg merged commit 03fb2c1 into NixOS:staging Aug 30, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

dezgeg added a commit that referenced this pull request Sep 1, 2016

Merge staging into master
Brings in:
    - changed output order for multiple outputs:
      #14766
    - audit disabled by default
      #17916

 Conflicts:
	pkgs/development/libraries/openldap/default.nix

dezgeg added a commit that referenced this pull request Sep 1, 2016

@lethalman

This comment has been minimized.

Show comment
Hide comment
@lethalman

lethalman Sep 5, 2016

Contributor

Wow, this is on staging? Awesome work!

Contributor

lethalman commented Sep 5, 2016

Wow, this is on staging? Awesome work!

@vcunat

This comment has been minimized.

Show comment
Hide comment
@vcunat

vcunat Sep 5, 2016

Member

It's in master, actually, since 8c4aeb1.

Member

vcunat commented Sep 5, 2016

It's in master, actually, since 8c4aeb1.

@grahamc

This comment has been minimized.

Show comment
Hide comment
@grahamc

grahamc Sep 5, 2016

Member

(and in 16.09!)

Member

grahamc commented Sep 5, 2016

(and in 16.09!)

@joachifm joachifm referenced this pull request Sep 7, 2016

Closed

add nix.optimise service #18378

4 of 7 tasks complete

@nbp nbp referenced this pull request Feb 26, 2017

Merged

RFC to describe the RFC process #1

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment