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

CI: Fix nightly lint error & fix rust nightly version #331

Merged
merged 2 commits into from
Feb 21, 2024

Conversation

Xynnn007
Copy link
Member

  1. Fixes nightly lint error
  2. Follow the discussion from @portersrc , I fix the version of rust nightly compiler in GHA to avoid unexpected CI errors everytime new rust nightly version is released.

@mkulke
Copy link
Contributor

mkulke commented Feb 19, 2024

there is a case for testing against nightly: you will pick up changes early that will propagate to stable in later releases, like the last nightly breakage that we've seen.

@@ -25,7 +25,7 @@ jobs:
rust:
- stable
- beta
- nightly
- nightly-2024-02-18
Copy link
Contributor

Choose a reason for hiding this comment

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

why fixing nightly to an (arbitrary?) date, would that give us any additional confidence?

@portersrc
Copy link
Member

I made a few comments in confidential-containers/confidential-containers#193

I would just drop nightly @mkulke. rust-nightly doesn't necessarily hit stable (if I understand correctly), and even rust-beta is overhead we don't need until we're ready to bump to a stable version.

But FWIW, I'd say @Xynnn007's pinned nightly is better than nightly, b/c of the reasons he stated. Arbitrary date is fine.

@mkulke
Copy link
Contributor

mkulke commented Feb 19, 2024

I made a few comments in confidential-containers/confidential-containers#193

I would just drop nightly @mkulke. rust-nightly doesn't necessarily hit stable (if I understand correctly), and even rust-beta is overhead we don't need until we're ready to bump to a stable version.

But FWIW, I'd say @Xynnn007's pinned nightly is better than nightly, b/c of the reasons he stated. Arbitrary date is fine.

no, not everything that hits nightly will end up in stable, but more specifically: if something doesn't compile in nightly today it's likely that eventually it will also break in stable, because the original behaviour was based on a bug. not sure if this is a hard rule, but that has been my experience. So in this respect nightly can be a good indicator of upcoming changes that need be dealt with.

that said, I wouldn't mind dropping beta and nightly (I don't see the point in testing against some revision of nightly, but I might be missing something) from the matrix. I don't think the project is affected too much by those changes.

@Xynnn007
Copy link
Member Author

@mkulke Right. Nightly test will tell us the potential error asap. however, there might be cases where indirect dependency breaks, which will block the daily development. At the same time, if we update nightly version periodically, we also might need to handle a bunch of errors one time.

This is a every-day-a-little vs a-batch-one-time problem. I am okay with both tbh temporarily as they both has pros and cons. If finally it is decided not to fix nightly version, I will drop the commit in this PR.

@mkulke
Copy link
Contributor

mkulke commented Feb 19, 2024

yeah, this is mostly bikeshedding, so I while I have opinions, it probably does not matter 😅 but I'm intellectually curious: what good would pinning a nightly version does vs just dropping nightly from the matrix?

would there be a checkbox item on the release list that says "bump the nightly version and fix all potential errors?"

@fitzthum
Copy link
Member

Personally I think I lean more towards dropping nightly. We don't make any claims about supporting it. I don't see much benefit in testing with it.

@Xynnn007
Copy link
Member Author

good. So it seem clear that we do not need to check against nightly and beta anymore. Let me just delete them in this PR : )

@Xynnn007 Xynnn007 marked this pull request as ready for review February 20, 2024 01:52
@Xynnn007 Xynnn007 requested a review from sameo as a code owner February 20, 2024 01:52
@Xynnn007 Xynnn007 force-pushed the fix-ci branch 2 times, most recently from 1a6cb20 to 7d4f0ee Compare February 20, 2024 07:26
@Xynnn007 Xynnn007 requested review from mkulke and mythi February 20, 2024 07:59
Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

LGTM @Xynnn007 !

@portersrc
Copy link
Member

Thanks @Xynnn007 for fielding this. Maybe catch kbs-e2e and kbs-docker-e2e in this PR, as well.

We do not need check the code against nightly and beta version.

Also, delete the matrix and replace with env

Signed-off-by: Xynnn007 <[email protected]>
@Xynnn007 Xynnn007 merged commit f62ac44 into confidential-containers:main Feb 21, 2024
17 checks passed
@Xynnn007 Xynnn007 deleted the fix-ci branch February 21, 2024 02:05
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.

6 participants