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

workspace_path can now act as a Bazel label within a Bazel workspace #185

Merged
merged 4 commits into from Aug 24, 2020
Merged

workspace_path can now act as a Bazel label within a Bazel workspace #185

merged 4 commits into from Aug 24, 2020

Conversation

UebelAndre
Copy link
Collaborator

This change makes it so that the Cargo.toml file can live within the recommended location and cargo-raze will output content into a Bazel workspace in a way that better matches the expectations set by Bazel's naming conventions

├── BUILD
├── Cargo.lock
│
├── Cargo.toml  <- HERE
│
├── README.md
├── cargo
│   │
│   ├-   <- INSTEAD OF HERE
│   │
│   └── libloading_global_static.BUILD
└── src
    └── main.rs

This makes it easier to transition Rust projects to start using Bazel since users no longer have to move their Cargo.toml files and the use of workspace_path (which looks like a Label) makes more sense. Getting started with Bazel/cargo-raze should now only require users to add the [raze] section to their existing Cargo.toml file and run cargo raze (assuming they're using the remote genmode, but otherwise could vendor the packages and do the same thing).

The smoke-tests show the up to date examples but I wasn't sure if I should update the actual examples with the generated results of the smoke-tests. Anyway, this is my first ever PR in Rust and am looking forward to the feedback 😄

@UebelAndre
Copy link
Collaborator Author

@damienmg are you able to take a look at this or ping someone who can?

@yesudeep
Copy link

I've been using symlinks for this and put the cargo directory in //third_party/cargo.
This should help!

@UebelAndre
Copy link
Collaborator Author

Yeah, I used symlinks to put my Cargo.toml file in the correct place such that rust-analyzer would work. But I feel this is a worth while change. I think cargo-raze shouldn't force users to move their Cargo.toml files. Especially since debugger support for rust through Bazel is kinda spotty. It's nice to be able to lean on the current rust tooling/ecosystem until the Bazel tooling matures enough to completely replace it.

Copy link
Collaborator

@dfreese dfreese left a comment

Choose a reason for hiding this comment

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

@UebelAndre sorry for the delay, and thanks for the PR. I've gone through and reviewed the code based on the current implementation.

I know that some of my teammates have also symlinked the Cargo.toml and Cargo.lock of our project into the workspace root to be more compatible with RLS, so I'm definitely open to exploring this sort of change, however, would there be a way to do it in a way that wouldn't be a breaking change? One thought would be a flag to opt-in to this behavior.

impl/src/bin/cargo-raze.rs Outdated Show resolved Hide resolved
impl/src/planning.rs Outdated Show resolved Hide resolved
impl/src/planning.rs Show resolved Hide resolved
impl/src/bazel.rs Outdated Show resolved Hide resolved
impl/src/bazel.rs Outdated Show resolved Hide resolved
impl/src/bazel.rs Outdated Show resolved Hide resolved
impl/src/bazel.rs Outdated Show resolved Hide resolved
@damienmg
Copy link
Member

@damienmg are you able to take a look at this or ping someone who can?

Sorry for the delay I tend to not look at Github outside of working hours (especially I am not even allowed to work on sundays in my country).

@UebelAndre UebelAndre requested a review from dfreese July 30, 2020 12:49
@UebelAndre
Copy link
Collaborator Author

UebelAndre commented Jul 30, 2020

@UebelAndre sorry for the delay, and thanks for the PR. I've gone through and reviewed the code based on the current implementation.

@dfreese Hey, no problem at all :D

I know that some of my teammates have also symlinked the Cargo.toml and Cargo.lock of our project into the workspace root to be more compatible with RLS, so I'm definitely open to exploring this sort of change, however, would there be a way to do it in a way that wouldn't be a breaking change? One thought would be a flag to opt-in to this behavior.

I added an incompatible_relative_workspace_path (names are hard 😜 ) flag to allow for "opt-in" behavior. I feel strongly that this should at some point become the default behavior which is why I updated the smoke-tests and went with an incompatible flag.

@yesudeep
Copy link

yesudeep commented Aug 5, 2020

I know that some of my teammates have also symlinked the Cargo.toml and Cargo.lock of our project into the workspace root to be more compatible with RLS

This breaks on Windows, which is why something that doesn't depend on symlinks may be a better idea.

@UebelAndre
Copy link
Collaborator Author

@dfreese Hey, got time this week to do another review? 😅

@UebelAndre
Copy link
Collaborator Author

Maybe @smklein or @acmcarther might have some time if @dfreese is not available? 😅

@UebelAndre
Copy link
Collaborator Author

Hey @damienmg, don't mean to be a bother but do you know how I can get some review support for this PR? 😅

@UebelAndre
Copy link
Collaborator Author

@dfreese Hey, I noticed you made changes to this repo recently. Do you think you could take a look at my PR as well? Or if not just let me know when you might have time?

@damienmg
Copy link
Member

Hi @UebelAndre,

Unfortunately this is not a good time for me, but I'll try to take a look this week. Let see also if @mfarrugi have some bandwidth

@damienmg damienmg closed this Aug 24, 2020
@damienmg damienmg reopened this Aug 24, 2020
@damienmg
Copy link
Member

(sorry for the misclick)

@mfarrugi
Copy link
Collaborator

This looks fine to me, and addresses the earlier comments. I don't think I've reviewed anything else for this codebase, though.

Copy link
Member

@damienmg damienmg left a comment

Choose a reason for hiding this comment

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

I also did a pass and see no issue with this code, I consider this one as good to go then seeing how many times it has been reviewed.

Thanks a lot for your patience!

@damienmg damienmg merged commit eac33e6 into google:master Aug 24, 2020
@UebelAndre UebelAndre deleted the dev/workspace-root branch August 24, 2020 15:15
samschlegel pushed a commit to discord/cargo-raze that referenced this pull request Aug 26, 2020
…oogle#185)

This change makes it so that the `Cargo.toml` file can live within the [recommended location](https://doc.rust-lang.org/cargo/guide/project-layout.html) and `cargo-raze` will output content into a [Bazel workspace](https://docs.bazel.build/versions/master/build-ref.html#workspace) in a way that better matches the expectations set by [Bazel's naming conventions](https://docs.bazel.build/versions/master/build-ref.html)
```
├── BUILD
├── Cargo.lock
│
├── Cargo.toml  <- HERE
│
├── README.md
├── cargo
│   │
│   ├-   <- INSTEAD OF HERE
│   │
│   └── libloading_global_static.BUILD
└── src
    └── main.rs
```

This makes it easier to transition Rust projects to start using Bazel since users no longer have to move their `Cargo.toml` files and the use of `workspace_path` (which looks like a [Label](https://docs.bazel.build/versions/master/build-ref.html#labels)) makes more sense. Getting started with Bazel/cargo-raze should now only require users to add the `[raze]` section to their existing `Cargo.toml` file and run `cargo raze` (assuming they're using the remote genmode, but otherwise could vendor the packages and do the same thing). 

The smoke-tests show the up to date examples but I wasn't sure if I should update the [actual examples](https://github.com/google/cargo-raze/tree/master/examples) with the generated results of the smoke-tests.
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

5 participants