refactor: remove flake-utils, various other nit picks#9
refactor: remove flake-utils, various other nit picks#9LordGrimmauld merged 10 commits intoLordGrimmauld:mainfrom
Conversation
|
since the flake is already getting reworked - is there a need for the rust-overlay ? I don't see any unstable features or anything unusual that would prevent nixpkgs rustc from compiling this, and removing it would save a decent amount of work |
|
since the stable toolchain is being used, i don't see a need for rust-overlay. what do you think Grimmauld? |
|
I found things like clippy and other nice rust lints are more stable in a devshell with rust-overlay, but am not entirely opposed to dropping that in favor of something cached on https://cache.nixos.org. |
cool, i'll add that too
actually i got ratio'd by misuing |
|
I'll have to go through these with some care. I've been copying my standard rust template around, and i wrote that ~2 years ago when i knew less nix and less rust. There is some good ideas in here for sure! |
|
@LordGrimmauld reminder ping :D |
LordGrimmauld
left a comment
LordGrimmauld
left a comment
There was a problem hiding this comment.
Built on x86_64-linux and aarch64-linux (both native, i'll have to figure out cross flakes some other day).
Tested checks on both platforms, tested dev shell on x86_64.
Overall much needed changes, thank you for doing this! There is a couple details that still look a little silly, but nothing i see the need to block for and much better than it was.
Sorry for the delay getting to review this, and thank you for your patience.
| "x86_64-linux" | ||
| "x86_64-darwin" | ||
| "aarch64-linux" | ||
| "aarch64-darwin" |
There was a problem hiding this comment.
Imo darwin is silly here. run0 as part of systemd will never run on darwin. I am aware this is a 1:1 replacement of what flakeutils did. For dev shell and formatting and such, darwin makes sense. For the package, not so much. Not a big issue, you don't have to fix everything that was bad before all at once, i won't block on this. Just noticed because it is now spelled out.
| { | ||
| lib, | ||
| rustPlatform, | ||
| ... |
There was a problem hiding this comment.
| ... |
Not a functional change, so won't block, but it is common for nixpkgs packages to not allow wildcard arguments and i like to follow those standards at least a little.
There was a problem hiding this comment.
You're 100% correct, just a habit from the module system to add , ...
| inherit (nixpkgs) lib; | ||
| cargo-toml = (lib.importTOML ./Cargo.toml).package; |
There was a problem hiding this comment.
Does the use of lib here cause eval slowdown? We are later loading nixpkgs.legacyPackages.${system}.lib, and importing lib twice may have some costs. If it is the same lib, then eval may be cached though, i am not quite familiar enough with evaluator internals to judge this correctly.
| if (action.id == "org.freedesktop.policykit.exec" || | ||
| action.id.indexOf("org.freedesktop.systemd1.") { | ||
| return polkit.Result.AUTH_ADMIN_KEEP; |
There was a problem hiding this comment.
I missed this during review, i really need to put polkit stuff on the test path.
| if (action.id == "org.freedesktop.policykit.exec" || | |
| action.id.indexOf("org.freedesktop.systemd1.") { | |
| return polkit.Result.AUTH_ADMIN_KEEP; | |
| if (action.id == "org.freedesktop.policykit.exec" || | |
| action.id.indexOf("org.freedesktop.systemd1.") == 0) { | |
| return polkit.Result.AUTH_ADMIN_KEEP; |
We are lucky the ) was missing, crashing the entire rule and triggering polkits fail-closed. This was noticed and responsibly reported by @zimward, who has been amazing, even providing a reproduction in a VM (which should have been part of the test suite to begin with).
Had ) been here, but just == 0 missing, chances are it'd have taken significantly longer to spot this issue, and would have been an active security risk (early return of AUTH_ADMIN_KEEP even for non-admin stuff and things that shouldn't be kept - particularly, anything except org.freedesktop.systemd1.*.
This has been fixed as of ef368ac. Considering this was a fail-closed issue, i do not believe we need to file for a github security advisory - i do want to document what went wrong though.
There was a problem hiding this comment.
I'm sorry. major skill issue on my part.
I don't even know how it happened because I copy-pasted that bit from my config. which I just checked and it's correct there
3 participants
Sorry if anything of this is too pedantic but i gave your flake a once over and this was all the low-hanging fruit i saw.
tried to split it up into separate enough commits to make it somewhat reviewable