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

For errors, gently suggest rustup check #784

Merged
merged 4 commits into from
Sep 23, 2022
Merged

Conversation

nealmcb
Copy link
Contributor

@nealmcb nealmcb commented Sep 22, 2022

Fixes #783

Signed-off-by: Neal McBurnett nealmcb@gmail.com

Fixes fermyon#783

Signed-off-by: Neal McBurnett <nealmcb@gmail.com>
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Great catch - thanks! I reckon we should also mention this on the Rust page: https://github.com/fermyon/spin/blob/main/docs/content/rust-components.md - otherwise all grand.

We should have a think about how to ensure the versioning information remains correct. Maybe pin CI to a specific Rust version and add a check that the docs match CI. (That's definitely out of scope for this PR! I'm just thinking out loud.)

question: is version 1.56 still recent enough?
Is there a reason for picking that version?


Signed-off-by: Neal McBurnett <nealmcb@gmail.com>
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

@nealmcb
Copy link
Contributor Author

nealmcb commented Sep 23, 2022

Thanks, @itowlson. I clarified the rust-components.md page a bit. I note they already mentioned Rust versioning, and they mention 1.56+ which is actually older than what i was running.

Given that this project is moving quickly, we could just remove the specific version number recommendations and suggest getting a "recent" version.

@itowlson
Copy link
Contributor

We could, though I quite like the specific minimum, as it avoids ambiguity. As the one who got bitten by the issue, do you think "check for a recent version" would have helped enough, or would you have preferred "check for 1.56+"?

@nealmcb
Copy link
Contributor Author

nealmcb commented Sep 23, 2022

I think just mentioning that rust versions are something to check is fine. I doubt that maintainers of the code base will enjoy trying to keep recommendations for specific person numbering up to date, until there's a 1.0 release.

@itowlson
Copy link
Contributor

I doubt that maintainers of the code base will enjoy trying to keep recommendations for specific version numbering up to date

can confirm grin

So on that basis, would you like us to merge this PR as it stands, or to reword, or to close this one and have us pick it up?

Until 1.0?

Signed-off-by: Neal McBurnett <nealmcb@gmail.com>
Signed-off-by: Neal McBurnett <nealmcb@gmail.com>
@nealmcb
Copy link
Contributor Author

nealmcb commented Sep 23, 2022

I dare say that this, without the version numbers, is best for now.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Yes, I like that - specific action but low-maintenance - thanks for the careful thought on this. Looks good to me; is it okay to merge?

@nealmcb
Copy link
Contributor Author

nealmcb commented Sep 23, 2022

Go for it. Thanks!

@itowlson itowlson merged commit a4bcbd6 into fermyon:main Sep 23, 2022
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.

Note which Rust versions are supported
2 participants