-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
freebsd: various new packages to support a NixBSD system #320475
Conversation
18c069c
to
a413a20
Compare
fa97067
to
c5cac2d
Compare
@@ -15,4 +19,61 @@ | |||
.${stdenv'.hostPlatform.parsed.cpu.name} or stdenv'.hostPlatform.parsed.cpu.name; | |||
|
|||
install-wrapper = builtins.readFile ../../lib/install-wrapper.sh; | |||
|
|||
filterPatches = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too long. Can it be at least documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does need documentation, but do note it is just being moved. I would be OK with that happening in a later PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an overview.
platforms = lib.platforms.freebsd; | ||
license = | ||
lib.optional withAmd licenses.unfreeRedistributableFirmware | ||
# Intel license requires no modification, this will wrap firmware files in an ELF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ambiguous.
What does it mean?
a) does not require modification
b) prohibits any modification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at the actual license file and it seems that it's the latter. I updated the verbage to be more explicit.
useWrappedLogin ? true, | ||
}: | ||
mkDerivation { | ||
path = "libexec/getty"; | ||
|
||
postPatch = '' | ||
sed -E -i -e "s|/usr/bin/login|${ | ||
if useWrappedLogin then "/run/wrappers/bin/login" else "${login}/bin/login" | ||
}|g" $BSDSRCDIR/libexec/getty/*.h | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this wrapping stuff should be left for later? Not really sure what it is yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's wrong with it? we are in fact doing suid binary wrappers on nixbsd and they work fine. Would you rather it default to off? What's the practical use of running a non-suid login binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant that /run/wrappers
isn't a Nixpkgs thing, so it's arguably a minor layer violation I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the parameter to be the wrappedLogin path itself, null if unused, as we discussed on matrix. You should go complain to the other package maintainers for their packages that hardcode wrapper paths!
]; | ||
|
||
postPatch = '' | ||
sed -i 's/4555/0555/' $BSDSRCDIR/sbin/ping/Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it's removing a suid bit
sed -i 's|/usr/share/zfs|${zfs-data}/share/zfs|' $BSDSRCDIR/cddl/sbin/zpool/Makefile | ||
''; | ||
|
||
# I lied, this is both zpool and zfs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through each commit, but I don't wanna nit-pick this to death :)
5af68dd
to
a43ef1a
Compare
- the patched kernel source is useful for a few out-of-tree derivations, similar to how the linux kernel headers are useful for other builds. - the build did not work due to stack pointer, relocation, and Makefile shenannigans. Fix these.
Co-authored-by: Anderson Torres <torres.anderson.85@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not hold this any further, subsequent improvements and bugfixes can come in a later PRs.
Note it's a "syntax" error to try to dereference invalid attribute. Is it possible to have a check to return an exception here? (Inless it's a genuine bug) |
Description of changes
61 of them,1 small package diff, 1 large package diff (sys), and 1 commit modifying lib and mkDerivation. This lets us boot NixBSD!
There are a few TODOs left in the code but I think they're fine for followup PRs.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.