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

sd-image: remove implicit profiles/all-hardware #128532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arcnmx
Copy link
Member

@arcnmx arcnmx commented Jun 28, 2021

Motivation for this change

Allows SD images to be created for systems not compatible with the all-hardware profile.

Things done
  • 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 pkgs 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 Relase 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 Jun 28, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 28, 2021
@@ -1,6 +1,7 @@
{ config, ... }:
{
imports = [
../../profiles/all-hardware.nix
Copy link
Member

Choose a reason for hiding this comment

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

Why is this added to this "alias-like" file?

Copy link
Member Author

Choose a reason for hiding this comment

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

No good reason, it just felt bad to not limit the impact of this change.

@samueldr
Copy link
Member

samueldr commented Jun 30, 2021

Can you describe what is "not compatible" with the all-hardware profile?

Is it something else than kernel modules causing issues? Am I right to assume further fallout from #78430? See also #109280 for additional discussion.

@arcnmx
Copy link
Member Author

arcnmx commented Jun 30, 2021

Can you describe what is "not compatible" with the all-hardware profile?

To be fair for the most part it was... the fact that I had a config/closure, and only wanted to pull in the SD image generation options to build an image of the system. It felt rather presumptuous of it to set a bunch of options as if I wanted to generate some bloated "installer" image. It certainly was not my intention to create a system that could even remotely be described as "all-hardware" just because I wanted to place it on an SD card?

... but maybe that's my fault for trying to use a module under the "installer/" subdir and the real solution here is to move it to a different name/path without the same connotations? It's generically useful for a lot more than just building an installer image.

Is it something else than kernel modules causing issues? Am I right to assume further fallout from #78430? See also #109280 for additional discussion.

Hm so the modules were part of it, but so were the firmware options. It all generally made initrd way too large to fit in memory to boot with.
(and yes, most of the modules it adds to the list do not exist)

@urbas
Copy link
Contributor

urbas commented Jul 4, 2021

I wonder if we ever use the nixos/modules/installer/sd-card/sd-image.nix module directly to build any official images. If so, this change might break those images. If not, then this PR should be safe to merge.

I agree with the thought about the installer/ directory. Perhaps a better approach would be to extract the base image stuff into a new module (maybe in nixos/modules/profiles/base-sd-image.nix or something like that). Then nixos/modules/installer/sd-card/sd-image.nix can import this module.

davegallant added a commit to davegallant/nixos-pi that referenced this pull request Aug 15, 2021
Copy link
Contributor

@urbas urbas left a comment

Choose a reason for hiding this comment

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

This looks good to me 👍

Edit: maybe just rebase on a recent nixpkgs-unstable.

@arcnmx
Copy link
Member Author

arcnmx commented Sep 20, 2021

Hm there shouldn't have been conflicts but rebased just in case!

@arcnmx
Copy link
Member Author

arcnmx commented Feb 12, 2022

rebased again

(this commit is basically just git mv nixos/modules/installer/sd-card/sd-image.nix nixos/modules/profiles/base-sd-image.nix, remove the all-hardware.nix import, and then put a stub where it used to be including both profiles)

@ofborg ofborg bot added 10.rebuild-linux: 1-10 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 12, 2022
@urbas
Copy link
Contributor

urbas commented Feb 13, 2022

Looks good to me 👍

Unfortunately, I don't have the permissions to merge PRs in this repo.

@samueldr, FYI

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 13, 2022
Removes the implicit profiles/all-hardware from SD image generation.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 21, 2023
@arcnmx
Copy link
Member Author

arcnmx commented Apr 21, 2023

Rebased again 😥

cc @samueldr

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 3, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants