nixos: use pkgsi686Linux for pkgs_i686 #24772

corngood · 2017-04-09T16:42:58Z

Motivation for this change

I found that my packageOverrides for e.g. mesa_drivers weren't being respected in pkgs_i686, so my override was having an effect in /run/opengl-drivers/ but not /run/opengl-drivers32/.

This is what I came up with, but it's a pretty fundamental change, and I'm not totally sure of the consequences. It does seem to behave more intuitively though. Presumably overlays would be affected in a similar way.

Thoughts?

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- 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.

mention-bot · 2017-04-09T16:43:00Z

@corngood, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nckx, @nbp and @edolstra to be potential reviewers.

nbp · 2017-04-10T12:48:57Z

If so, we should probably just remove the pkgs_i686 module argument, and add a release note with an example which highlight how to replace pkgs_i686 by pkgs.pkgsi686Linux.

corngood · 2017-04-10T14:29:01Z

@nbp That seems reasonable to me. What about then renaming pkgsi686Linux to pkgs_i686? It's mostly used like:

androidenv = callPackage ../development/mobile/androidenv {
    pkgs_i686 = pkgsi686Linux;
};

I'm not sure what we gain from having the kernel in the name, when it's not there for callPackage_i686, stdenv_32bit etc. It should probably also have a similar predicate to stdenv_32bit. Perhaps we should even rename stdenv_32bit to match as well, and stick to a common suffix (_i686, -i686, _32bit, -32bit)?

nbp · 2017-04-10T15:30:56Z

Both names are coming from 2010. I think having Linux made sense because the system string is i686-linux. As you pointed out, this is not respected by other functions, so we might as well convert it to pkgs_i686, but I am afraid this might break more users than the NixOS argument. In which case we should probably have both aliases for a while and remove the old one after X releases.

corngood · 2017-04-10T20:52:27Z

I did a bit of renaming and removed the uses of the module param. I left the old names deprecated for the module param and stuff in all-packages, but the changes to the package params named pkgs_i686 would be breaking for overrides.

I went with lisp-case names since that seems to be the general trend.

  pkgs-i686 =
    if system == "x86_64-linux" then
      forceSystem "i686-linux" "i386"
    else
      self;

I based this on stdenv_32bit, but it seems a bit dirty. Maybe it should have a case for i686-linux -> self, and error otherwise? Is self even correct here?

nbp

These changes deserve an entry in the release notes, as well as trace messages to suggest users to follow the renaming which happens in these patches.

nbp · 2017-04-11T14:54:36Z

pkgs/top-level/all-packages.nix

+    if system == "x86_64-linux" then
+      forceSystem "i686-linux" "i386"
+    else
+      self;


I would just throw for cases which are not supported.

I've left i686-linux to point at itself, and made it throw otherwise.

nbp · 2017-04-11T14:55:38Z

pkgs/top-level/all-packages.nix

+
+  # DEPRECATED
+  pkgsi686Linux = pkgs-i686;
+  callPackage_i686 = callPackage-i686;


I am not sure about this naming convention, do we have any precedent / coding style suggesting we should go one way or the other? Any reason to rename it at all?

I don't have a particular desire to do these renames. I'm mostly worried about the packageOverrides fix.

I'm happy to remove the uses of the pkgs_i686 module argument, and add a trace warning for it as well, and we can leave it at that for now?

I'm happy to remove the uses of the pkgs_i686 module argument, and add a trace warning for it as well, and we can leave it at that for now?

Yes, with a release-note entry.

The nixpkgs release notes haven't been touched in a few years. Do you mean the nixos release notes for 17.09?

nbp · 2017-04-11T14:59:23Z

nixos/modules/misc/nixpkgs.nix

-    _module.args.pkgs = import ../../.. config.nixpkgs;
+    _module.args.pkgs = _pkgs;
+    # DEPRECATED
+    _module.args.pkgs-i686 = _pkgs.pkgs-i686;


Most people don't read the code, they copy working examples. I think this deserve a proper builtins.trace message when used, to at least warn the user that uses of this argument are deprecated.

I will also note that using a new name for a deprecated feature is weird. Rename it to pkgs_i686.

I dropped the change that deprecated this argument. It wasn't as simple as putting a trace here, because the attribute set seems to be evaluated and merged even if it's not used in any modules.

nbp · 2017-04-11T15:01:07Z

pkgs/top-level/all-packages.nix

