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

system76-dkms: 1.0.13 -> 1.0.16 #349177

Merged
merged 2 commits into from
Oct 30, 2024
Merged

system76-dkms: 1.0.13 -> 1.0.16 #349177

merged 2 commits into from
Oct 30, 2024

Conversation

khumba
Copy link
Contributor

@khumba khumba commented Oct 17, 2024

The latest release adds a small compatibility fix needed for building against Linux 6.11 (which I've confirmed builds okay now).

Requested by: #348615

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@khumba khumba requested a review from Mic92 October 17, 2024 03:08
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Oct 17, 2024
@khumba
Copy link
Contributor Author

khumba commented Oct 17, 2024

Hi @Mic92, you reviewed this before so I hope you don't mind me pinging you again. I'm not sure what's up with the Codeowners check failure, but it looks transient or unrelated.

@sjmonson
Copy link
Contributor

@ahoneybun

@ahoneybun
Copy link
Contributor

@ahoneybun

if you have testing method I'd be happy to test this once I get my system76-power build issue fixed.

@khumba
Copy link
Contributor Author

khumba commented Oct 18, 2024

Hi @ahoneybun, great to see you here. The way I would test this is:

  1. Clone nixpkgs.git locally, and either check out the exact commit my system is already at, or just the nixos-YY.MM or nixos-unstable channel branch[1] matching my system.
  2. Cherry pick this commit.
  3. Run nixos-rebuild boot -Q -I nixpkgs=/path/to/local/nixpkgs.git, confirm that the updated module is built in the output, then reboot into the new system.

Thanks in advance for testing this. The nixpkgs-review run confirmed that it builds against the latest 6.11 kernel as well as older ones, but I haven't booted into it yet on NixOS ($otherDistros went fine).

On that note: would you be interested in taking over for me maintaining these three kernel modules in Nixpkgs? Once this is merged, I am planning to orphan of all of my packages in Nixpkgs except for nvd, as my involvement with Nix has been basically limited to just nvd for quite a while. Well, that plus a legacy Nix system at work :).

[1] https://wiki.nixos.org/wiki/Channel_branches

@khumba
Copy link
Contributor Author

khumba commented Oct 18, 2024

(That's assuming you're not using flakes, if you are then you might need a different approach.)

@ahoneybun
Copy link
Contributor

(That's assuming you're not using flakes, if you are then you might need a different approach.)

From my understanding I am using Flakes:

https://gitlab.com/ahoneybun/nix-configs/-/blob/main/flake.nix?ref_type=heads

but I have a general configuration.nix file:

https://gitlab.com/ahoneybun/nix-configs/-/blob/main/configuration.nix?ref_type=heads

@ahoneybun
Copy link
Contributor

Hi @ahoneybun, great to see you here. The way I would test this is:

1. Clone nixpkgs.git locally, and either check out the exact commit my system is already at, or just the `nixos-YY.MM` or `nixos-unstable` channel branch[1] matching my system.

2. Cherry pick this commit.

3. Run `nixos-rebuild boot -Q -I nixpkgs=/path/to/local/nixpkgs.git`, confirm that the updated module is built in the output, then reboot into the new system.

Thanks in advance for testing this. The nixpkgs-review run confirmed that it builds against the latest 6.11 kernel as well as older ones, but I haven't booted into it yet on NixOS ($otherDistros went fine).

On that note: would you be interested in taking over for me maintaining these three kernel modules in Nixpkgs? Once this is merged, I am planning to orphan of all of my packages in Nixpkgs except for nvd, as my involvement with Nix has been basically limited to just nvd for quite a while. Well, that plus a legacy Nix system at work :).

[1] https://wiki.nixos.org/wiki/Channel_branches

I may be willing to if you stay around for some questions as I have just been maintaining a few COSMIC packages but no modules.

@khumba
Copy link
Contributor Author

khumba commented Oct 18, 2024

I may be willing to if you stay around for some questions as I have just been maintaining a few COSMIC packages but no modules.

I'm happy to share any knowledge I have.

From my understanding I am using Flakes:

I think you could set flake.nix's inputs.nixpkgs.url to your local nixpkgs path then, either as a literal path or a "path:..." string, if I'm reading the manual correctly:

https://nix.dev/manual/nix/2.18/command-ref/new-cli/nix3-flake.html#path-like-syntax

@ahoneybun
Copy link
Contributor

ahoneybun commented Oct 18, 2024

I may be willing to if you stay around for some questions as I have just been maintaining a few COSMIC packages but no modules.

I'm happy to share any knowledge I have.

From my understanding I am using Flakes:

I think you could set flake.nix's inputs.nixpkgs.url to your local nixpkgs path then, either as a literal path or a "path:..." string, if I'm reading the manual correctly:

https://nix.dev/manual/nix/2.18/command-ref/new-cli/nix3-flake.html#path-like-syntax

Thank you very much and I look forward to getting system76-io-dkms updated as well:

#349436

I think that's similar to how I tested my nixos-hardware PR for the system76 galp5-1650. I most likely will just do a normal NixOS install using their official installer to make it easier to test.

@nothingmuch
Copy link

I think you could set flake.nix's inputs.nixpkgs.url to your local nixpkgs path then

To avoid changing a flake based config you can also use --override-input, in this example for the branch directly but of course this could also be a local working tree with a cherry picked commit:

nix build /etc/nixos#nixosConfigurations.$(hostname -s).config.system.build.toplevel --no-write-lock-file --override-input nixpkgs github:NixOS/nixpkgs/pull/349177/head

FWIW this built for me with hardware.system76.enableAll = true, whereas if nixpkgs is master or unstable as of yesterday it does not, but I haven't yet tested activating or booting the built profile

@ahoneybun
Copy link
Contributor

I think you could set flake.nix's inputs.nixpkgs.url to your local nixpkgs path then

To avoid changing a flake based config you can also use --override-input, in this example for the branch directly but of course this could also be a local working tree with a cherry picked commit:

nix build /etc/nixos#nixosConfigurations.$(hostname -s).config.system.build.toplevel --no-write-lock-file --override-input nixpkgs github:NixOS/nixpkgs/pull/349177/head

FWIW this built for me with hardware.system76.enableAll = true, whereas if nixpkgs is master or unstable as of yesterday it does not, but I haven't yet tested activating or booting the built profile

I think it may be fairly important to have that enabled since the whole point of this is for a system76 Thelio desktop.

@nothingmuch
Copy link

nothingmuch commented Oct 22, 2024

I think it may be fairly important to have that enabled since the whole point of this is for a system76 Thelio desktop.

I need it for my laptop as well, my machine configuration hasn't been building until this change due to that being enabled. Unfortunately rebooting right now to fully test is problematic for me, probably not for a few hours, but I wanted to share the tip about --override-input so I posted prematurely sorry

@ahoneybun
Copy link
Contributor

I think it may be fairly important to have that enabled since the whole point of this is for a system76 Thelio desktop.

I need it for my laptop as well, my machine configuration hasn't been building until this change due to that being enabled. Unfortunately rebooting right now to fully test, probably not for a few hours, but I wanted to share the tip about --override-input so I posted prematurely sorry

Ah sorry this is for the dkms not the io-dkms which is Thelio only.

@ahoneybun
Copy link
Contributor

It would be really nice to get this in soon as folks are stuck on the 6.10 or below kernel until this is merged.

@nothingmuch
Copy link

sorry for the delay, confirmed working on lemp12 laptop

@ahoneybun
Copy link
Contributor

It looks like the 6.10 is now EOL so I had to drop down to the 6.6 kernel.

@ahoneybun ahoneybun self-requested a review October 28, 2024 14:58
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 29, 2024
@nickgerace
Copy link
Contributor

Thank you @khumba for the PR! Looks good to me.

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Oct 29, 2024
@nyabinary
Copy link
Contributor

Can you copy the suggestions I gave here #349436 (review) to this PR as well?

The latest release adds a small compatibility fix needed for Linux 6.11.
@khumba
Copy link
Contributor Author

khumba commented Oct 30, 2024

Thanks for the reviews everyone.

Can you copy the suggestions I gave here #349436 (review) to this PR as well?

Sure, done.

@Mic92 Mic92 merged commit 990f185 into NixOS:master Oct 30, 2024
24 checks passed
@khumba khumba deleted the system76-dkms branch October 30, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants