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

installer: split sd cards -> base for bespoke sd images #110827

Merged
merged 5 commits into from Feb 21, 2021

Conversation

blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Jan 26, 2021

Motivation for this change

https://discourse.nixos.org/t/sd-images-are-they-really-installation-devices/11190

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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

resolved Regardless of the future of this PR, I'd really appreciate tutoring on how to make this properly backwards compatible. I had thought of:

  • putting the old files in place
  • redirecting to the new files
  • issue a deprecation warning

But I've no experience with backwards compatibility.

There might also be references to those files all over the place, that I would need to update as well. How to best find those? I've never proposed such a "fundamental" change.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/sd-images-are-they-really-installation-devices/11190/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/casual-nixpkgs-contributions/9607/10

@blaggacao blaggacao force-pushed the da/sd-are-no-installation-devices branch from e3b52f6 to eff70fe Compare January 28, 2021 03:01
@blaggacao blaggacao marked this pull request as ready for review January 28, 2021 03:01
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/sd-images-are-they-really-installation-devices/11190/9

@blaggacao blaggacao changed the title installer: sd cards are no installation devices installer: split sd cards -> base for bespoke sd images Jan 28, 2021
Copy link
Contributor

@mohe2015 mohe2015 left a comment

Choose a reason for hiding this comment

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

Looks good, didn't test it though.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/how-to-deprecate-a-module-path/11247/1

@blaggacao blaggacao force-pushed the da/sd-are-no-installation-devices branch from 91ee955 to 0b54ab9 Compare February 2, 2021 22:47
@blaggacao
Copy link
Contributor Author

@samueldr May I ask you for a review (maybe even merge)? You seem rather familiar with that portion of the code.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/40

Copy link
Contributor

@mohe2015 mohe2015 left a comment

Choose a reason for hiding this comment

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

Didn't test it but otherwise LGTM. Also seems to be backward compatible and a change I also have thought about.

Edit: Hint: Look at the commit-wise diff. Github messes up the whole PR diff.

@blaggacao blaggacao force-pushed the da/sd-are-no-installation-devices branch from 969b5e3 to 5ddb35d Compare February 19, 2021 17:37
@blaggacao
Copy link
Contributor Author

squashed & rebased

@blaggacao
Copy link
Contributor Author

/cc @flokli @lopsided98 @jslight90 please consider a review to help me get this merged.

@lopsided98
Copy link
Contributor

This seems to make sense based on a quick look at the older version, but since you squashed the commits it is very hard to review because I can't distinguish renames from meaningful changes. I would prefer if you rebased to remove the merge commits but kept the individual commits.

@mohe2015
Copy link
Contributor

@blaggacao IMHO you should undo the squash as the previous commits were semantically easy to parse and would make more sense in the git history.

@blaggacao

This comment has been minimized.

@lopsided98
Copy link
Contributor

git reflog?

@blaggacao blaggacao force-pushed the da/sd-are-no-installation-devices branch from 5ddb35d to afef283 Compare February 19, 2021 23:56
@blaggacao
Copy link
Contributor Author

git reflog?

Thx! Wasn't familiar with this command. Pretty useful!

@blaggacao blaggacao force-pushed the da/sd-are-no-installation-devices branch from afef283 to 68afbf9 Compare February 20, 2021 00:00
@blaggacao
Copy link
Contributor Author

As for 68afbf9 : should we leave any well-known regexable for the release manager of the next-next release?

Copy link
Contributor

@mohe2015 mohe2015 left a comment

Choose a reason for hiding this comment

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

Didn't test it but LGTM. Also seems to be backward compatible and a change I also have thought about.

Hint: Look at the commit-wise diff. Github messes up the whole PR diff.

@mohe2015
Copy link
Contributor

As for 68afbf9 : should we leave any well-known regexable for the release manager of the next-next release?

I don't know.

Copy link
Contributor

@lopsided98 lopsided98 left a comment

Choose a reason for hiding this comment

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

I haven't tested this, but the possibility of breakage seem limited assuming everything evals.

One thought I had (not necessarily a suggestion for this PR) is that we might not need sd-image-aarch64-new-kernel.nix and sd-image-raspberrypi4.nix. It makes sense to provide the -installer versions so users can download them, but for building custom images they don't really offer much since they only override one option.

@blaggacao
Copy link
Contributor Author

It makes sense to provide the -installer versions so users can download them, but for building custom images they don't really offer much since they only override one option.

On the discourse thread, some people raised that this is avery valid use case. I doubt it in a way, omce you ever tried tu build something on the pi (takes days and usually hangs). I think this split will allow for a better re-assessment some poimt in the future.

One thought I had (not necessarily a suggestion for this PR) is that we might not need sd-image-aarch64-new-kernel.nix and sd-image-raspberrypi4.nix.

Let's leave that for another PR, indeed. But I agree, it might ne wise to deduplicate a little and reduce here.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/352

@lopsided98
Copy link
Contributor

On the discourse thread, some people raised that this is avery valid use case. I doubt it in a way, omce you ever tried tu build something on the pi (takes days and usually hangs).

I'm not really sure what you mean. If someone doesn't already have another aarch64 machine, how would they install NixOS without downloading one of the prebuilt images? Modern aarch64 SBCs are also pretty capable builders, not necessarily the RPi <=3, but certainly the RPi 4, or RK3328/RK3399 boards (which can use the prebuilt installer image as well if you manually install the bootloader).

@blaggacao
Copy link
Contributor Author

blaggacao commented Feb 20, 2021

I'm not really sure what you mean. If someone doesn't already have another aarch64 machine, how would they install NixOS without downloading one of the prebuilt images? Modern aarch64 SBCs are also pretty capable builders, not necessarily the RPi <=3, but certainly the RPi 4, or RK3328/RK3399 boards (which can use the prebuilt installer image as well if you manually install the bootloader).

I definitley failed with pi3, indeed. I was hoping that nix proper cross building would save me here. It might also be slightly more convenient to work off my powerful workstation.

Actually, the driving use case for this PR is to explore cross build in the context of divnix/digga#72

I'm not actually trying to dismiss the installer version. I can imagine that cross builds are actually more user friendly and convenient, though.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

To summarize, this moves modules around so that their file paths actually make sense and extracts parts of them into new files so that they can be used independently, all without modifying existing installer image configurations.

@roberth roberth merged commit 92b1ef6 into NixOS:master Feb 21, 2021
blaggacao pushed a commit to xoe-labs/nixpkgs that referenced this pull request Feb 21, 2021
roberth added a commit that referenced this pull request Feb 21, 2021
installer: fixup sd-card folder move from #110827
@dramforever
Copy link
Contributor

@blaggacao

Sorry for the necrobump. Someone pointed me to this topic elsewhere, which prompted me to read this whole thread. I saw this bit:

On the discourse thread, some people raised that this is avery valid use case. I doubt it in a way, omce you ever tried tu build something on the pi (takes days and usually hangs). I think this split will allow for a better re-assessment some poimt in the future.

Yeah I'm one of the people who said that. I was thinking about why your experience of building on rpi is described is so different from mine when I realized: I use aarch64-linux on a Raspberry Pi 4. There's official binary cache for aarch64-linux.

This means that most of the time taken to rebuild the system is to download the packages, not build them. If I need to cross-compile an image, I need to build everything, which takes hours on my laptop (yes I've tried) for the first time, and then hours whenever there's a major update. But since the native stuff is all on cache.nixos.org, rebuilding on an rpi takes a negligible amount of time compared to me testing things out and debugging stuff. Not to mention avoiding the pain of needing to physically move the SD card around to write new images.

(If I remember right, there's an unofficial 32-bit ARM binary cache, but I couldn't remember the details.)

So I hope that when it comes to see whether the installer targets should stay or should leave, the fact that you just simply don't have to build that much stuff when there's binary cache available is taken into consideration.

Or I could have horribly misunderstood what this means, in which case I'm happy to be corrected or even ignored.

@blaggacao
Copy link
Contributor Author

blaggacao commented May 28, 2021

@dramforever Thanks for the context, fortunately this change is a pareto-improvement and I (at least) don't plan to follow up on removing anything. 😉

bors bot added a commit to nix-community/nixos-generators that referenced this pull request Jan 25, 2023
203: Fix formats/sd* r=Mic92 a=LegitMagic

During the [movement](NixOS/nixpkgs#110827) of the nixpkgs module paths for sd*, more than just the cd-dvd -> sd-card was changed. As noted within the [old nixpkgs module location](https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/installer/cd-dvd/sd-image-aarch64.nix), the intended change of location was sd-card/sd-image-aarch64-installer, rather than just a path change.

For the non installer version for sd-aarch64, I've updated the version here with the version from nixpkgs, minus the import of profiles/base.nix. This means that the images produced should work on the Pi 4 as well as the Pi 3. Although I have not personally tested on hardware. The removal of the CMA kernel param is deliberate, as 32M is too low for the Pi 4, and that it would need to be either upped to the default for the Pi 4 of 64M, or the option removed and let defaults take care of it.

More information about the motivation of these changes can be seen in this comment #168 (comment).

In regards to a bit of discussion, is it worth having our own copy of the nixpkgs version of sd-aarch64? The only difference I could really tell is that the version in nixos-generators doesn't include profiles/base. In my opinion there's not much of a need for profiles/base to be included, but does it hurt to do so? I'm pretty new to Nix so I'm not fully aware of its features, but as far as I understand it, I don't think there's a way to override what gets imported in an import.

Co-authored-by: LegitMagic <76862862+LegitMagic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants