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

.github/workflows: add build action #94658

Closed
wants to merge 1 commit into from
Closed

Conversation

Kloenk
Copy link
Member

@Kloenk Kloenk commented Aug 4, 2020

Motivation for this change

Add a github build action to build on macos and ubuntu

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@edolstra
Copy link
Member

edolstra commented Aug 4, 2020

What's the goal? Isn't ofborg sufficient? I'd hate to add yet another CI system that can break.

@@ -0,0 +1,28 @@
name: "Build Packages"
Copy link
Member

Choose a reason for hiding this comment

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

What does "Build Packages" mean? What packages?

Copy link
Member

Choose a reason for hiding this comment

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

Packages are taken from commit message: i.e. hello: 1.0.0 -> 2.0.0. This is also how ofborg works now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Packages described in a PR title as ofborg builds them

@Mic92
Copy link
Member

Mic92 commented Aug 4, 2020

What's the goal? Isn't ofborg sufficient? I'd hate to add yet another CI system that can break.

It's to migrate responsibilities from ofborg. Ofborg can be only maintained right now by a 1-2 people, because there is not testing setup that allows to reproduce the environment. The idea is to combined this with a binary cache so that builds can be re-used between runs (important for staging) and also that reviewers can download them without re-compiling. ofborg also does not build macOS for all users since builds run without sandbox. Users needs to be explicitly trusted/whitelisted. Github actions solve this problem.

@Mic92 Mic92 requested a review from grahamc August 4, 2020 13:21
@Mic92
Copy link
Member

Mic92 commented Aug 4, 2020

@grahamc also has expressed that we wants to move ofborg to a different infrastructure (i.e. buildkite). Self-hosted github actions would be one solution to run evaluations that requires a lot of RAM.

@Mic92 Mic92 changed the title add build action .github/workflows: add build action Aug 4, 2020
- uses: cachix/install-nix-action@v10
with:
nix_path: nixpkgs=.
- uses: kloenk/ofbuild@0.0.2
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 makes a lot of sense to put the action into nixpkgs rather than being external.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense the code itself just parses git commit messages so complexity will be kept low.

Copy link
Member Author

Choose a reason for hiding this comment

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

The complete source code, or just the compile is asset?

Also not exactly sure if she need a action.yml if we move it into nixpkgs

Copy link
Member

Choose a reason for hiding this comment

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

If so the complete code.

@edolstra
Copy link
Member

edolstra commented Aug 4, 2020

It's to migrate responsibilities from ofborg.

Do you mean that this is intended to replace ofborg?

The idea is to combined this with a binary cache

Who has write access to this binary cache?

@Mic92
Copy link
Member

Mic92 commented Aug 4, 2020

It's to migrate responsibilities from ofborg.

Do you mean that this is intended to replace ofborg?

Ideally yes. It's less maintenance effort in long term if we offload the github api integration to github itself. There have been some problems with webhooks in the past if I remember correctly. Also the current UI for logs is not as good as having the log output directly in github itself. But I also would like to hear @grahamc opinion on that.

The idea is to combined this with a binary cache

Who has write access to this binary cache?

The Github CI and repo admins. The signing key would be stored in github itself. @domenkozar offered to host it. In any case it would be kept separate from hydra.

@zowoq
Copy link
Contributor

zowoq commented Aug 5, 2020

We may want to see about getting the required status checks working if we're going to start adding more actions.

@Mic92
Copy link
Member

Mic92 commented Aug 5, 2020

We may want to see about getting the required status checks working if we're going to start adding more actions.

I don't think with the current CI infrastructure we will get required status checks, @grahamc was concerned about the availability of ofborg. The Wait for ofborg action improves the situation because if ofborg is down users will get an indication for that. This can be further improved if ofborg would create one succeeding check even before the evaluation finishes and also create the evaluation checks before it actually start the evaluation. The current check api is buggy with w.r.t. external checks and only exposes finished checks and not pending one, I also tried web scraping however github only exposes CI status to logged in users...

