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

Optionalize more kernel features #19194

Closed

Conversation

aneeshusa
Copy link
Contributor

Motivation for this change

Make it possible to disable some kernel features which are not required for NixOS.
Hopefully by having smaller commits it is more clear that these features can be disabled safely (compare with #18058). I have been running this configuration without any issues.

If this is accepted, expect more in the same vein.

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
    • OS X
    • 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.

cc @spacekitteh, who is likely interested in this

@mention-bot
Copy link

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

@copumpkin
Copy link
Member

Your message doesn't address the concerns people raised on the original. Putting question marks doesn't make it easier for a user to disable certain features, right?

@aneeshusa
Copy link
Contributor Author

Currently, we don't have a structured kernel config so extraConfig must be used to disable features. However, when I add SOUND n for example to extraConfig, it will error out at build time, because the config options without questions marks (e.g. SND_DYNAMIC_MINORS) unconditionally select SOUND. Adding question marks turns them into recommendations, so that if SOUND is enabled they will also be set, but preventing an error if the user tries to disable SOUND.

I've scoped this PR to features which are clearly not required to run NixOS, so making them recommendations instead of required lets me disable them in my kernel, making it more customizable. A structured config would be better, but this is still useful in the short term.

@copumpkin
Copy link
Member

Oh, I see. I'd personally rather someone keep pushing #12158 than do it this way, but I'm obviously biased. I've had very little time for Nix stuff recently, but putting effort into that space feels more useful long-term than adding question marks (not to demean your particular question marks, but text munging is always going to be painful)

@aneeshusa
Copy link
Contributor Author

I definitely want to see #12158 as well, but it doesn't seem like it's going to be in a mergeable place soon, and I'd rather not let the perfect be the enemy of the good here.
Text munging is never fun, but it's working today.

@copumpkin
Copy link
Member

Sure, I agree with that in principle, but I'll let others who actually deal with the kernel expression weigh in on whether this is a good maintainability trade-off. Maybe one day I'll finish that PR 😄

@edolstra
Copy link
Member

edolstra commented Oct 4, 2016

If we're going to add ? to (nearly) every kernel option, we may as well either a) disable the check that options are used, and remove all the ?; or b) add an option to disable that check for people who want to override the default config.

@aneeshusa
Copy link
Contributor Author

For a), I think it is still a good idea to make it impossible to disable any options that are required for NixOS (there's way too many kernel options for me at least to know what they all do or if they're required). It does seem like many options are recommendations instead of requirements though, so maybe default to being optional, and have something to mark (maybe the suffix !) required options (which are the only ones checked)?

I don't know which options are actually required though, so I couldn't implement that.

For b), I think it's already possible to use the configfile option with buildLinux directly if you want full control of the generated config, so I don't think another option to disable checks is useful.

Alternately I saw @cstrahan assigned himself to #12158, so maybe that will happen sooner rather than later.

@cstrahan
Copy link
Contributor

Alternately I saw @cstrahan assigned himself to #12158, so maybe that will happen sooner rather than later.

Dunno when I might get to it, though...

aneeshusa referenced this pull request Oct 19, 2016
Enable encryption support for both F2FS and ext4. For ext4 this is a bit
tricky, since pre-4.8 the way to enable it as a module was just
"EXT4_ENCRYPTION=m" but after that it changed to "FS_ENCRYPTION=m &&
EXT4_ENCRYPTION=y".

Also make sure UDF is enabled.
@spacekitteh
Copy link
Contributor

@grahamc please tag as security, enhancement

@bobvanderlinden
Copy link
Member

This is quite an old PR and a lot has changed. Is this PR still relevant?

@teto
Copy link
Member

teto commented Apr 14, 2019

Might be related #55755

@teto
Copy link
Member

teto commented Jul 10, 2019

with the structured config, it's possible to disable specific items or make them optional. Closing.

@teto teto closed this Jul 10, 2019
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

10 participants