Skip to content
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

linux_mptcp: makes linux_mptcp.override works #31596

Merged
merged 1 commit into from Nov 13, 2017
Merged

Conversation

teto
Copy link
Member

@teto teto commented Nov 13, 2017

I needed to override some parameters because of an error I had:
"Error: modDirVersion specified in the Nix expression is wrong, it should be: 4.9.60+"

but the following override would not be taken into account

  pkg.override ({
    modDirVersion="4.9.60+";
    src=pkgs.lib.cleanSource /home/teto/mptcp;
  })

because the override would be overriden by the nixpkgs parameters
because of concatenation order (https://nixos.org/nix/manual/#sec-language-operators); it was args // hardcoded and I reversed it to hardcoded // args

Motivation for this change
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
    • 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.

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 13, 2017

@layus for review

@globin globin merged commit 3873738 into NixOS:master Nov 13, 2017
@teto teto deleted the mptcp_override branch November 13, 2017 10:19
Copy link
Member

@layus layus left a comment

Choose a reason for hiding this comment

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

@globin I think this should be reverted. There is already a way to implement the required task without changing nixpkgs. If we really want this change to be merged, we should apply it to all the kernels in nixpkgs, not only this one.

@@ -43,4 +43,4 @@ import ./generic.nix (args // rec {
TCP_CONG_BALIA m

'' + (args.extraConfig or "");
} // (args.argsOverride or {}))
} // args // (args.argsOverride or {}))
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this code was designed to use argsOverride to do what you are doing, not args directly.
In this case, your override should look like

  pkg.override ({
    argsOverride = {
      modDirVersion="4.9.60+";
      src=pkgs.lib.cleanSource /home/teto/mptcp;
    };
  })

Now, this may not be the most obvious way to do it. However, I would prefer to keep this consistent with the other kernel implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw the args.argsOverride afterwards. Any technical reason for args.argsOverride or is it legacy ? seeing that there is no specific documentation for kernels, people are bound to look at the nix code either way or use override out of habit.
Btw I would be interested in your opinon over #31463 :)

Copy link
Member

Choose a reason for hiding this comment

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

Really no idea... You could get more info by blame'ing on original kernels.

Copy link
Member Author

Choose a reason for hiding this comment

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

well so far it works but I had lots of trouble compiling the kernel for many reasons; i'll try to send some doc for the manual.

I needed to override some parameters because of an error I had:
"Error: modDirVersion specified in the Nix expression is wrong, it should be: 4.9.60+"

but the following override would not be taken into account
  pkg.override ({
    modDirVersion="4.9.60+";
    src=pkgs.lib.cleanSource /home/teto/mptcp;
  })

because the override would be overriden by the nixpkgs parameters
because of concatenation order:
https://nixos.org/nix/manual/#sec-language-operators
layus added a commit to layus/nixpkgs that referenced this pull request Nov 24, 2017
argsOverride were introduced in 31fa2cd (NixOS#1654).
It seems simpler to just override the default values with the attrs themselves.
It also makes it more intuitive to use. (See NixOS#31596).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants