NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.45k stars 13.65k forks source link

Bcachefs patches no longer apply on linuxPackages_latest (6.1.6+) #212086

Closed jansol closed 1 year ago

jansol commented 1 year ago

Steps To Reproduce

Steps to reproduce the behavior:

  1. add bcachefs to boot.supportedFilesystems

Build log

building '/nix/store/25h43lfhj7rl4b03m070jkzf9m4189vy-linux-config-6.1.7-bcachefs-unstable-2022-12-29.drv'...
unpacking sources
unpacking source archive /nix/store/jwgfb0mwqdvqpvhy9p8l5csvp4p1kbz0-linux-6.1.7.tar.xz
source root is linux-6.1.7
setting SOURCE_DATE_EPOCH to timestamp 1674039514 of file linux-6.1.7/virt/lib/irqbypass.c
patching sources
applying patch /nix/store/hxqx22d2acr51drgh1gb4i3kfk256lkc-bcachefs-8f064a4cb5c7cce289b83d7a459e6d8620188b37.diff
<snip>
patching file kernel/trace/trace.c
Hunk #1 succeeded at 1680 (offset 1 line).
Hunk #2 succeeded at 3753 (offset 1 line).
Hunk #3 FAILED at 6788.
Hunk #4 succeeded at 6829 (offset 14 lines).
Hunk #5 succeeded at 6855 (offset 14 lines).
Hunk #6 succeeded at 6873 (offset 14 lines).
Hunk #7 succeeded at 9916 (offset 16 lines).
1 out of 7 hunks FAILED -- saving rejects to file kernel/trace/trace.c.rej

Notify maintainers

@davidak @Madouura @Mic92

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 6.1.7, NixOS, 23.05 (Stoat), 23.05pre445203.5ed48194335`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.12.0`
 - channels(root): `"nixos"`
 - channels(user): `""`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`
Mic92 commented 1 year ago

I reverted two linux kernel bumps in my nixpkgs to fix this: https://github.com/Mic92/nixpkgs/commits/main

Madouura commented 1 year ago

Likely needs an update. Current bcachefs is on 6.1.

Madouura commented 1 year ago

Looks like it's a bit more complicated than that. If it's anything like last time with printBuf, it'll be fixed upstream at some point. Until then, IIRC there's not much I can do without completely changing the derivation again.

Mic92 commented 1 year ago

I am now actually back at no longer applying kernel patches but just downloading the original kernel source. I proposed this in the past, but the other maintainer did not like it: https://github.com/Mic92/nixpkgs/commit/51887ee48cf5720276af99d65dac2e862e6b4a1e As I no longer use the way it's handled in nixpkgs, please don't ping me in the future on this particular package.

davidak commented 1 year ago

I am now actually back at no longer applying kernel patches but just downloading the original kernel source.

I proposed this in the past, but the other maintainer did not like it

i objected the change to patch approach previously (https://github.com/NixOS/nixpkgs/issues/121767#issuecomment-833002911)

this would break it in unexpected ways

what now happened

Until then, IIRC there's not much I can do without completely changing the derivation again.

that's a bad experience for the users

when just packaging upstream code, you can just revert the latest changes, so it always works for users

maybe move back to that approach? i don't think it has bigger downsides than this situation

cc @hyperfekt

ziguana commented 1 year ago

just ran into this while trying bcachefs on a new machine. does this happen often? looking at the derivation, i don't see how it wouldn't.

hyperfekt commented 1 year ago

It doesn't happen very often but clearly often enough to pose a real problem. The correct solution here would've been to switch this package to use the latest working minor instead of the main package when that was updated instead of just letting it break (which is not very difficult as it is an argument passed in linux-kernels.nix). If the package is changed to use an upstream tarball I still argue that it should not be pulled in automatically by adding bcachefs to boot.supportedFilesysyems because NixOS does not have a policy of shipping its users known-vulnerable software in an obscured manner.

Mic92 commented 1 year ago

I am now maintaining a functional package for bcachefs in disko, where I need to maintain it for testing anyway. Here is a usage example: https://github.com/Mic92/dotfiles/blob/a98d31df9a408d191bcdcd2b9260b9b9e232ed29/nixos/flake-module.nix#L165

superherointj commented 1 year ago

While there isn't a conclusion on how to handle bcachefs, can it be marked broken?

Mic92 commented 1 year ago

I think it might cause troubles with the nixos module and I would like to keep the nixos module

superherointj commented 1 year ago

I think it might cause troubles with the nixos module and I would like to keep the nixos module

There is no way to workaround this? Maybe override package to not broken?

I ask because bcachefs lists on many builds in nixpkgs-review. It is always popping up. Just happened now again: image

PedroHLC commented 1 year ago

If the latest commit in Kent's master simply works, why do we double the effort by extracting a diff/patch from it and applying to upstream? If we simply use the developer's git:

  1. It works out-of-the-box;
  2. We already have to manually bump the commit and kernel major version – we could also manually bump the full version – I could do it, I already manually bump linux-lqx and linux-zen;
  3. It's guaranteed to keep working, tested by Kent himself – this issue wouldn't reappear;
  4. We wouldn't reinvent the wheel, we already do this for zen-kernels (src is totally replaced by a new fetchFromGitHub)

We would, of course, lose the mirrors to the source (when using evilpiepirate.org), for solving this we could use GitHub that at least have a CDN (rn it is currently correctly mirrored).

hyperfekt commented 1 year ago

It used to work that way and it's pretty easy to do but the bcachefs kernel isn't maintained the same way zen and lqx are, it doesn't get any patch updates after the .0 minor release, so any non-bcachefs issues are not patched until the next minor version.

PedroHLC commented 1 year ago

It used to work that way and it's pretty easy to do but the bcachefs kernel isn't maintained the same way zen and lqx are, it doesn't get any patch updates after the .0 minor release, so any non-bcachefs issues are not patched until the next minor version.

Let's pin it, and every time I bump the "patch" in zen, I bump it here too. If it does not build, then I leave it stuck until someone has patches or the "minor" bumps again.

davidak commented 1 year ago

It used to work that way and it's pretty easy to do but the bcachefs kernel isn't maintained the same way zen and lqx are, it doesn't get any patch updates after the .0 minor release, so any non-bcachefs issues are not patched until the next minor version.

I see your point, but for me the point of this kernel package is to test bcachefs. If you need something unrelated to work, use the regular kernel!

hyperfekt commented 1 year ago

I don't need anything to work. My insistence isn't for my sake, as I currently don't even use the bcachefs kernel. I feel that we're talking in circles... I just don't want people to get tripped up by this.

davidak commented 1 year ago

@hyperfekt i mean "you" as in "the person who uses it".

We see we can't have both: working bcachefs and it using the latest upstream linux version. So we have to prioritize one.

And i argue to focus on a working bcachefs package even when it does not get the latest linux patches. That should be fine, since it's marked as testing.

hyperfekt commented 1 year ago

Oh, I think I got it. We could use Kent's tree and put it behind the insecure flag, just like Python 2.

davidak commented 1 year ago

This is now fixed by https://github.com/NixOS/nixpkgs/pull/214265. You can see here if it already is in the unstable channel: https://nixpk.gs/pr-tracker.html?pr=214265

@hyperfekt marking an insecure package as such sounds reasonable. Not sure if kernel packages are somehow special here. Do you want to create a PR? (or someone else?)