-
Notifications
You must be signed in to change notification settings - Fork 402
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
Allow specifying rust versions #307
Conversation
Before I merge this, can you share what your use-case for this was? It looks like you want to be able to use an older version of rustc, is that right? The support for beta and nightly is also somewhat disingenuous, since it would only work for the handful of shas listed in |
@mfarrugi I wanted to easily build with a newer rust version (1.42.0) than the default. I didn't actually need the nightly or beta versions but it looked like it would be easy to add support for that as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks this change looks good but I will let Marco opine on if something needs to change.
From what I understand, this only lets you choose between versions that we have sha's for because of Bazel's sha requirement, no? I have no problem with this PR, but it doesn't look like it solves your problem. |
@mfarrugi if the shas aren't in the list it just doesn't enforce them: rules_rust/rust/repositories.bzl Line 266 in 0deef6d
Maybe the docs should say that changing the value to one that's not included in the shipped list will make your build non-hermetic. |
So you've tested this works? The docs imply it might error on empty string rather than None. Feel free to update the doc, although I'm not too paranoid about the rust tool distros. |
The default is an empty string, right?
But yeah, I'm running my local builds with this patch and setting the version to 1.42.0. |
I don't think the default argument ever makes it through before this change, but didnt look into it. Thanks! |
No description provided.