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

Specify minimum supported Rust version in fuels-rs manifest #243

Merged
merged 2 commits into from
May 7, 2022

Conversation

mitchmindtree
Copy link
Contributor

This ensures that users are warned in the case that their installed Rust version predates the version required by fuels-rs.

This should be particularly useful for users building forc-generated test harnesses, as I believe we've already had a couple of reports of strange compile errors that just required a Rust version update.

You can find documentation on this field here.


Originally I had planned on exposing this minimum required Rust version from a function in the crate root like so. The idea was that forc could use this function to specify the necessary rust-version field in the generated test harness manifest files.

However after thinking on this a little more, I realised it should be enough to simply specify the minimum required version here as fuels will be a dependency within the generated test harness anyway, meaning the user will be notified either way. As a result, I believe this closes FuelLabs/sway#1239.

@mitchmindtree mitchmindtree added enhancement New feature or request tech-debt Improves code quality or safety labels Apr 29, 2022
This ensures that users are warned in the case that their installed Rust
version predates the version required by `fuels-rs`.

This should be particularly useful in `forc`-generated harnesses, as I
believe we've already had a couple of reports of strange compile errors
that just required a Rust version update.

You can find documentation on this field [here][1]:

[1]: https://doc.rust-lang.org/cargo/reference/manifest.html#the-rust-version-field
adlerjohn
adlerjohn previously approved these changes May 1, 2022
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

LGTM, but will defer to @digorithm

@mitchmindtree
Copy link
Contributor Author

I'm just realising we should probably have another CI check that uses the minimum specified version to cargo check the workspace and make sure we don't actually require a newer version?

I'm unsure whether or not specifying the rust-version like this will cause cargo to automatically error on use of language features that became available after the specified version - I suspect that it won't.

ra0x3
ra0x3 previously approved these changes May 1, 2022
digorithm
digorithm previously approved these changes May 2, 2022
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

I'm unsure whether or not specifying the rust-version like this will cause cargo to automatically error on use of language features that became available after the specified version - I suspect that it won't.

Yeah, that would be very weird. I think it should be fine.

* add ci check to ensure toolchains match
@Voxelot Voxelot dismissed stale reviews from digorithm, ra0x3, and adlerjohn via a38cb6f May 5, 2022 07:10
@ra0x3
Copy link

ra0x3 commented May 5, 2022

@adlerjohn Can you help me understand why we did this, this way?

  • I'm assuming fuels-rs is going to be present in forc, so we just put the requirement in fuels-rs ?
  • Why would we not just put this in forc?

(I have no answers here, I'm just curious)

@Voxelot
Copy link
Member

Voxelot commented May 5, 2022

@ra0x3 fuel-rs is independent of forc. For example in our faucet app, we use the SDK to send genesis coins to users without interacting with contracts or forc at all. Forc is a sway app dev tool, but SDK usecases can go far beyond that.

@adlerjohn
Copy link
Contributor

Put another way, fuels-rs is independent of the particular high level language used.

@Voxelot Voxelot mentioned this pull request May 7, 2022
@adlerjohn adlerjohn merged commit 7ff5415 into master May 7, 2022
@adlerjohn adlerjohn deleted the mitchmindtree/rust-version branch May 7, 2022 03:34
@mitchmindtree
Copy link
Contributor Author

Thanks for finishing this off / landing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tech-debt Improves code quality or safety
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forc test should check Rust version
5 participants