Skip to content

some overrideDerivation cleanups#41732

Merged
matthewbauer merged 1 commit intoNixOS:masterfrom
infinisil:overrideDerivation
Jun 23, 2018
Merged

some overrideDerivation cleanups#41732
matthewbauer merged 1 commit intoNixOS:masterfrom
infinisil:overrideDerivation

Conversation

@infinisil
Copy link
Member

These four top-level packages were the only ones that didn't have the
meta.position attribute automagically set. This commit fixes this.

Motivation for this change
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/)
  • Fits CONTRIBUTING.md.

@grahamc
Copy link
Member

grahamc commented Jun 9, 2018

@infinisil
Copy link
Member Author

Maybe he meant both overrideAttrs and overrideDerivation? No idea, but I won't let it worry me without an explanation :)

The docs certainly favor overrideAttrs: https://nixos.org/nixpkgs/manual/#sec-pkg-overrideDerivation

@GrahamcOfBorg GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jun 9, 2018
@dezgeg
Copy link
Contributor

dezgeg commented Jun 9, 2018

Yes, I believe this warning in the linked manual chapter applies to both:

Warning: Do not use this function in Nixpkgs as it evaluates a Derivation before modifying it, which breaks package abstraction and removes error-checking of function arguments. In addition, this evaluation-per-function application incurs a performance penalty, which can become a problem if many overrides are used. It is only intended for ad-hoc customisation, such as in ~/.config/nixpkgs/config.nix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to not introduce overrideAttrs here:

luajit_2_0 = generic {
     version = "2.0.5";
     isStable = true;
     sha256 = "0yg9q4q6v028bgh85317ykc9whgxgysp76qzaqgq55y6jy11yjw7";
     meta = meta // {
      platforms = lib.filter (p: p != "aarch64-linux") meta.platforms;
 };

generic = { ..., meta ? meta }:
 stdenv.mkDerivation rec {
      inherit name version src meta;
....
;

overrideAttrs doesn't make this generative stuff cleaner IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. @infinisil can you adjust it as shown by @danbst?

@danbst
Copy link
Contributor

danbst commented Jun 12, 2018

I see no harm in replacing lib.overrideDerivation to .overrideAttrs, that doesn't make situation "worse". The one that introduces can be rewritten to not introduce.

@FRidh FRidh requested a review from andir June 12, 2018 16:30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. @infinisil can you adjust it as shown by @danbst?

These four top-level packages were the only ones that didn't have the
meta.position attribute automagically set. This commit fixes this.
@infinisil infinisil force-pushed the overrideDerivation branch from fe55ce0 to 02f11bc Compare June 14, 2018 20:29
@infinisil
Copy link
Member Author

Fixed that, but I couldn't use meta as the attribute in the top level, because apparently { meta ? meta }: ... tries to use the arguments meta for the default value, didn't know that (and it doesn't make any sense for it to be that way)!

@danbst
Copy link
Contributor

danbst commented Jun 15, 2018

oh right, forgot func args are implicilty rec

@dezgeg
Copy link
Contributor

dezgeg commented Jun 15, 2018

{ meta ? meta }: is pretty nonsensical indeed, but having e.g. { stdenv, enableFoo ? stdenv.isLinux }: is pretty useful.

enableParallelBuilding = false;
}) // {

meta = with stdenv.lib; {
Copy link
Member

@matthewbauer matthewbauer Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think meta works with overrideAttrs. Can you confirm that the meta is still being applied? I thought you still had to do a union like with overrideDerivation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does indeed work, the main reason I'm doing this PR is to have meta.position available. All other meta attributes work as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok looks good then.

@matthewbauer matthewbauer merged commit a006243 into NixOS:master Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants