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

nixos/tests/installer: add missing system.extraDependencies #50392

Closed
wants to merge 1 commit into from

Conversation

nyanloutre
Copy link
Member

@nyanloutre nyanloutre commented Nov 15, 2018

Motivation for this change

since 39a9b86 the test VM
depends on some extra packages to build the system to be installed.
This broke the installer test as it tried to download/build these
packages in a sandbox.

original issue : #45866

Things done

I did what @xeji did in 43e30b1

Running the test on my machine is working and is fixing the issue

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

since 39a9b86 the test VM
depends on some extra packages to build the system to be installed.
This broke the installer test as it tried to download/build these
packages in a sandbox.
@nyanloutre
Copy link
Member Author

cc @vcunat which was involved in the previous commit discussion

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/channels-stuck-for-one-week/1458/1

@Ekleog
Copy link
Member

Ekleog commented Nov 16, 2018

Adding gtk as a dependency for the (headless) installer sounds wrong: it'd mean most headless systems would get this dependency.

Have you investigated whether there is a reason for setting default to true? Would it be possible to have it eg. mkDefault config.services.xserver.enable; to have it turned it on by default iff X is turned on?

@Ekleog
Copy link
Member

Ekleog commented Nov 16, 2018

cc @romildo about gtk being pulled in for headless systems following 39a9b86

@vcunat
Copy link
Member

vcunat commented Nov 17, 2018

I pushed a different quick solution for now: 80738ed.

@vcunat vcunat closed this Nov 17, 2018
@Ekleog
Copy link
Member

Ekleog commented Nov 17, 2018

@vcunat I think your commit triggers an infinite loop not detected by nix: my configuration takes more than 1 minute to eval (didn't wait longer) following your commit, and less than 15 seconds before.

I think you can't use config. directly in default, and that's why I stated mkDefault.

@vcunat
Copy link
Member

vcunat commented Nov 17, 2018

@Ekleog: if I do that, it won't even evaluate (type check fails). What I pushed seems to work as expected in the headless test and my desktop config (I did check before pushing).

I don't know the module system well; I tried to defer default definition into config =, but that seems not sufficient without further hacks, though that might be the best approach to try. (I can have a look in the evening and/or tomorrow.)

@Ekleog
Copy link
Member

Ekleog commented Nov 17, 2018

Oh indeed, trying it again my desktop config finishes in less than 15 seconds… weird. I guess something somewhere made my config take more than 1 minute to evaluate… maybe some non-pinned IFD somewhere that triggered a download at eval time that no longer occurs? I can't remember anything like this, but…

Anyway, sorry for the false flag and thank you for the fix!

@nyanloutre nyanloutre deleted the installer-test-fix branch January 14, 2019 09:29
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

5 participants