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

Add ARCHITECTURE.md #596

Merged
merged 2 commits into from
Feb 23, 2021
Merged

Add ARCHITECTURE.md #596

merged 2 commits into from
Feb 23, 2021

Conversation

hlopko
Copy link
Member

@hlopko hlopko commented Feb 20, 2021

In this file I hope to describe how we think about the architecture
of rules_rust. It's goal is to help contributors orient themselves, and to
document code restrictions and assumptions.

@hlopko
Copy link
Member Author

hlopko commented Feb 20, 2021

Also r? @UebelAndre :)

@hlopko
Copy link
Member Author

hlopko commented Feb 20, 2021

Comments and contributions are very welcome! This is just a first draft. I haven't written anything about many packages such as proto or bindgen.

@UebelAndre
Copy link
Collaborator

😄 Thanks for starting this! I think it looks good so far.

Does this mean you're looking to move ./util into ./rust/tools?

I'm also particularly curious about cargo. I think I like it as it's own package but I can also see it as part of rust/tools. Your thoughts?

@dfreese
Copy link
Collaborator

dfreese commented Feb 22, 2021

Looks like a solid start. Thanks for putting that together.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

Looks good!

ARCHITECTURE.md Outdated Show resolved Hide resolved
ARCHITECTURE.md Outdated Show resolved Hide resolved
@hlopko
Copy link
Member Author

hlopko commented Feb 22, 2021

Does this mean you're looking to move ./util into ./rust/tools?

Yup (or //rust/private/tools, depending on the tool, I haven't thought about where tools should go).

I'm also particularly curious about cargo. I think I like it as it's own package but I can also see it as part of rust/tools. Your thoughts?

If you're asking about cargo build script runner, I'm open to suggestions as I haven't looked into how it is used and how the rule using it works. If there's not strong reason why it should be in //rust, I'd vote to not put it there.

@hlopko hlopko force-pushed the add_architecture_md branch from 36adaca to 269dd8b Compare February 22, 2021 13:51
@hlopko
Copy link
Member Author

hlopko commented Feb 22, 2021

So right now we say //rust is core package, everything else is less so. One alternative is to have a //contrib package that would contain all the "less so" packages. Opinions?

@UebelAndre
Copy link
Collaborator

If you're asking about cargo build script runner, I'm open to suggestions as I haven't looked into how it is used and how the rule using it works. If there's not strong reason why it should be in //rust, I'd vote to not put it there.

To my knowledge, the build script runner is there to run build.rs scripts which is a concept in Cargo, not rustc. So I think it makes more sense to leave it in the cargo directory. I think I like the existence of a cargo directory because it can act as the separation of cargo and rustc. Notably, #598 and #590 are also something I feel should go into cargo since they build on cargo manifests to generate Bazel repository definitions.

@damienmg
Copy link
Collaborator

If you're asking about cargo build script runner, I'm open to suggestions as I haven't looked into how it is used and how the rule using it works. If there's not strong reason why it should be in //rust, I'd vote to not put it there.

To my knowledge, the build script runner is there to run build.rs scripts which is a concept in Cargo, not rustc. So I think it makes more sense to leave it in the cargo directory. I think I like the existence of a cargo directory because it can act as the separation of cargo and rustc. Notably, #598 and #590 are also something I feel should go into cargo since they build on cargo manifests to generate Bazel repository definitions.

Yes cargo_build_script is named like this and in a different package because it is an interface with cargo not with the rust toolchain.

@hlopko hlopko force-pushed the add_architecture_md branch from a07c312 to c5e1479 Compare February 23, 2021 13:46
@hlopko
Copy link
Member Author

hlopko commented Feb 23, 2021

I've created #601 and #601 to track cleanups relevant to this PR.

@hlopko hlopko merged commit 32ad192 into bazelbuild:main Feb 23, 2021
@hlopko hlopko deleted the add_architecture_md branch February 23, 2021 13:54
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.

5 participants