-
Notifications
You must be signed in to change notification settings - Fork 81
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
Move Linux CI jobs to GitHub Actions #1471
Conversation
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.
Awesome, thank you! A couple small comments.
.bazelrc
Outdated
build:linux-nixpkgs --host_platform=@io_tweag_rules_nixpkgs//nixpkgs/platforms:host | ||
build:macos-nixpkgs --host_platform=@io_tweag_rules_nixpkgs//nixpkgs/platforms:host | ||
build:nixpkgs --host_platform=@io_tweag_rules_nixpkgs//nixpkgs/platforms:host |
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 breaks the config flags documented in the README. With this change the last reference to linux-nixpkgs
is gone and --config=linux-nixpkgs
becomes invalid. I'd prefer to keep the config matrix in the README valid for symmetry with the bindist cases and in case we need platform specific flags in the future.
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.
Bazel doesn't appear to support empty configurations (would make a good feature request upstream). We could hack around that, but I opted instead to remove this configuration from the README.
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 opened such a feature request: bazelbuild/bazel#12844.
I don't think that change is correct. On Linux/Nixpkgs we would need --config=nixpkgs
but the README claims (unneeded)
. On macOS we would now need both --config=nixpkgs --config=macos-nixpkgs
but the README claims --config=macos-nixpkgs
. The aim of #1301 was to require only one top-level --config
flag for developers.
I've pushed a commit that loads --config=nixpkgs
from both linux-nixpkgs
and macos-nixpkgs
. With this developers only need a single top-level config and both linux-nixpkgs and macos-nixpkgs unfold the nixpkgs config as well.
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'm not sure I follow. --config=nixpkgs
doesn't exist in this branch.
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.
Sorry, spurious comment. I had the wrong branch checked out.
.buildkite/pipeline.yml
Outdated
@@ -1,45 +0,0 @@ | |||
steps: |
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.
tests/RunTests.hs
now contains dangling references to .circleci
and .buildkite
in comments that need to be fixed.
bac7408
to
47b83e0
Compare
@aherrmann just realized that yesterday when I did the force push, this somehow went to This is probably why I use Magit, which has a concept of "upstream" and "push" branch: https://magit.vc/manual/magit/The-Two-Remotes.html. This is likely culprit. I noticed before that it interacts badly with using Git CLI. |
Hmm, I thought branch protection on |
AFAIU branch protection only protects against force pushes from admins.
Though we can enable stronger protection.
|
I see, if I understand these docs correctly then admins can always push.
|
This was failing on buildifier warnings |
That looks like some potential network causing a timeout to be hit - the invocation uploaded just fine on our end, and I can't find any errors in the logs related to that invocation: Are you seeing this consistently? |
Thanks for looking into this @siggisim .
No, this was the first time I noticed this. Yes, looks like network flakiness. I'll let you know if this happens repeatedly. |
This reverts commit 6a7d835.
It now points you to the master branch history.
f5249b6
to
3be1b15
Compare
Tested on #1471 in Mergify Config Editor UI.
I've included an update to the mergify configuration and checked it in their config editor UI. |
Hot on the heals of #1460, we move the Linux jobs to GitHub Actions as
well. In practice, BuildKite with custom runners wasn't buying us that
much time, though we may return to custom runners (via Actions) later.
The only CI job not on Actions is now the Windows one.