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

Fix kernel configuration merge #84032

Merged
merged 3 commits into from May 22, 2020
Merged

Fix kernel configuration merge #84032

merged 3 commits into from May 22, 2020

Conversation

teto
Copy link
Member

@teto teto commented Apr 1, 2020

Motivation for this change
Things done

Fix #71803

Was a bit tougher than expected because I misunderstood how mkMerge worked. I thought using submodule would create the missing fields in attribute sets but it doesn't.
The fix made apparent that some configuration entries in common-config.nix are unused.
As it may depend on kernel version, I marked the entries optional. It felt less likely to break builds than plainly removing them (but removing them could be the correct thing to do).

I improved the tests and made them more readable.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

teto added 2 commits April 1, 2020 22:25
Addresses NixOS#71803:
Kernel options are not merged as described, especially the "optional"
aspects. The error silences legitimate warnings.
With the fix in kernel configuration merging, some kernel configuration items
marked as mandatory now correctly trigger an error when unused (while they
previously were unused).
@flokli
Copy link
Contributor

flokli commented Apr 2, 2020

should this target staging?

@teto teto changed the title Fix kernel merge Fix kernel configuration merge Apr 2, 2020
@teto teto changed the base branch from master to staging April 2, 2020 13:57
@teto
Copy link
Member Author

teto commented Apr 2, 2020

done

@FRidh FRidh added this to WIP in Staging via automation Apr 3, 2020
@eadwu
Copy link
Member

eadwu commented Apr 11, 2020

This doesn't seem to fix INET_TCP_DIAG? For me INET_TCP_DIAG? m is set which should be correct since it is now an option, but I'm still getting warning: option not set correctly: INET_TCP_DIAG (wanted 'm', got 'y') still. Or is it expected to be like that?

@teto
Copy link
Member Author

teto commented Apr 11, 2020

The warning for INET_TCP_DIAG already occurs on nixos-unstable. The way I see it, it should have triggered an error (and now would thanks to this PR, I changed the config to INET_TCP_DIAG = option module; to keep it working).
The config INET_TCP_DIAG = module; fails because the kernel never asks for the setting and defaults to yes (probably because INET_DIAG = yes; and some TCP setting is builtin).
I've removed the entry in pkgs/os-specific/linux/kernel/common-config.nix due to your comment since it was not respected anyway. If someone really wants INET_TCP_DIAG as a module, He or I can tweak it (cc @ivan from #69388 ).

@teto
Copy link
Member Author

teto commented May 17, 2020

would like to merge this since it fixes a non-obvious bug CC @NeQuissimus

@flokli flokli merged commit cfb4d0d into NixOS:staging May 22, 2020
Staging automation moved this from WIP to Done May 22, 2020
@teto teto deleted the fix_kernel_merge branch May 22, 2020 12:41
@wizeman wizeman mentioned this pull request May 26, 2020
10 tasks
wizeman added a commit to wizeman/nixpkgs that referenced this pull request Jun 10, 2020
Some of the options didn't have correct kernel version constraints,
others had been removed or made optional unnecessarily in NixOS#84032.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

wrong merging of kernel "optional" setting
3 participants