The outcome of the doc format RFC could mean that this isn't needed in a few months which might actually be a point in its favour as it can be easily dropped.

I'm assuming that adding these builds to ofborg is non-trivial and wouldn't be a productive use of time if we might end up changing formats anyway.

I also like github workflows for these sort of task more since they don't involve writing Rust and are easier to prototype.

cross-posted from #87853:

I would like to revert this PR and close #88488. Not because I don't like github actions or feel like my pet project ofborg is threatened, but because I think these checks are actually harming the clearness of the checks. I'll explain:

The editorconfig check puts a big green check next to a commit, and hides the list of checks which passed. If ofborg is down or behind, it is likely people will think ofborg has approved the PR before it did. This could cause, well, all the problems ofborg is there to prevent.

The concrete problem here is this green check appears very early and is misleading, and there is no poka yoke to help people do the right thing.

One fear of #88488 is that it will introduce red X's if it takes a long time to build the manual, or something in the target branch is failing to build. This is reminiscent of the Travis times for me: Travis gave out so few green checkmarks, that if Travis gave a PR a green check-mark, it inspired more "okay what is broken that let travis pass it?" than "okay good to merge".

The goal of ofborg, and ever since ofborg became a thing, the "rule" has been green checkmark -> good to merge and red X -> don't merge.

The editorconfig check errodes the clearness of the green checkmark signal.

The big-rebuild failures we'll certainly have with #88488 errodes the clearness of the red X signal.

The clearness of this signal has been my guiding light on the design of ofborg for years now, and I am very afraid of threatening it.

A few suggestions that have been brought up to improve this:

1. make ofborg faster I'd love to do this, but it doesn't solve this problem if ofborg is seriously delayed or down.
2. require the ofborg evaluation check for merges this is really the only solution, but not something we can do today, because of some workflow issues we need to resolve.

That makes me think that, today, we should be undoing this check and closing #88488.

We can now safely run github actions since there is an action that just waits for ofborg. However is this PR here still needed since ofborg already builds the manual?

However is this PR here still needed since ofborg already builds the manual?

AFAIK ofborg doesn't?

With the docs format changes ongoing I wasn't planning on doing any work on this for now.

ok.

Hmm, I guess the actual docs format change may not happen for a while but in the meantime we still have broken builds.

May as well give this a go.

Edit: I've just realised that in the last three weeks I've dealt with broken manual builds on three occasions. I've fixed the nixpkgs manual once myself, merged a PR that was also fixing the nixpkgs manual and merged another PR fixing the nixos manual.

Now that the install-nix-action is much faster using the full clone / git diff action is the bottleneck. May as well just run the build anyway.

Binary cache was mentioned in the other builds PR, would running the manual builds on master and staging as an hourly cron job and pushing to the cache address the rebuild concerns?

Binary cache was mentioned in the other builds PR, would running the manual builds on master and staging as an hourly cron job and pushing to the cache address the rebuild concerns?

This would not be even necessary. We could a cachix signing key that would just upload the manual on every push to master.

Binary cache was mentioned in the other builds PR, would running the manual builds on master and staging as an hourly cron job and pushing to the cache address the rebuild concerns?

This would not be even necessary. We could a cachix signing key that would just upload the manual on every push to master.

There might be one problem with approach though:

#94658 (comment)

That might also work: https://docs.github.com/en/actions/configuring-and-managing-workflows/caching-dependencies-to-speed-up-workflows

5GB for a manuals is more than enough.

Looks like if we want a job status we need to set it manually or just post a comment when it finishes with pass/fail and a link, would probably need to set another token for it to work.

Edit: also need to set the the pr/commit for the checkout.

I've switched this back to running when paths are modified and to use pull_request_target.

@Mic92 Which cachix account would we wan to use, nix-community or something else?

I've switched this back to running when paths are modified and to use pull_request_target.

@Mic92 Which cachix account would we wan to use, nix-community or something else?

This is up to @domenkozar One cachix key has the advantage that people have to add less caches to add but may be less secure because more people & machines have access to it.

I think it's best to create a separate account. Not sure who should do that, probably someone from the infrastructure team like @zimbatm

I might just create a cache under my name and include the key as plaintext in the workflow for now, we can switch to an official account later if this works.

@zowoq ok, but you likely need to give the credentials to someone who has administrative rights in nixpkgs like @domenkozar so its accessible from the NixOS org when starting actions from master.

@zowoq I added a signing key to the repo. I think it's fine to merge now. External contributors won't be able to push to the cache.

Thanks @zimbatm.

I've split this into two separate actions so that the entire job is conditional on its path being modified.

Apologies for the belated follow up on this, I reverted this shortly after it was merged as it wasn't working correctly and forgot to comment here.

pull_request_target was checking out only the base branch and not the PR branch so it wasn't actually doing anything except verifying that the manuals were building on the base branch. pull_request_target also allows write access to the repo so we can't just force it to check out the PR branch.

pull_request doesn't have write access but it will expose the cache token which is what we were trying to avoid, IIRC there have been some changes on the Cachix side with tokens but I haven't investigated that yet to see if it would help here.

Apologies for the belated follow up on this, I reverted this shortly after it was merged as it wasn't working correctly and forgot to comment here.

pull_request_target was checking out only the base branch and not the PR branch so it wasn't actually doing anything except verifying that the manuals were building on the base branch. pull_request_target also allows write access to the repo so we can't just force it to check out the PR branch.

Can it not checkout the pull request branch without executing code from it besides building the manual?

Maybe I'm misunderstanding what you mean but wouldn't building the manual also build any of it's dependenices that the PR modifies anyway?

Maybe I'm misunderstanding what you mean but wouldn't building the manual also build any of it's dependenices that the PR modifies anyway?

Yes, but in a nix sandbox, no? The nix binary would be provided by the installer-action. I think the most important security aspect would be that no secrets are in environment variables and than the nix sandbox should take care of the rest.

I think the most important security aspect would be that no secrets are in environment variables

What about the cachix token?

I think the most important security aspect would be that no secrets are in environment variables

What about the cachix token?

I think it no longer is exported via environment variables. I think the with statement only sets it for the cachix action itself: https://nix.dev/tutorials/continuous-integration-github-actions.html
Some sort of restricted eval to include files from the systemd should be disabled so.

Let's try this again: #106036