Skip to content

Conversation

@UebelAndre
Copy link
Collaborator

@UebelAndre UebelAndre commented Sep 8, 2020

This is a slight cleanup pull request but also adds the @io_bazel_rules_rust//rust/platform:unix and @io_bazel_rules_rust//rust/platform:wasm32-unknown-unknown targets

@UebelAndre UebelAndre mentioned this pull request Sep 8, 2020
@UebelAndre UebelAndre changed the title Platforms cleanup Add 'unix' platform Sep 9, 2020
@damienmg
Copy link
Collaborator

damienmg commented Sep 9, 2020

Ok the PR LGTM but I want to find a better title for the commit. Maybe Cleanup platform configurations? WDYT?

@UebelAndre
Copy link
Collaborator Author

@damienmg Is that preferred over the current commit message?

Add 'unix' platform

I'm happy to change it. I've definitely noticed I've gotten lazy in my commit messages 😅

@damienmg
Copy link
Collaborator

damienmg commented Sep 9, 2020

It not only add unix platform, but also fix the wasm32-unknown-unknown, this is not for the commit message, more for the actual message I'll put when I hit the merge button. I am fine with putting Add 'unix' platform but that does not seems to reflect what is done here.

@UebelAndre
Copy link
Collaborator Author

Oh man! I totally forgot I added that here, I'll close #402. Sorry about that, haven't been sleeping well 😅

@damienmg
Copy link
Collaborator

damienmg commented Sep 9, 2020

Anyway, what do you think about the name of this PR?

@UebelAndre UebelAndre changed the title Add 'unix' platform Cleanup platform configurations Sep 9, 2020
@damienmg
Copy link
Collaborator

damienmg commented Sep 9, 2020

Thanks LGTM!

@damienmg damienmg merged commit b798f14 into bazelbuild:master Sep 9, 2020
@UebelAndre
Copy link
Collaborator Author

@damienmg Is it the name of the PR that shows as the commit message? I've updated the name and top comment as well as the commit message for this PR. Thoughts?

@damienmg
Copy link
Collaborator

damienmg commented Sep 9, 2020

Yes if you create a PR with a single commit, the title and description of that PR is going to be the title and description of the commit (we can change it when we do the merge). If there is multiple commit, it create a bullet list with all the commit title as the commit description.

@UebelAndre UebelAndre deleted the platform_cleanup branch September 9, 2020 18:30
damienmg pushed a commit that referenced this pull request Sep 24, 2020
…#407)

This PR adds `rust_workspace` which is meant to define transitive dependencies as well as cleans up some duplicate files that were in the root of the project but only used by examples.

## Updated documentation
Note the updated documentation in [docs/index.md](https://github.com/UebelAndre/rules_rust/blob/workspace-structure/docs/index.md)

## Added `bazel_skylib` to `rust_repositories`
It appears there are some uses of `bazel_skylib` within these rules but there aren't really any guarantees that that repository is available. This may be my fault with the changes made in #399 to [@io_bazel_rules_rust//rust/platform:platform.bzl](https://github.com/bazelbuild/rules_rust/blob/master/rust/platform/platform.bzl#L3) but I feel the `bazel_skylib` dependency should be conditionally added

## Added `rust_workspace`
I feel this is much nicer on the users's end as it gives them a single place to go to look at dependencies and is nicer on the repo because it means dependencies can more easily updated. I'm not the biggest fan of the name `rust_workspace` so if this was an acceptable concept, I'm happy to take suggestions on what the name should be called.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants