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

Fix rust-project.json generation in subpackages #724

Merged
merged 4 commits into from
May 10, 2021

Conversation

hlopko
Copy link
Member

@hlopko hlopko commented May 6, 2021

This PR fixes a bug in gen_rust_project where we assumed that rust-project.json is generated directly under bazel-bin. For rust_analyzer targets deeper in the workspace tree that is not correct and this PR correctly descents into the right package in bazel-bin to find the rust-project.json.

I'm amazed how well I nerdsniped myself into writing a label parsing library in Rust. How hard can it be, though I. Foolish me.

@google-cla google-cla bot added the cla: yes label May 6, 2021
@hlopko hlopko force-pushed the extend_rust_analyzer branch 3 times, most recently from a12b540 to 4e77255 Compare May 6, 2021 09:27
@hlopko hlopko marked this pull request as ready for review May 6, 2021 09:55
@hlopko
Copy link
Member Author

hlopko commented May 6, 2021

CC @djmarcin

deps = [":label_error"],
)

rust_library(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why make this two different targets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm a fan of small targets. I agree that this might be over the line :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I too am a fan but don't think each module needs to be a separate Bazel target. I'd opt to combine these unless you feel they're isolated enough that it warrants being separated

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this PR.

Just throwing my two cents in on naming -- I like small targets but I think rust makes it un-ergonomic due to how it handles crate imports.

The first problem is that small targets are prone to naming conflicts -- any other package named label would result in needing to alias one of them so that you could use the right one in your code.

The second problem (not really unique to small targets) is that the name gives absolutely no indication of where a package comes from. This isn't a problem if everything just comes from crates.io, but it is a problem if it comes from some directory in a monorepo. In order to figure out where the relevant code is you have to look at the BUILD file, which isn't ideal.

One workaround is to name these targets with their full path, e.g. name = "util_label" so that when used in rust it looks like use util_label which gives you an indication of where it exists in your directory tree, but it's unsatisfying to have to do this in your BUILD files (and prone to getting out of date if you move targets around). Another idea would be for the bazel rules themselves to embed this directory information into the crate name, but that's a rather large breaking change to the existing rules. This probably isn't the right place to have this discussion, but food for (future) thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I merged label with label_error.

I agree that there is a tension between small crates and unique names, and changing the rules to include package name in crate name crossed my mind in the past. I consider this problem not yet solved.

util/label/label.rs Outdated Show resolved Hide resolved
util/label/label.rs Outdated Show resolved Hide resolved
util/label/label.rs Outdated Show resolved Hide resolved
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.

4 participants