@@ -498,7 +507,7 @@ with pkgs;
  });

  androidenv = callPackage ../development/mobile/androidenv {
-    pkgs_i686 = pkgsi686Linux;
+    pkgs-i686 = pkgs-i686;


Knowing that callPackage is used, this attribute can be removed, as it would be inherited from self anyway.

nbp · 2017-04-11T15:02:16Z

pkgs/top-level/all-packages.nix

@@ -552,7 +561,7 @@ with pkgs;
  xcodeenv = callPackage ../development/mobile/xcodeenv { };

  titaniumenv = callPackage ../development/mobile/titaniumenv {
-    pkgs_i686 = pkgsi686Linux;
+    pkgs-i686 = pkgs-i686;


Same here, remove this line.

corngood · 2017-04-20T02:32:41Z

I went back to the original change. The rename trace was getting fired of from various nix-env operations, and I think it was distracting from the purpose of this change anyway.

corngood · 2017-04-24T19:39:00Z

@vcunat @abbradar I'm interested in your feedback on this, since we've talked about gfx drivers in the past.

vcunat

It seems OK. I certainly agree with the spirit of the change. I didn't know this part was ignoring nixpkgs.config.

vcunat · 2017-04-26T06:59:50Z

Deprecation traces always had the problem with inability to differentiate between nix-env -qa and direct reference. There's still no way around that AFAIK; nix-env would probably have to get modified.

I think in this state it can go without an entry in release notes, as pkgs_i686 is only used on a few places and even there it would mean a change only in rather special circumstances (i.e. when you hit this bug).

abbradar

Seems a right thing to do for me. I agree with @vcunat in that perhaps we don't even need a release notes entry for this.

corngood · 2017-04-26T12:21:41Z

I'm happy to add a release note. Without any of the renaming it would just be something like "packageOverrides now applies to packages in pkgsLinuxi686 on 64-bit linux (pkgs_i686 in nixos modules)". I'll do that later today, but if you'd rather not have the note you can use what's here.

I could also open an issue to document the problem I was trying to fix, if that's preferable.

vcunat · 2017-04-26T16:24:21Z

I don't have any preference whether or not to add a release-note entry. I actually wonder whether to pick this to 17.03; I would personally be inclined to do so.

corngood force-pushed the pkgs_i686 branch from d26b0fd to 6ed63f5 Compare April 9, 2017 16:49

nbp suggested changes Apr 11, 2017

View reviewed changes

nbp added 0.kind: bug 0.kind: enhancement 9.needs: changelog labels Apr 11, 2017

corngood force-pushed the pkgs_i686 branch 3 times, most recently from 7feb1ed to b613ac8 Compare April 12, 2017 01:07

corngood force-pushed the pkgs_i686 branch from b613ac8 to 6945a47 Compare April 20, 2017 02:26

nixos: use pkgsi686Linux for pkgs_i686

2b6ee96

corngood force-pushed the pkgs_i686 branch from 6945a47 to 2b6ee96 Compare April 20, 2017 02:30

corngood changed the title ~~WIP: nixos: use pkgsi686Linux for pkgs_i686~~ nixos: use pkgsi686Linux for pkgs_i686 Apr 22, 2017

vcunat approved these changes Apr 26, 2017

View reviewed changes

abbradar approved these changes Apr 26, 2017

View reviewed changes

vcunat merged commit 7deb425 into NixOS:master Apr 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos: use pkgsi686Linux for pkgs_i686 #24772

nixos: use pkgsi686Linux for pkgs_i686 #24772

corngood commented Apr 9, 2017

mention-bot commented Apr 9, 2017

nbp commented Apr 10, 2017

corngood commented Apr 10, 2017

nbp commented Apr 10, 2017

corngood commented Apr 10, 2017

nbp left a comment

nbp Apr 11, 2017

corngood Apr 12, 2017

nbp Apr 11, 2017

corngood Apr 11, 2017

nbp Apr 11, 2017

corngood Apr 12, 2017

nbp Apr 11, 2017

corngood Apr 12, 2017

nbp Apr 11, 2017

nbp Apr 11, 2017

corngood commented Apr 20, 2017

corngood commented Apr 24, 2017

vcunat left a comment

vcunat commented Apr 26, 2017

abbradar left a comment •

edited

corngood commented Apr 26, 2017

vcunat commented Apr 26, 2017

nixos: use pkgsi686Linux for pkgs_i686 #24772

nixos: use pkgsi686Linux for pkgs_i686 #24772

Conversation

corngood commented Apr 9, 2017

Motivation for this change

Things done

mention-bot commented Apr 9, 2017

nbp commented Apr 10, 2017

corngood commented Apr 10, 2017

nbp commented Apr 10, 2017

corngood commented Apr 10, 2017

nbp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corngood commented Apr 20, 2017

corngood commented Apr 24, 2017

vcunat left a comment

Choose a reason for hiding this comment

vcunat commented Apr 26, 2017

abbradar left a comment • edited

Choose a reason for hiding this comment

corngood commented Apr 26, 2017

vcunat commented Apr 26, 2017

abbradar left a comment •

edited