-
Notifications
You must be signed in to change notification settings - Fork 250
Better 'required' job that includes everything #976
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
Conversation
This requires explicitly identifying the things tht don't work and removing them. I used our own `disabled` attribute, because `broken` is weird and breaks evaluation, which isn't really what we want.
bors try |
bors try |
tryAlready running a review |
bors try |
tryAlready running a review |
bors try- |
bors try |
tryBuild failed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- if it builds!
isDisabled = d: | ||
let meta = d.meta or {}; | ||
in meta.disabled or false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea - I was thinking about doing the same thing in cardano-wallet.
meta.disabled = stdenv.hostPlatform.isMusl; | ||
meta = { | ||
platforms = platforms.all; | ||
disabled = stdenv.hostPlatform != stdenv.buildPlatform || stdenv.hostPlatform.isMusl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I previously had a horrible time doing these sort of platform checks in order to remove jobs from the Hydra jobset.
What I think was happening, is that Hydra was getting the job list by evaluating with its native linux platform. So I was unable to remove jobs for e.g. macOS only.
But I was probably doing something wrong - is this working alright on Hydra?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was working before! The platform it's evaluating on shouldn't matter (unless you use buildins.currentSystem
- this is checking the host/build platforms from stdenv, so should be okay.
bors try |
tryBuild failed: |
bors try |
bors try |
tryAlready running a review |
bors try- |
bors try |
This doesn't actually matter, not sure why I was changing it.
tryBuild failed: |
bors try |
tryTimed out. |
Interestingly, the commit status for the previous commit is okay, so it actually evaluated there. I guess it's just right on the edge of what's manageable? I guess I'm just not sure how to make it use less memory... |
bors try |
tryBuild failed: |
bors try |
bors try |
tryAlready running a review |
bors try- |
bors try |
bors try |
tryAlready running a review |
bors try- |
bors try |
tryBuild succeeded: |
OMG NixOS/hydra#715 worked! It seems to fix the OOM issue with having one massive required job. This PR now also cuts the number of jobs from over 2000 to 216. This seems to greatly speeds up the time it takes small changes (where most stuff remains the same) to be processed by hydra. |
🥇 Hamish you are a champion! |
This requires explicitly identifying the things tht don't work and removing them. I used our own `disabled` attribute, because `broken` is weird and breaks evaluation, which isn't really what we want. Cuts the number of jobs from over 2000 to 216. This seems to greatly speeds up the time it takes small changes (where most stuff remains the same) to be processed by hydra. Uses NixOS/hydra#715 to reference the jobs in required by name avoiding OOM issues. Co-authored-by: Hamish Mackenzie <Hamish.Mackenzie@iohk.io>
This requires explicitly identifying the things that don't work and
removing them. I used our own
disabled
attribute, becausebroken
isweird and breaks evaluation, which isn't really what we want.