Skip to content

Improve Rust workspace ergonomics#6324

Closed
mkrasnitski wants to merge 1 commit into
Vector35:rust_cleanup_0from
mkrasnitski:workspace
Closed

Improve Rust workspace ergonomics#6324
mkrasnitski wants to merge 1 commit into
Vector35:rust_cleanup_0from
mkrasnitski:workspace

Conversation

@mkrasnitski
Copy link
Copy Markdown
Contributor

Right now, running cargo doc --open in the root of the workspace generates docs for all members, but only opens the ones for arch/msp430. This might be confusing to devs. If we introduce a default-members field to the workspace, then unless the --workspace flag is given to cargo, only those members will be used when running cargo {check,build,doc}.

IMO this makes it more obvious that the workspace is just a DX thing, and makes it more explicitly opt-in. Note that this essentially mimics having the binaryninja package as the root package in the workspace, where this behavior would be the default (see here). Also, since binaryninjacore-sys is a dependency of binaryninja, it'll always be pulled in so its inclusion in the list is not strictly necessary.

Also, cargo was putting all its artifacts in /target, but it would be better to keep that directory from polluting the project root, so I added a local cargo config file that moves it back into the rust subdir.

@emesare
Copy link
Copy Markdown
Member

emesare commented Jan 19, 2025

I am not in favor of restricting default members to just the wrapper crate. We have the workspace so that you must build all rust code in the api repository.

I am also not in favor of moving the target dir into a subdirectory. This would require adding documentation so that people are not confused as to where they are suppose to find build artifacts.

I did however see the issue with documentation opening to msp430 crate first, I updated the README to show how to open to the wrapper crate.

See the commit: 05b7a18

@mkrasnitski
Copy link
Copy Markdown
Contributor Author

I feel like rust/target is a good default place for artifacts, no? Compared to a build dir that gets generated for cpp, target is less obvious a name to indicate its purpose, and since those artifacts will be unique to rust code, putting them in rust/ seems fine to me. Also, the other workspace members will put their artifacts in that directory regardless of where it lives. If you build one of the plugins, would you rather the artifacts be put in /target, or /rust/target? Also, the build.rs for binaryninja assumes the location of the target dir, which either way should get corrected:

let _ = std::fs::create_dir("target");
let _ = std::fs::create_dir("target/doc");
let _ = std::fs::copy("../docs/img/favicon.ico", "target/doc/favicon.ico");
let _ = std::fs::copy("../docs/img/logo.png", "target/doc/logo.png");

@emesare
Copy link
Copy Markdown
Member

emesare commented Jan 20, 2025

I feel like rust/target is a good default place for artifacts, no? Compared to a build dir that gets generated for cpp, target is less obvious a name to indicate its purpose, and since those artifacts will be unique to rust code, putting them in rust/ seems fine to me.

That is the rust bindings directory. Not where all rust code lives. For example we have a riscv architecture plugin that lives separate in the arch directory. The reason the builds should not be in the /rust directory is because it is not the workspace directory, therefore the changes you propose are analogous to making /rust the workspace directory. I feel as though that obscures the relationship between the rust api and the plugins. The relationship being that the plugins depend on the rust api and are not the rust api (which is what moving the target directory would necessitate).

As for why I am so against this, I want to keep divergent behavior to a minimum, that is, if someone who has no clue of the project structure were to clone the repository, they would not be confused to see the target directory be generated. They would be confused to see that the target directory exist in the rust/ directory, especially when they did not touch the rust/ directory and instead were touching the arch, plugins or view directories.

let _ = std::fs::create_dir("target");
let _ = std::fs::create_dir("target/doc");
let _ = std::fs::copy("../docs/img/favicon.ico", "target/doc/favicon.ico");
let _ = std::fs::copy("../docs/img/logo.png", "target/doc/logo.png");

Good catch, will update with the next round of commits.

@mkrasnitski mkrasnitski deleted the workspace branch January 20, 2025 00:49
@emesare emesare self-assigned this Feb 3, 2025
@emesare emesare added this to the Gallifrey milestone Apr 21, 2025
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.

2 participants