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

Move Linux CI jobs to GitHub Actions #1471

Merged
merged 6 commits into from
Feb 1, 2021
Merged

Move Linux CI jobs to GitHub Actions #1471

merged 6 commits into from
Feb 1, 2021

Conversation

mboes
Copy link
Member

@mboes mboes commented Jan 10, 2021

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.

Copy link
Member

@aherrmann aherrmann left a 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
Comment on lines 15 to 17
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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@@ -1,45 +0,0 @@
steps:
Copy link
Member

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.

@mboes
Copy link
Member Author

mboes commented Jan 21, 2021

@aherrmann just realized that yesterday when I did the force push, this somehow went to master instead of this branch. So this PR is effectively already merged, although in the local state when I did the push, i.e. without your latest commits. So I rebased this branch on top of the new master, taking into account your commits.

This is probably why master is red currently. :-/

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.

@aherrmann
Copy link
Member

@aherrmann just realized that yesterday when I did the force push, this somehow went to master instead of this branch. So this PR is effectively already merged, although in the local state when I did the push, i.e. without your latest commits. So I rebased this branch on top of the new master, taking into account your commits.

This is probably why master is red currently. :-/

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 master would prevent pushing to master directly. Checking the settings, branch protection is enabled on master.

@mboes
Copy link
Member Author

mboes commented Jan 21, 2021 via email

@aherrmann
Copy link
Member

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.

People and apps with admin permissions to a repository are always able to push to a protected branch.

@aherrmann
Copy link
Member

This was failing on buildifier warnings

@aherrmann
Copy link
Member

Ecountered an error of the following form here

ERROR: The Build Event Protocol upload failed: All retry attempts failed. DEADLINE_EXCEEDED: DEADLINE_EXCEEDED: deadline exceeded after 14999960100ns DEADLINE_EXCEEDED: DEADLINE_EXCEEDED: deadline exceeded after 14999960100ns

cc @siggisim

@siggisim
Copy link

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:
https://app.buildbuddy.io/invocation/d607713d-0b2b-47c0-8f3f-ef81dce03b56#

Are you seeing this consistently?

@aherrmann
Copy link
Member

Thanks for looking into this @siggisim .

Are you seeing this consistently?

No, this was the first time I noticed this. Yes, looks like network flakiness. I'll let you know if this happens repeatedly.

Tested on #1471 in Mergify
Config Editor UI.
@aherrmann
Copy link
Member

I've included an update to the mergify configuration and checked it in their config editor UI.

@aherrmann aherrmann added the merge-queue merge on green CI label Feb 1, 2021
@aherrmann aherrmann merged commit f5578b0 into master Feb 1, 2021
@aherrmann aherrmann deleted the gh-actions-all branch February 1, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-queue merge on green CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants