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

Replace bors with Github native merge queue #222

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

Veetaha
Copy link
Contributor

@Veetaha Veetaha commented Aug 19, 2023

Configuration

You need to enable the merge queues manually in the repository settings like this.

Details image image image

Be sure to specify all the required checks. Unfortuntelly they must be maintained separately in the repo settings. Maybe once this becomes a big enough problem and integrations/terraform-provider-github#1481 is closed, we may use terraform to manage the github configurations as code.

It's possible to trigger a test workflow run using the Actions tab in github UI:

image

Not sure if this is in fact useful, but why not.

The merge queue can be observed under a special URL. That URL can be obtained elsewhere. For example, when you submit the PR into the queue, a link will appear where the merge button was. This is really stupid, but yeah, github didn't take care to create an explicit tab in the UI to job to the merge queue view.

Details image

Merging strategy

The Squash and merge method will produce a commit with the message that begins with the PR title, PR number, and the body of the message will contain all the commit messages combined together. There isn't a way to customize it, unfortunately. I wish github could put the PR description there (just like bors does that), but we are here to suffer.

image

Anyway, I don't think it is very important. The main thing is that the PR title is in the head of the commit message and the PR reference is also there, so if someone wants to read what the commit does they could read just the commit title, or follow the PR link for details.

CI changes

Squashed the mdbook CI with into a shared ci.yml. The reason for that is that this way it is simpler. It also makes it possible to make the mdbook a required check in the repo config. Unfortunatelly, if we make this job conditional (running only on md files changes), then we won't be able to make it required in the repo config. I know another workaround that involves making each step of the job conditional, but that is so hacky and dirty, that I don't want to apply it at this stage. This job requires mere seconds to run, I don't think it makes sense to optimize it and make it conditional.


Split the rust checks, tests and lints into separate jobs. This way they all run in parallel and should be more efficient. With this change CI for all major platforms is going to run on PRs as well. I don't think it makes sense to run only ubuntu on CI. The CI run time is very small and we don't pay for it. Let's use all of its juice for early feedback.


Transitioned to actions-rust-lang/setup-rust-toolchain@v1. This action handles caching of CARGO_HOME and ./target dir artifacts for us.


I tried to use cargo-nextest to speed up the test runs. But the problem is that marker_uilints has to support the interface of libtest, i.e. the CLI parameters required by cargo-nextest for custom harnesses: https://nexte.st/book/custom-test-harnesses.html. Maybe at some point, we'll fix this in #221.

I even made the download scripts cross-platform to be able to download cargo-nextest on windows, but then found blocker described in #221. I left the changes to download scripts. At least the guys that use windows/mac will be able to use them to download various tools used in this repo on their platforms using the scripts from this repo

Code changes

I removed the usage of lint_reasons nightly feature from marker_adapater to make it compile on stable. I think it makes sense to use the stable toolchain and only stable features if possible. For example, marker_lints could also compile on stable, but I didn't tackle that as part of this PR.

Maybe marker_rustc_driver could live in a separate cargo workspace with the nightly toolchain in its rust-toolchain file. I didn't play with that, and not sure if it will compilate things instead.


Closes: #129

@xFrednet xFrednet self-assigned this Aug 20, 2023
@xFrednet xFrednet added C-enhancement Category: New feature or request A-infra Area: Infrastructure and CI magic :sparkles: S-waiting-on-review Status: Awaiting review labels Aug 20, 2023
@xFrednet
Copy link
Member

Thank you for the PR, I'll review it in the coming week :)

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

LGTM, just two questions, and then I can enable merge queues and merge this :)

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 20, 2023

Sorry I had to force-push because I just noticed I had local changes that were done with git commit --amend but were not pushed.

The diff is this

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 33f351b..b105a6c 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -15,10 +15,12 @@ defaults:
 
 env:
   RUST_BACKTRACE: 1
-  CARGO_INCREMENTAL: 0
   CARGO_TERM_COLOR: always
   RUSTDOCFLAGS: --deny warnings
   RUSTFLAGS: --deny warnings
+  # This disables incremental compilation for workspace packages and path deps.
+  # All other dependencies including registry deps will still use the incremental cache.
+  CARGO_INCREMENTAL: 0
 
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}
@@ -57,7 +59,7 @@ jobs:
     steps:
       - uses: actions/checkout@v3
       # We don't need any cache here, so use dtolnay action directly
-      - uses: dtolnay/rust-toolchain@stable
+      - uses: dtolnay/[email protected]
       - run: cargo fmt --check
 
   # This job ensures required packages can be built with a stable toolchain
@@ -74,7 +76,7 @@ jobs:
       - run: rm rust-toolchain.toml
       - uses: actions-rust-lang/setup-rust-toolchain@v1
         with:
-          toolchain: stable
+          toolchain: 1.65.0
 
       - run: >-
           cargo check

```
error[E0658]: custom discriminant values are not allowed in enums with tuple or struct variants
```
@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 20, 2023

Hey @xFrednet I see that CI now fails on this error when trying to build with 1.66:

error: `-Csplit-debuginfo=unpacked` is unstable on this platform

I suppose support for unpacked debug info on windows was added in some recent version of rust toolchain? Could you elaborate why this setting was enabled in .cargo/config.toml?

@xFrednet
Copy link
Member

xFrednet commented Aug 20, 2023

I suppose support for unpacked debug info on windows was added in some recent version of rust toolchain? Could you elaborate why this setting was enabled in .cargo/config.toml?

We use that setting in Clippy, as it slightly speeds up compile times on some platforms, if it makes trouble, you can remove it.

Or maybe better, we just delete the file, in the stable compilation check

@xFrednet
Copy link
Member

Sorry I had to force-push because I just noticed I had local changes that were done with git commit --amend but were not pushed.

No need to be sorry :) Simple force pushes diffs are also displayed correctly by GH

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 20, 2023

I wouldn't like to delete the config file in CI because it may contain additional configs in the future. I'll just disable this for now.

@Veetaha
Copy link
Contributor Author

Veetaha commented Aug 20, 2023

Looks like CI is green now. So it can probably be merged

@Veetaha Veetaha requested a review from xFrednet August 20, 2023 20:19
@xFrednet
Copy link
Member

This version looks good to me, thank you very much! I'll merge this later today, when I have a bit more time to read through all the settings again and debug if necessary.

@xFrednet xFrednet added this to the v0.2.0 milestone Aug 21, 2023
@xFrednet
Copy link
Member

Merging strategy

I like having the option to squash, but it seems like GH requires us to select a single mode. I'll let it be the default merge commit one for now. I don't want to lose the commit structure or PRs, especially if they're still affecting many areas right now.

Let's see how this merge will go :)

@xFrednet xFrednet added this pull request to the merge queue Aug 21, 2023
Merged via the queue into rust-marker:master with commit 06b1928 Aug 21, 2023
14 checks passed
@xFrednet
Copy link
Member

Well, the queue seems to be working, but the book deployment broke, I'll see if I can fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infra Area: Infrastructure and CI magic :sparkles: C-enhancement Category: New feature or request S-waiting-on-review Status: Awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to github merge queues
2 participants