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

Remove rust-toolchain.toml #2369

Closed
wants to merge 1 commit into from
Closed

Remove rust-toolchain.toml #2369

wants to merge 1 commit into from

Conversation

ethanuppal
Copy link
Member

Since #2364, which added rust-toolchain.toml, still had failing tests on main (https://github.com/calyxir/calyx/actions/runs/12151446330/job/33886064698), I don't think this is needed?

@polybeandip
Copy link
Collaborator

Apologies, that's my fault. Strangely, I passed CI on my branch, but failed on main; I didn't know there was a difference between those CI checks.

I think the problem was that I forgot to remove line 18 in playground.yml so it ends up ignoring rust-toolchain.toml and grabbing the latest verison?

I'm neutral on whether it should be removed; fine with either call

@ethanuppal
Copy link
Member Author

in #2365 I removed the root of the issue with a temporary hack. Not sure if we should just straight up enforce the Rust version.

@EclecticGriffin
Copy link
Collaborator

I think it is worth keeping the toolchain toml and just fixing the line that overrides it. It's better for us to mandate a version of rust for the CI rather than have the periodic breakage when a new lint or check gets introduced, since that makes things pretty brittle and causes a fair bit of chaos.

@EclecticGriffin
Copy link
Collaborator

This particular cycle of CI breakage is by no means the first time this has happened and is arguably attributable to not standardizing a single rust version for all of CI

Copy link
Collaborator

@EclecticGriffin EclecticGriffin left a comment

Choose a reason for hiding this comment

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

We should keep the file and just ensure it is used everywhere

@ethanuppal
Copy link
Member Author

ok

EclecticGriffin added a commit that referenced this pull request Dec 10, 2024
Supersedes #2369.

`rust-toolchain.toml` now determines the version of rust used for every
aspect of the CI, rather than having it change on its own or be set
separately. As it turns out, the version of rust used for the clippy
lints was also pinned separately, so I also had to address all those
lints. As a result, I had to make minor changes to a bunch of code, the
vast majority of which were either docstring indents or places where
people manually implemented `ToString` instead of using `Display`. I
don't anticipate any of these changes messing with things but if I did
break something, do let me know.

Assuming I've done things correctly, it should be the case that the rust
version will not change on its own anymore, and we can update the
version used by all of Ci by editing the toolchain file
@EclecticGriffin
Copy link
Collaborator

Closed in favor of #2374

@ethanuppal ethanuppal deleted the remove-rust-toolchain branch December 10, 2024 22:12
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.

3 participants