Skip to content

CI workflow changes #139

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

Closed
GabrielMajeri opened this issue Jul 14, 2020 · 3 comments · Fixed by #141
Closed

CI workflow changes #139

GabrielMajeri opened this issue Jul 14, 2020 · 3 comments · Fixed by #141

Comments

@GabrielMajeri
Copy link
Collaborator

GabrielMajeri commented Jul 14, 2020

CI has been red for a while now, due to various problems. At this point, it's not very useful psychologically (does CI fail because the code is broken, or because of the well-known issue with QEMU?).

I propose some changes to the way we currently integrate new code into uefi-rs:

  • make the master branch protected, and only allow merging PRs with it
    • this might eventually require we set up something like bors to ensure code on master always compiles. Although at this point it seems unnecessary, we rarely have more than one PR at a time.
  • allow the current CI job which tries to build and run the code in QEMU to fail. We'll fix it, eventually, in Integration Test: Multiprocessor Test fails in QEMU/OVMF #103
  • add a new job which runs cargo check on all the crates - this will be required for merging
  • keep the job which runs cargo fmt - this will also be required for merging.
  • perhaps set up a cron job to regularly check if it still builds on latest nightly?

Cc @HadrienG2

@HadrienG2
Copy link
Contributor

HadrienG2 commented Jul 14, 2020

I agree that a perpetually failing CI is not useful. However, we still want to have the CI test the library as much as possible. Have it merely running cargo check is unsatisfying in this respect:

  • It does not test if we compile and link correctly (which, in nightly, may not be a given).
  • It disables all runtime QEMU tests, not just the ones that don't work (AFAIK there is only one failing SMP-related test, but I'll admit that I have not been looking at this in a long while and uefi-rs has evolved a lot since then).

Therefore, I would propose instead to specifically disable the SMP test that fails under current QEMU using a feature flag (like the one we use to detect whether we can exit on abort on x86), and leave the other tests that presumably work running.

Aside from that, I'm okay with making master protected.

@GabrielMajeri
Copy link
Collaborator Author

I wasn't necessarily saying we wouldn't run the tests in CI, I was thinking about running them as a job which doesn't fail the whole CI if any test fails. But I'll investigate if disabling the SMP test is enough.

@HadrienG2
Copy link
Contributor

According to #103, the current failure occurs when running the test test_switch_bsp_and_who_am_i from uefi-test-runner/src/proto/pi/mp.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants