canonical / chisel

GNU Affero General Public License v3.0
270 stars 42 forks source link

chore: add noble as separate github action #132

Closed letFunny closed 5 months ago

letFunny commented 6 months ago

Remove noble from the CI because the tests break too often. Will add them back when the release is stable.


letFunny commented 5 months ago

@niemeyer The problem is that noble is not stable yet, meaning the slice definitions change frequently and break. This is something Cris has be struggling with lately because it requires a lot of changes to keep up with upstream.

In my opinion, it is good to know that it is failing and we'll try to address it from the chisel-releases side but meanwhile, I think it should not impact releasing new versions of chisel the tool. It has to be treated similarly to a flaky test.

niemeyer commented 5 months ago

@niemeyer The problem is that noble is not stable yet, meaning the slice definitions change frequently and break. This is something Cris has be struggling with lately because it requires a lot of changes to keep up with upstream.

In my opinion, it is good to know that it is failing and we'll try to address it from the chisel-releases side but meanwhile, I think it should not impact releasing new versions of chisel the tool. It has to be treated similarly to a flaky test.

Ironically, that's exactly my concern: flaky tests are irrelevant, because you learn to ignore them. Either we do care about what the tests are saying and pursue their correction timely when we see a breakage, or we disable the test because they are teaching us that broken tests are just fine.

letFunny commented 5 months ago

Proof of tests running https://github.com/canonical/chisel/actions/runs/8663377963/job/23757382499?pr=132#step:5:172:

 2024-04-12 14:09:28 Starting 1 worker for the following jobs: 6
    - adhoc:ubuntu-23.04:tests/basic:focal
    - adhoc:ubuntu-23.04:tests/basic:jammy
    - adhoc:ubuntu-23.04:tests/basic:mantic
    - adhoc:ubuntu-23.04:tests/use-a-custom-chisel-release:focal
    - adhoc:ubuntu-23.04:tests/use-a-custom-chisel-release:jammy
    - adhoc:ubuntu-23.04:tests/use-a-custom-chisel-release:mantic
cjdcordeiro commented 5 months ago

The tests will help us identify the issues, and the split proposed in this PR makes sense because devel chisel-releases will tend to break more often than stable ones. Nonetheless, Chisel should still have a test for them.

In this particular case, the fix needs to come in chisel-releases, and I've been in touch with the foundations team because this issue in particular is a tricky one. It is related to Y2038, in a way that Noble has now introduced a bunch of *t64 package variants that offer exactly the same contents as the original packages - e.g. libssl3t64 offers the same contents as libssl3, and with time, only the libssl3t64 packages should survive. But until libssl3 is fully phased out, it may start dropping support for some of its previously supported architectures, and other packages that once relied on it, may now start relying on libssl3t64.

Since Chisel doesn't yet handle path conflicts nor architecture-specific essentials, our remediation (for now) will be to fully replace libssl3 by libssl3t64 for the entire ubuntu-24.04 chisel-release. I'm hoping this will land early next week

niemeyer commented 5 months ago

We have two options here:

1) We fix the tests

2) We disable the test

Having a flaky test that we're fine to see red makes no sense. Who has been looking at the Spread logs on every PR change to see exactly which tests have failed again in Noble? The answer is certainly "nobody".

We can trivially reenable the tests for Noble once you deem the release stable enough for that.

cjdcordeiro commented 5 months ago

Yep. I'm actually up for dropping these spread tests entirely, because now we have CI in the chisel-releases, which does a very similar job to what spread is doing here. The chisel-releases CI is flagging the same issues for 24.04 (and more, cause it's more than just libssl3*)

So actually these tests become redundant. I'm ok if you want to drop them.

letFunny commented 5 months ago

Well but the ones here test the latest version in the PR before merging, as a sort of smoke test before any commit enters main. For an error to be caught by the CI in chisel-releases then it has to already be part of main.

cjdcordeiro commented 5 months ago

True. Let's talk about it offline.