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

nixosTests: use hostPlatform.system instead of builtins.currentSystem #50028

Closed
wants to merge 1 commit into from

Conversation

Ekleog
Copy link
Member

@Ekleog Ekleog commented Nov 9, 2018

Motivation for this change

This should (hopefully) fix the invalid use of builtins.currentSystem in #44439.

I have tested on normal non-cross NixOS only, unfortunately. Also, I must say I don't really get the meaning of the system argument in tests (thought it was the platform on which the test was supposed to run, and hence should default to builtins.currentSystem, but I guess I was wrong), so this is likely not the correct system to pick :/

cc @Ericson2314

Things done
  • 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.

@Ekleog Ekleog changed the title nixosTests: use targetPlatform.system instead of builtins.currentSystem nixosTests: use buildPlatform.system instead of builtins.currentSystem Nov 10, 2018
@Ekleog
Copy link
Member Author

Ekleog commented Nov 10, 2018

OK so targetPlatform was apparently a bad pick, let's try with buildPlatform (I really don't understand what targetPlatform means for NixOS tests build, I guess, and I initially assumed builtins.currentSystem would be the same as buildPlatform… which may be wrong?)

Let's see if ofborg eval passes this time.

@Ekleog Ekleog changed the title nixosTests: use buildPlatform.system instead of builtins.currentSystem nixosTests: use hostPlatform.system instead of builtins.currentSystem Nov 10, 2018
@Ekleog
Copy link
Member Author

Ekleog commented Nov 10, 2018

Looks like a failure for buildPlatform too… let's try hostPlatform?

@@ -75,8 +75,7 @@ with pkgs;

nixosTests =
let
# TODO(Ericson2314,ekleog): Check this will work correctly with cross-
system = builtins.currentSystem;
system = hostPlatform.system;
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be stdenv.hostPlatform.system.

@Ekleog
Copy link
Member Author

Ekleog commented Nov 11, 2018

Closing this as I'm working on addressing the full tests concerns, which will likely render this PR irrelevant. Thank you for your feedback, will integrate in the upcoming PR! :)

@Ekleog Ekleog closed this Nov 11, 2018
@Ekleog
Copy link
Member Author

Ekleog commented Nov 11, 2018

This, and more and better (hopefully) is now at #50233.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants