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 DEFAULT_RUST_EDITION #1256

Merged
merged 2 commits into from Apr 25, 2022
Merged

Conversation

dtolnay
Copy link
Contributor

@dtolnay dtolnay commented Apr 11, 2022

This is 1 commit on top of #1255. It removes DEFAULT_RUST_EDITION as the default edition argument of rust_repositories and various other places. Instead, if the downstream WORKSPACE does not pass an edition to rust_repositories, the edition becomes mandatory in every rust_* target in their workspace, rather than using an arbitrary fallback default defined within rules_rust (previously 2018).

This approach was suggested by @dfreese in #1255 (comment) and +1 by @UebelAndre in #1255 (review). I am ambivalent between them and would be happy with either this or #1255 by itself.

rust/private/rust.bzl Outdated Show resolved Hide resolved
@dtolnay
Copy link
Contributor Author

dtolnay commented Apr 11, 2022

Sorry about all the force pushes -- it took a bit of iteration with CI to get all the platform-specific and "manual" targets filled in correctly.

I figure we do not want to set a default edition in the top-level WORKSPACE.bazel because doing so would mask problems such as #1254, so some new edition attrs are required in this PR at the target level. I have extracted that commit to #1257 in case it's easier to review separately and because it's mergeable separately from this PR.

Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

I feel like there's something I'm not thinking about regarding the impact of this change but what I've come up with seems like there's good guidance on how to maintain the current behavior. So this looks good to me, give or take a few nits, thanks!

rust/private/rust.bzl Outdated Show resolved Hide resolved
@@ -55,6 +54,5 @@ rust_common = struct(
crate_info = CrateInfo,
dep_info = DepInfo,
stdlib_info = StdLibInfo,
default_edition = DEFAULT_RUST_EDITION,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it's worth leaving this for a little bit to reduce the impact to public API? I'm torn though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you see as being the intended use of this API?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have any strong answers. Just noticing we're deleting a value from rust_common and thought maybe there should be more comms about this. But it can also be a note in the next release.

@UebelAndre
Copy link
Collaborator

@dtolnay can you rebase this one so we can get it merged, and then #1255?

@dtolnay
Copy link
Contributor Author

dtolnay commented Apr 25, 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.

None yet

2 participants