Skip to content

nixos.system.boot.kernel: stronger constraints for kernelParam type#133625

Merged
nrdxp merged 1 commit intoNixOS:masterfrom
cab404:kernel-params-checking
Oct 15, 2021
Merged

nixos.system.boot.kernel: stronger constraints for kernelParam type#133625
nrdxp merged 1 commit intoNixOS:masterfrom
cab404:kernel-params-checking

Conversation

@cab404
Copy link
Member

@cab404 cab404 commented Aug 12, 2021 •
edited
Loading

Checks whether all spaces are inside double quotes, thus ensuring that one
string parses as no more than one kernel param.

Motivation for this change

Users often add spaces in expressions like "a = 12", and having them parsed as 3 separate params is not user friendly.

Things done

kernelParams type changed to a string match.

  • 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 packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Aug 12, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 12, 2021
pennae
pennae reviewed Aug 12, 2021
View reviewed changes
@cab404 cab404 force-pushed the kernel-params-checking branch from 74bf6fb to 4cab2cd Compare August 12, 2021 23:49
@cab404 cab404 marked this pull request as ready for review August 15, 2021 00:06
@cab404
Copy link
Member Author

cab404 commented Aug 15, 2021

It actually contains some breaking changes (e.g some of the old configs won't build), so I will need to update release notes.

@cab404 cab404 force-pushed the kernel-params-checking branch from 4cab2cd to 98034b5 Compare August 15, 2021 07:03
@github-actions github-actions bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Aug 15, 2021
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 15, 2021
@cab404 cab404 force-pushed the kernel-params-checking branch 2 times, most recently from 7ee3129 to e3f05e1 Compare August 15, 2021 07:39
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 15, 2021
@cab404 cab404 force-pushed the kernel-params-checking branch 2 times, most recently from 636eedf to 5126497 Compare August 15, 2021 08:52
@nixos-discourse
Copy link

nixos-discourse commented Aug 15, 2021

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/575

@nixos-discourse
Copy link

nixos-discourse commented Sep 14, 2021

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/403

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 16, 2021
@cab404 cab404 mentioned this pull request Oct 6, 2021
Closed
Artturin
Artturin reviewed Oct 6, 2021
View reviewed changes
nixos/doc/manual/release-notes/rl-2111.section.md Outdated
Copy link
Member

@Artturin Artturin Oct 6, 2021

Choose a reason for hiding this comment

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

You forgot this

Copy link
Member Author

@cab404 cab404 Oct 6, 2021

Choose a reason for hiding this comment

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

wasn't me, look at the base commit)

i'm going to rebase that anyway if someone drops by and tells that "yeah, just fix the conflicts and good to go"

Copy link
Member Author

@cab404 cab404 Oct 6, 2021

Choose a reason for hiding this comment

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

@tomberek @SuperSandro2000 btw — should I rebase so you can merge, or I should wait further review?

Copy link
Member

@SuperSandro2000 SuperSandro2000 Oct 6, 2021

Choose a reason for hiding this comment

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

fixing merge conflicts is always good but I don't really can say something about the change

nrdxp
nrdxp reviewed Oct 15, 2021
View reviewed changes
pennae
pennae reviewed Oct 15, 2021
View reviewed changes
nrdxp
nrdxp approved these changes Oct 15, 2021
View reviewed changes
Copy link

@nrdxp nrdxp left a comment

Choose a reason for hiding this comment

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

Seems to work as intended when evaluating a system from this branch of nixpkgs. Just rebase and you've got my approval to merge 👍

@cab404 @pennae
nixos.system.boot.kernel: stronger constraints for kernelParam type
a868222 
Checks whether all spaces are inside double quotes, thus ensuring that one
string parses as no more than one kernel param.

Co-authored-by: pennae <82953136+pennae@users.noreply.github.com>
@cab404 cab404 force-pushed the kernel-params-checking branch from 21ae16c to a868222 Compare October 15, 2021 22:21
@github-actions github-actions bot removed the 8.has: documentation This PR adds or changes documentation label Oct 15, 2021
@github-actions github-actions bot removed the 8.has: changelog This PR adds or changes release notes label Oct 15, 2021
@cab404
Copy link
Member Author

cab404 commented Oct 15, 2021

@nrdxp waaaait, docs not updated

@nrdxp
Copy link

nrdxp commented Oct 15, 2021

There is no demerge button unfortunately, but I'll gladly merge a doc PR 😅
Sorry, I guess I got a little antsy 😓

@cab404 cab404 mentioned this pull request Oct 15, 2021
Merged
12 tasks
@cab404
Copy link
Member Author

cab404 commented Oct 15, 2021 •
edited
Loading

There is no demerge button unfortunately, but I'll gladly merge a doc PR

#141818 ❤️

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Oct 18, 2021

There is no demerge button unfortunately

There is, you can revert the merge commit.

@samhug samhug mentioned this pull request Oct 18, 2021
Merged
12 tasks
@Artturin Artturin mentioned this pull request Dec 12, 2021
Open
@rnhmjoj rnhmjoj mentioned this pull request Jan 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@Artturin Artturin Artturin left review comments

@pennae pennae pennae approved these changes

@tomberek tomberek tomberek approved these changes

+1 more reviewer

@nrdxp nrdxp nrdxp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cab404 @nixos-discourse @nrdxp @SuperSandro2000 @tomberek @Artturin @pennae