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

Jump to bottom

Fix evaluation of release-20.03-aarch64 jobset #82886

Merged
merged 2 commits into from Apr 6, 2020
Merged

Fix evaluation of release-20.03-aarch64 jobset #82886

merged 2 commits into from Apr 6, 2020

Conversation

bennofs
Copy link
Contributor

@bennofs bennofs commented Mar 18, 2020 •
edited

Motivation for this change

The nixos-20.03-aarch64 jobset on hydra currently fails to eval. This is because the constituents of the tested job are hardcoded to refer to the x86_64-linux tests, which aren't available in that jobset.

Proposed solution

This PR uses helper functions to only generate tested constituents for the available platforms. I also checked which tests are supported on aarch64, and limited everything else to x86_64-linux.

Variant for master: https://github.com/bennofs/nixpkgs/tree/fix-nixos-aarch64-eval-master

Things done
  1. verify that tested.constituents did stay the same before and after the change: diff of the output from nix-instantiate --eval --strict --json -A tested.constituents nixos/release-combined.nix | jq before and after the change:
-  "nixos.iso_minimal.aarch64-linux",
-  "nixos.iso_minimal.i686-linux",
   "nixos.iso_minimal.x86_64-linux",
+  "nixos.iso_minimal.i686-linux",
+  "nixos.iso_minimal.aarch64-linux",
  1. verify that all constituents of the tested job can be instantiated for aarch64:
#!/usr/bin/env fish
nix-instantiate --eval --strict --json -A tested.constituents --arg 'limitedSupportedSystems' '[]' --arg 'supportedSystems' '["aarch64-linux"]' nixos/release-combined.nix |
  jq -r '.[]' |
  while read attr;
      echo + instantiate $attr; 
      nix-instantiate -A $attr --arg 'limitedSupportedSystems' '[]' --arg 'supportedSystems' '["aarch64-linux"]' nixos/release-combined.nix;
  end

@bennofs
nixos/release-combined.nix: fix eval for aarch64
6d73360 
The release-20.03-aarch64 jobset on hydra only evals for aarch64, so the
x86_64 jobs do not exists. We need to make sure that the tested job only
aggregates jobs that actually exist.

This commit solves the issue by generating the tested job constituents
names based on the supported systems.
@bennofs
nixos/release-combined.nix: fix tested for aarch64
c5a9ee8 
This removes tests from the tested aggregate on aarch64 which are not
available for that platform.
@bennofs bennofs changed the title release- Fix evaluation of release-20.03-aarch64 jobset Mar 18, 2020
samueldr
samueldr requested changes Mar 18, 2020
View reviewed changes
Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

(Recording the change discussed on IRC here)

This change should first be made on master, then backported (though the backport will not be a trivial cherry-pick).

@bennofs
Copy link
Contributor Author

bennofs commented Mar 18, 2020

There is a version of this PR for master on https://github.com/bennofs/nixpkgs/tree/fix-nixos-aarch64-eval-master. Note however that the situation for master is a little bit different, as we don't have split jobsets for x86_64-linux and aarch64-linux there. Thus, if this change is applied to master, aarch64-linux tests will become part of the tested job on trunk-combined.

@bennofs
Copy link
Contributor Author

bennofs commented Mar 18, 2020

Is it OK to keep this PR for discussion? Or should I create a new one against master?

@worldofpeace
Copy link
Contributor

There is a version of this PR for master on https://github.com/bennofs/nixpkgs/tree/fix-nixos-aarch64-eval-master. Note however that the situation for master is a little bit different, as we don't have split jobsets for x86_64-linux and aarch64-linux there. Thus, if this change is applied to master, aarch64-linux tests will become part of the tested job on trunk-combined.

I guess that means we'd need two PRs, with this one merged just for 20.03.

@worldofpeace worldofpeace added this to the 20.03 milestone Mar 23, 2020
@bennofs bennofs mentioned this pull request Mar 23, 2020
Merged
@bennofs
Copy link
Contributor Author

bennofs commented Mar 23, 2020

@worldofpeace ok, I created a second PR for master.

worldofpeace
worldofpeace approved these changes Mar 28, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I believe this is fine, though I'm not sure how I feel about the function names (can't suggest better right now I'm afraid).

@andir
Copy link
Member

andir commented Mar 28, 2020

I did some prior work on a related topic. When the hydra eval changed we had to apply the conversion to strings on release-19.09 as well:
https://github.com/NixOS/nixpkgs/blob/release-19.09/nixos/release-combined.nix#L47-L149

I tried to be as close to the "old" style definition as possible to make it more familiar and reduce conflicts if we would be cherry-picking changes.

No real opinion just felt like that might be a good piece of information.

@doronbehar doronbehar mentioned this pull request Apr 3, 2020
Closed
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/go-no-go-meeting-nixos-20-03-markhor/6495/16

@worldofpeace worldofpeace merged commit 649b8e1 into NixOS:release-20.03 Apr 6, 2020
grahamc added a commit that referenced this pull request Apr 8, 2020
@grahamc
Merge pull request #83249 from bennofs/fix-nixos-aarch64-eval-master
2bf8a72 
nixos/release-combined.nix: fix tested/supportedSystems (master version of #82886)
@bennofs bennofs mentioned this pull request Apr 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samueldr samueldr samueldr approved these changes

@worldofpeace worldofpeace worldofpeace approved these changes

@fpletz fpletz Awaiting requested review from fpletz

@edolstra edolstra Awaiting requested review from edolstra

@grahamc grahamc Awaiting requested review from grahamc

@flokli flokli Awaiting requested review from flokli

@andir andir Awaiting requested review from andir

@Ma27 Ma27 Awaiting requested review from Ma27

Assignees
No one assigned
Labels
6.topic: nixos 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bennofs @worldofpeace @andir @nixos-discourse @samueldr