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 toolchain: input so that rust-toolchain file will be used #823

Merged
merged 4 commits into from
Oct 17, 2022

Conversation

vivekvpandya
Copy link
Contributor

@vivekvpandya vivekvpandya commented Oct 6, 2022

As per https://github.com/marketplace/actions/rust-toolchain#the-toolchain-file
May close #780
As per discussion on https://lightrun.com/answers/actions-rs-toolchain-support-new-toml-format-for-rust-toolchain currently actions-rs has some issues using toolchain files. So used suggested solution there.

@vivekvpandya vivekvpandya added the s:review-needed The pull request requires reviews label Oct 6, 2022
@vivekvpandya vivekvpandya self-assigned this Oct 6, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

Ok, the "rustup show" looks like a dirty hot fix 😂. As I understand https://github.com/marketplace/actions/rust-toolchain#the-toolchain-file correctly, they just say, that the toolchain input can be removed to use this file.

In these cases the toolchain can be named in the project's directory in a file called rust-toolchain.toml or rust-toolchain.

When leaving out toolchain in the workflow files doesn't work, maybe rename the root file to rust-toolchain.toml and try again.

@vivekvpandya
Copy link
Contributor Author

Ok, the "rustup show" looks like a dirty hot fix joy. As I understand https://github.com/marketplace/actions/rust-toolchain#the-toolchain-file correctly, they just say, that the toolchain input can be removed to use this file.

In these cases the toolchain can be named in the project's directory in a file called rust-toolchain.toml or rust-toolchain.

When leaving out toolchain in the workflow files doesn't work, maybe rename the root file to rust-toolchain.toml and try again.

Not sure why that will work? I see moonbeam also using the hack "rustup show" https://github.com/PureStake/moonbeam/blob/master/.github/workflows/build.yml

@Chralt98
Copy link
Member

Ok, so you tried it without the toolchain input? I just thought: Why shouldn't it work, when actions-rs/toolchain directly explains, that it works with a toolchain file. But sure, we can go with rustup show. I asked, because this little hack could be misunderstood later.

@Chralt98
Copy link
Member

I researched a little bit and found this issue comment actions-rs/toolchain#126 (comment) . So yeah, sounds like this hack is used at the moment.

Chralt98
Chralt98 previously approved these changes Oct 17, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

OK

@maltekliemann
Copy link
Contributor

I don't get it. You provide this link: https://github.com/marketplace/actions/rust-toolchain#the-toolchain-file. Why use this workaround?

@vivekvpandya
Copy link
Contributor Author

I don't get it. You provide this link: https://github.com/marketplace/actions/rust-toolchain#the-toolchain-file. Why use this workaround?

It does not work as advertise for now, there is a bug and not fixed yet.

@maltekliemann
Copy link
Contributor

This issue? actions-rs/toolchain#126

@vivekvpandya
Copy link
Contributor Author

This issue? actions-rs/toolchain#126

Yes it seems so, here is relevant PR actions-rs/toolchain#166

Copy link
Contributor

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

It's not pretty, but it does seem to do what we need right now.

.github/workflows/benchmark.yml Show resolved Hide resolved
Co-authored-by: Malte Kliemann <mail@maltekliemann.com>
Comment on lines 36 to 39
# TODO(#XXX): `rustup show` installs the rustc version specified in the `rust-toolchain` file
# according to https://rust-lang.github.io/rustup/overrides.html. We don't use actions-rs due to the
# lack of support of `rust-toolchain` files with TOML syntax:
# https://github.com/actions-rs/toolchain/issues/126.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please fill in the XXX with the issue for this, and add this to the other two places where these changes are made? Oh, also I forgot to add indentation to the comment.

Suggested change
# TODO(#XXX): `rustup show` installs the rustc version specified in the `rust-toolchain` file
# according to https://rust-lang.github.io/rustup/overrides.html. We don't use actions-rs due to the
# lack of support of `rust-toolchain` files with TOML syntax:
# https://github.com/actions-rs/toolchain/issues/126.
# TODO(#XXX): `rustup show` installs the rustc version specified in the `rust-toolchain` file
# according to https://rust-lang.github.io/rustup/overrides.html. We don't use actions-rs due
# to the lack of support of `rust-toolchain` files with TOML syntax:
# https://github.com/actions-rs/toolchain/issues/126.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I missed that 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we create issue on our git repo?

Copy link
Contributor

@maltekliemann maltekliemann Oct 17, 2022

Choose a reason for hiding this comment

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

No worries. Yeah, let's open a GitHub issue just so we don't forget about removing the workaround when we get the chance.

Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

👍

@Chralt98 Chralt98 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Oct 17, 2022
@vivekvpandya vivekvpandya merged commit 2bc14c3 into main Oct 17, 2022
@vivekvpandya vivekvpandya deleted the ci_rust_toolchain branch October 17, 2022 15:48
@sea212 sea212 added this to the v0.3.6 milestone Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make CI use rust version specified in rust-toolchain
4 participants