-
Notifications
You must be signed in to change notification settings - Fork 390
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
enable cargo workspace and path dependencies to work seamlessly #684
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also use we can also walk through all non-workspace packages, and if This will effectively make path dependencies zero-cost, as long as those paths are defined and shown with |
this is possibly an argument for using cargo as a library, but that locks us to the features of the specific cargo version that is used. |
I've created https://github.com/cross-rs/test-workspace to test against |
2efc53b
to
7860d47
Compare
bors try --target x86_64-unknown-linux-gnu |
tryBuild succeeded: |
README.md
Outdated
However, you may use Cargo's `--manifest-path` option to reference your | ||
target crate, executed from a common root directory from which all your | ||
dependencies are available. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modify
// TODO: Also filter out things that are in workspace, but not a workspace member | ||
self.non_workspace_members().filter_map(|p| p.crate_path()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this todo is not super important, but probably good to have since it minifies the mounts needed
it works! https://github.com/cross-rs/cross/runs/5873527788?check_suite_focus=true#step:7:1482 If anyone is able to review this before it's actually ready, please do <3 |
bors try |
tryBuild succeeded: |
7860d47
to
b1577ce
Compare
b777258
to
4bc0b65
Compare
bors try --target *unknown-linux-gnu |
tryBuild succeeded: |
Ensure we build the entire workspace using the `--workspace` flag both with and without the manifest path, and also ensure we build the binary with all our workspace dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks great and the updated tests all pass. I believe this is ready to merge. Should I r+
it?
bors r+ |
684: enable cargo workspace and path dependencies to work seamlessly r=Alexhuszagh a=Emilgardis This will allow cargo workspaces to work seamlessly with cross, assuming they don't reference anything outside the current workspace root. This change also automatically mounts (if needed) path dependencies. If they do reference things outside the workspace root, and that fact is not know to `cargo metadata` (e.g, it's not a workspace member or a path dependency), you'll have to mount it with ```toml [build.env] volumes = ["THING"] ``` and set `$THING="/path/to/thing"` (we need to work on accessibility for this feature too, a lot of reported issues are solved by it) I'm sure there is an issue directly related to workspaces, but I can't find any. fixes #388 Co-authored-by: Emil Gardström <[email protected]> Co-authored-by: wngr <[email protected]> Co-authored-by: Alexander Huszagh <[email protected]>
Build failed: |
`"${@}"` is an unbound variable on Bash 3.2, while `"$@"` is not, and Bash 3.2 is the pre-installed version on macOS.
bors try --target x86_64-apple-darwin |
tryBuild succeeded: |
bors r+ |
Build succeeded: |
This will allow cargo workspaces to work seamlessly with cross, assuming they don't reference anything outside the current workspace root. This change also automatically mounts (if needed) path dependencies.
If they do reference things outside the workspace root, and that fact is not know to
cargo metadata
(e.g, it's not a workspace member or a path dependency), you'll have to mount it withand set
$THING="/path/to/thing"
(we need to work on accessibility for this feature too, a lot of reported issues are solved by it)I'm sure there is an issue directly related to workspaces, but I can't find any.
fixes #388