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

clean up ci/make ci nicer #682

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Conversation

likewhatevs
Copy link
Contributor

Replace build scheds and merged with caching build, and rename caching build to build-and-test.

This should make the CI reports on PRs be nice and specific (i.e. at a glance, know what passes and what fails).

It also keeps PR CI jobs up to date (as folks edit things) and has them all use one config/24.04 etc.

Replace build scheds and merged with caching build, and rename
caching build to build-and-test.

This should make the CI reports on PRs be nice and specific
(i.e. at a glance, know what passes and what fails).

It also keeps PR CI jobs up to date (as folks edit things) and
has them all use one config/24.04 etc.
@likewhatevs
Copy link
Contributor Author

Github CI is running the PR config in this PR on this PR so we can see if we like it/if it is better.

the defaults for the pr ci runs look good,
they are "run on open/reopen/update code",
so use those.
@likewhatevs
Copy link
Contributor Author

I think this works (comment is to see if I generate a ton more jobs, just checking).

@JakeHillion
Copy link
Contributor

Some drive by comments on the CI as this diff brought them to mind:

  • Should we be using apt-get instead of apt to avoid those warnings about apt not having a stable CLI interface? It's never made any difference to me before though...
  • Is for-next still the right branch? I'm wondering if we care more about what's in Linus's tree now we've landed, though it would be brutal for the caching to pull from torvalds/linux/master. Maybe for-next is better for testing the kernel changes, but for testing PRs in this repo I feel like master or really the latest stable kernel would be better once we get to that point.
  • What does # cache virtiofsd (goes away w/ 24.04) mean? The images look like they are 24.04 already.

Otherwise, I think these checks are way more useful than a mega check with far too much output.

build-kernel:
runs-on: ubuntu-24.04
steps:
# prevent cache permission errors
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed everywhere? Is the base image bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this needed everywhere? Is the base image bad?

Ehh, bad is a strong term. I'd argue the cache action is not very good for leaving that foot-gun in (everywhere cuz someone will get it wrong in the future otherwise).

@@ -185,6 +210,8 @@ jobs:
matrix:
scheduler: [ scx_bpfland, scx_lavd, scx_layered, scx_rlfifo, scx_rustland, scx_rusty ]
steps:
# prevent cache permission errors
- run: sudo chown root /usr/bin/tar && sudo chmod u+s /usr/bin/tar
- uses: actions/checkout@v4
- uses: ./.github/actions/install-deps-action
# cache virtiofsd (goes away w/ 24.04)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably add the per scheduler veristat meson script as well to this. That way if there is a verifier error it will be in the test for the scheduler as well as in the integration test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd add that separately because that might have flakiness of it's own/I haven't seen that run on CI a lot (i.e. to goal of this is mainly to make CI output more useful while one scheduler is broken + ensure caching works right so queuing doesn't become an issue).

@likewhatevs
Copy link
Contributor Author

  • Should we be using apt-get instead of apt to avoid those warnings about apt not having a stable CLI interface? It's never made any difference to me before though...

I agree w/ this, but wouldn't tackle it here (i.e. we've seen the current set up run for a while, this is mainly splitting that, that should be added separately IMO).

  • Is for-next still the right branch? I'm wondering if we care more about what's in Linus's tree now we've landed, though it would be brutal for the caching to pull from torvalds/linux/master. Maybe for-next is better for testing the kernel changes, but for testing PRs in this repo I feel like master or really the latest stable kernel would be better once we get to that point.

I think so. That prevents issues from ending up there (like the SM_IDLE one) IMO.

  • What does # cache virtiofsd (goes away w/ 24.04) mean? The images look like they are 24.04 already.

I agree w/ this, but wouldn't tackle it here (i.e. we've seen the current set up run for a while, this is mainly splitting that, that should be added separately IMO). Moreso w/ this one because there could be packaging issues etc.

@likewhatevs likewhatevs merged commit d8246fd into sched-ext:main Sep 24, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants