Skip to content

Fix PR #193175#193495

Merged
roberth merged 2 commits intoNixOS:masterfrom
hercules-ci:fix-pull-193175
Sep 29, 2022
Merged

Fix PR #193175#193495
roberth merged 2 commits intoNixOS:masterfrom
hercules-ci:fix-pull-193175

Conversation

@roberth
Copy link
Member

@roberth roberth commented Sep 29, 2022

Description of changes

Unblock channel.

Fixes error introduced as #193175 and cleans up the config mess.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@roberth roberth added the 1.severity: channel blocker Blocks a channel label Sep 29, 2022
@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 Sep 29, 2022
@roberth
Copy link
Member Author

roberth commented Sep 29, 2022

@ofborg test boot.biosCdrom

@roberth roberth requested review from K900 and alyssais September 29, 2022 10:24
@roberth
Copy link
Member Author

roberth commented Sep 29, 2022

tests.boot.biosCdrom on aarch64-linux Skipped — No attempt

Expected because

// optionalAttrs (pkgs.stdenv.hostPlatform.system == "x86_64-linux") {
    biosCdrom = makeBootTest "bios-cdrom" {

@alyssais
Copy link
Member

Ugh, sorry about that. OfBorg was happy.

@roberth
Copy link
Member Author

roberth commented Sep 29, 2022

Ugh, sorry about that. OfBorg was happy.

Happens to all of us.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 29, 2022
@alyssais
Copy link
Member

So, do I understand correctly that config here was just the config value passed in by the module system (the one containing the final values of all NixOS options), and that it ended up just shadowing itself? And wasn't actually used in at least some of the functions it was passed into? Trying to follow what this did before makes my head hurt.

@unbel13ver
Copy link
Contributor

Sorry for introducing this bug! I actually tested that with
$ nix-shell -p nixos-generators --run "nixos-generate --format iso --configuration ./iso.nix -o result" -I nixpkgs=<patched nixpkgs>
where iso.nix is:

{ pkgs, modulesPath, lib, ... }: {
  imports = [
    "${modulesPath}/installer/cd-dvd/installation-cd-base.nix"
  ];
  boot.loader.grub.forcei686 = true;
  # use the latest Linux kernel
  boot.kernelPackages = pkgs.linuxPackages_latest;
  boot.supportedFilesystems = lib.mkForce ["ext4fs"];
}

@roberth
Copy link
Member Author

roberth commented Sep 29, 2022

So, do I understand correctly that config here was just the config value passed in by the module system (the one containing the final values of all NixOS options), and that it ended up just shadowing itself? And wasn't actually used in at least some of the functions it was passed into? Trying to follow what this did before makes my head hurt.

Correct. I've double-checked that config is only brought into scope 1. as the (top level) module's module argument, 2. in the two functions that always received the config module argument; and nowhere else.

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

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

Thank you for the additional refactor to make this less confusing!

@roberth
Copy link
Member Author

roberth commented Sep 29, 2022

Sorry for introducing this bug! I actually tested that with $ nix-shell -p nixos-generators --run "nixos-generate --format iso --configuration ./iso.nix -o result" -I nixpkgs=<patched nixpkgs> where iso.nix is:

{ pkgs, modulesPath, lib, ... }: {
  imports = [
    "${modulesPath}/installer/cd-dvd/installation-cd-base.nix"
  ];
  boot.loader.grub.forcei686 = true;
  # use the latest Linux kernel
  boot.kernelPackages = pkgs.linuxPackages_latest;
  boot.supportedFilesystems = lib.mkForce ["ext4fs"];
}

I'm surprised that that worked. Maybe something else was also going on.
At least this was caught early. Very early in fact, because the error was discovered by Nix's static scope checking. 🤓

@roberth roberth merged commit bd9f4f3 into NixOS:master Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.severity: channel blocker Blocks a channel 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 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants