Use $PWD and --manifest-path to select a default package in workspaces
#25
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarijnS95
force-pushed
the
workspace-default-package
branch
from
2b8ac40 to
d227346
Compare
MarijnS95
commented
Nov 25, 2022
| pub struct Workspace { | ||
| #[serde(default)] | ||
| pub default_members: Vec<String>, |
There was a problem hiding this comment.
Should start using this too.
MarijnS95
commented
Nov 25, 2022
MarijnS95
force-pushed
the
workspace-default-package
branch
2 times, most recently
from
3034a40 to
29f28ea
Compare
MarijnS95
commented
Nov 25, 2022
This was referenced Nov 26, 2022
MarijnS95
force-pushed
the
workspace-default-package
branch
2 times, most recently
from
d7f35e7 to
68b8a09
Compare
|
is libraryfication of cargo ever going to happen? I really whish we didn't have to rebuild these components |
No idea, would be cool. Even cooler would be a plugin system, where |
…paces This restores original, prevalent `--manifest-path` usage to select a package containing the APK crate in a `[workspace]`, instead of using `-p` for that.
When calling `.parent()` on a filename the result is `""`, which should be treated as PWD (`"."`) but `dunce::canonicalize()` ends up in a "No such file or directory".
`.ancestors()` only calls `.parent()` on a `Path` which walks up until the string empty, but this isn't the root of the filesystem if the path wasn't absolute.
MarijnS95
force-pushed
the
workspace-default-package
branch
from
68b8a09 to
89165f5
Compare
Instead of relying on non-workspace logic.
MarijnS95
force-pushed
the
workspace-default-package
branch
from
89165f5 to
2ade565
Compare
MarijnS95
commented
Dec 8, 2022
src/utils.rs
Outdated
Comment on lines
103
to
113
| // Find the closest member based on the given path | ||
| Ok(path | ||
| .ancestors() | ||
| // Move manifest out of the HashMap | ||
| .find_map(|dir| all_members.remove(dir)) | ||
| .unwrap_or_else(|| { | ||
| ( | ||
| workspace_manifest_path.to_owned(), | ||
| workspace_manifest.clone(), | ||
| ) | ||
| })) |
There was a problem hiding this comment.
This logic is slightly incomplete as it falls back to the package defined in the workspace (found in xbuild):
- Even if it doesn't contain a
[package](leading to a panic in the outer function that expects this to have been checked already); - When there's a manifest along the way that isn't the workspace, but it is not part of it (this is a proper error in
cargo).
I'll resolve these in xbuild first (where an identical setup to this PR has already been merged) and apply the same solution here afterwards.
There was a problem hiding this comment.
Fixed here too by replicating rust-mobile/xbuild#93!
|
I'll probably merge this tonight and push out a new |
dvc94ch
approved these changes
Dec 8, 2022
The previous implementation had one fatal flaw: having a non-workspace `Cargo.toml` in a subdirectory, with a `[workspace]` defined some directories higher is invalid when that `[workspace]` doesn't include the given `Cargo.toml` subdirectory/package. `cargo` rejects this with a `current package believes it's in a workspace when it's not`, and we should do the same instead of having an `unwrap_or_else` that falls back to using the workspace root `Cargo.toml` as a package (especially if it might not contain a `[package]` at all). This would for example fail when running `x new template` in the repo directory, `cd`'ing into it and invoking `x build`. Instead of complaining about `template/Cargo.toml` not being part of the workspace, it detects the workspace and falls back to building the root package because the `template` directory appears to just be a subdirectory of the root without defining a subpackage along it. Since our root doesn't define a `[package]`, the supposed-to-be-infallible `unwrap()` on `manifest.package` below fails.
MarijnS95
force-pushed
the
workspace-default-package
branch
from
f07e2bb to
9e8401e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes rust-mobile/cargo-apk#8
This restores original, prevalent
--manifest-pathusage to select a package containing the APK crate in a[workspace], instead of using-pfor that.