@Mic92
Copy link
Member

Mic92 commented Aug 5, 2020

To further improve the situation I created NixOS/ofborg#527
It adds a check that gets triggered earlier so that "Waiting for ofborg" are less likely to timeout.

@FRidh
Copy link
Member

FRidh commented Aug 5, 2020

It would be good to write out the intentions on say the mailing list (discourse) so it does not come as a surprise. I was already surprised to see red crosses due to some linting.

@Mic92
Copy link
Member

Mic92 commented Aug 6, 2020

I see one problem with the integration of cachix. The signing key would be only available to builds in master, not every PR. Also every nixpkgs member could potentially export the key by pushing a commit to master. @domenkozar do you have scope how this could be limited?

@Mic92
Copy link
Member

Mic92 commented Aug 11, 2020

What's the goal? Isn't ofborg sufficient? I'd hate to add yet another CI system that can break.

The bus factor of our current CI is also too low: NixOS/ofborg#527 (comment)

@domenkozar
Copy link
Member

I see one problem with the integration of cachix. The signing key would be only available to builds in master, not every PR.

That depends on the goal. If the goal is to reduce build times, then it doesn't matter if we trust committers as long as people are not using the binary cache to trust the binaries, but only in a sandbox environment such as hosted runners.

If the goal is to trust the binaries without exposing the signing key to write users, then github model doesn't allow that - we have to wait for them to add a way to limit access further by locking down to a branch or a whitelist of users.

This can be somewhat limited by introducing IP whitelist on Cachix side, but an attacker could leak on github actions and push binaries via github actions.

@Mic92
Copy link
Member

Mic92 commented Aug 13, 2020

We don't have a macOS builder running at the moment. Graham won't be able to apply fixes to ofborg due to personal reasons that would make them work better together with github actions. Hence my proposal is to start those actions manually:

on: 
  issue_comment:
    types: [created]
name: Build packages mentioned in commits
jobs:
  build:
    name: build
    if: github.event.issue.pull_request != '' && contains(github.event.comment.body, '/build')
    runs-on: ubuntu-latest # and macOS?
    steps:
    - name: Checkout the latest code
      uses: actions/checkout@v2
    # ...

That should be a good compromise for people that want to test packages on different platforms, no?
Also it would gives us the opportunity to test new actions before enabling them by default.

@Mic92
Copy link
Member

Mic92 commented Aug 13, 2020

I see one problem with the integration of cachix. The signing key would be only available to builds in master, not every PR.

That depends on the goal. If the goal is to reduce build times, then it doesn't matter if we trust committers as long as people are not using the binary cache to trust the binaries, but only in a sandbox environment such as hosted runners.

If the goal is to trust the binaries without exposing the signing key to write users, then github model doesn't allow that - we have to wait for them to add a way to limit access further by locking down to a branch or a whitelist of users.

This can be somewhat limited by introducing IP whitelist on Cachix side, but an attacker could leak on github actions and push binaries via github actions.

Sounds good to me, if you are happy to implement this in cachix. Things would be a lot easier if we would have more control on what actions are executed, i.e. if actions would always come from the master branch.

@zowoq
Copy link
Contributor

zowoq commented Aug 13, 2020

We don't have a macOS builder running at the moment.

https://monitoring.nix.ci/d/000000002/ofborg?orgId=1&refresh=10s

Looks like it's back?

@Mic92
Copy link
Member

Mic92 commented Aug 13, 2020

We don't have a macOS builder running at the moment.

monitoring.nix.ci/d/000000002/ofborg?orgId=1&refresh=10s

Looks like it's back?

Still it's not available for general use, whereas github is. And since we only got one builder its availability is potentially also not so great.

@domenkozar
Copy link
Member

See #99722

@stale
Copy link

stale bot commented Jul 20, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 20, 2021
@Kloenk Kloenk closed this Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants