-
-
Notifications
You must be signed in to change notification settings - Fork 300
flake: add remaining drvs to ci.buildbot #2060
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
0xda157
commented
Dec 3, 2025
- I certify that I have the right to submit this contribution under the MIT license
- Commit messages adhere to Stylix commit conventions
- Theming changes adhere to the Stylix style guide
- Changes have been tested locally
- Changes have been tested in testbeds
- Each commit in this PR is suitable for backport to the current stable branch
dc1e859 to
39615fa
Compare
trueNAHO
left a comment
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.
Although this is not a critical test, it would be great to also evaluate formatter.${system} in ci.buildbot.
39615fa to
380ec70
Compare
380ec70 to
c49a504
Compare
c49a504 to
e013472
Compare
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.
A couple prompts/questions about grouping up some of the related/trivial checks for the purpose of reducing the number of workers/jobs buildbot-nix needs to spin up:
flake/dev/treefmt.nix
Outdated
| indentWidth = 2; | ||
| lineWidth = 80; | ||
| treefmt = { | ||
| projectRootFile = "flake.nix"; |
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.
Off-topic: this projectRootFile is no longer correct, now that you have two flake.nix files in-tree.
You could use "flake/dev/flake.nix" or any other path that can only be resolved when at the root of the flake.
We don't really need a flakeRootFile now, since treefmt knows to ask git rev-parse --show-toplevel if it's not defined. However, I don't think treefmt-nix is updated to know that, so it will always define it...
e013472 to
50fa0a2
Compare
50fa0a2 to
653f10e
Compare
4632081 to
7d28206
Compare
|
Testing locally: However, I can't see any of the new attrs being built here: https://buildbot.nix-community.org/#/builders/7620/builds/158 Does buildbot-nix get the attrs to be built from the base branch instead of the head of the PR? |
it shouldn't. cc @zowoq |
|
https://buildbot.nix-community.org/api/v2/logs/1848491/raw_inline Looks like they are skipped because they already built and cached? |
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.
What about evaluating formatter.${system} in ci.buildbot? I assume this is a negligible performance hit and ensures everything is tested.
The formatter derivation (treefmt) is a dependency of your default devShell, which is built by buildbot-nix: ci.buildbot = { inherit (config) devShells; };(Incidentally, that should probably be a linkFarm, for the same reasons as elsewhere: ci.buildbot.devShells = pkgs.linkFarm "devShells" config.devShells; |
treefmt is also built as part of the pre-commit drv |
7d28206 to
72dc809
Compare
72dc809 to
69f1522
Compare
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.
If removing this workflow, you'll need to update your ruleset just before you merge; currently this PR is blocked waiting on required checks that'll never run.
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.
If removing this workflow, you'll need to update your ruleset just before you merge
Will this not automatically be updated once this workflow is no longer on the target master branch? In that case, pending PRs that are not rebased on top of this patchset will implicitly use an incomplete Buildbot CI, meaning pending PRs should be rebased before merging.
currently this PR is blocked waiting on required checks that'll never run.
I force merged this PR for now.
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.
Will this not automatically be updated once this workflow is no longer on the target
masterbranch? In that case, pending PRs that are not rebased on top of this patchset will implicitly use an incomplete Buildbot CI, meaning pending PRs should be rebased before merging.
No. Required status checks relates to the name of the status check, it has nothing to do with the workflow. For workflows, the status check name is the same as the job name.
Currently your ruleset will have something like:
- Required status checks:
"x86_64-linux""aarch64-linux""x86_64-darwin""aarch64-darwin"
I.e. it expects status checks with each of the above names to be reported as "successful" or "skipped". It doesn't know or care that these come from jobs in the check.yml workflow.
You need to update your ruleset to instead expect the nix-build status check, as used by buildbot. Or, for belts and braces, nix-eval and nix-build.
In that case, pending PRs that are not rebased on top of this patchset will implicitly use an incomplete Buildbot CI
That's already the case, and is unrelated to your ruleset. Buildbot will run on all branches, using that branch's configuration and flake outputs.
Other PRs not rebased on this should still run the GHA workflow, since it is still present in their branch. You'll have to manually pay attention to this during the transition period once it is not a "required" status check.
Even without "required status checks", you will still notice the big red ❌ on the PR indicating that at least some CI thing 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.
Will this not automatically be updated once this workflow is no longer on the target
masterbranch? In that case, pending PRs that are not rebased on top of this patchset will implicitly use an incomplete Buildbot CI, meaning pending PRs should be rebased before merging.No. Required status checks relates to the name of the status check, it has nothing to do with the workflow. For workflows, the status check name is the same as the job name.
Currently your ruleset will have something like:
- Required status checks:
"x86_64-linux""aarch64-linux""x86_64-darwin""aarch64-darwin"I.e. it expects status checks with each of the above names to be reported as "successful" or "skipped". It doesn't know or care that these come from jobs in the
check.ymlworkflow.You need to update your ruleset to instead expect the
nix-buildstatus check, as used by buildbot. Or, for belts and braces,nix-evalandnix-build.
Thanks as always for the thorough explanation.
Based on the CI status from commit 551df12 ("flake: add remaining drvs to ci.buildbot (#2060)"), it seems the buildbot/nix-build, buildbot/nix-effects, and buildbot/nix-eval checks are already included. According to nix-community/infra#2027 (comment), this happened automatically. In that case, only the following assumed entries have to be removed:
"x86_64-linux""aarch64-linux""x86_64-darwin""aarch64-darwin"
AFAICT, I do not have permission to view or modify the rulesets. IIUC, @nix-community/stylix-committers seems to indicate that @danth has the necessary permissions. IIRC, #655 (comment) is the last time @danth changed the rulesets.
In that case, pending PRs that are not rebased on top of this patchset will implicitly use an incomplete Buildbot CI
That's already the case, and is unrelated to your ruleset. Buildbot will run on all branches, using that branch's configuration and flake outputs.
Other PRs not rebased on this should still run the GHA workflow, since it is still present in their branch. You'll have to manually pay attention to this during the transition period once it is not a "required" status check.
Even without "required status checks", you will still notice the big red ❌ on the PR indicating that at least some CI thing failed.
I suppose unless we force merge, the broken CI will be obvious enough that no extra precautions are needed.
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.
The point of required status checks is to make "force" merging necessary when the specified check failed (or hasn't run).
I.e. you can't normally merge a PR until all required status checks have finished successfully (or been skipped).
The required status checks ruleset rule doesn't affect which checks actually run.
Any repo admin or owner, and any org owner, can manage rulesets. Cc @zowoq.
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've updated the ruleset to require the new Buildbot checks, and removed the old GitHub Actions ones. Existing PRs will show the Buildbot checks as "expected" until a new commit is pushed, triggering a CI run.
trueNAHO
left a comment
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.
LGTM.
|
Successfully created backport PR for |