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.

Sign up for GitHub

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 confusing error and docs wrt. virtual manifests #4492

Merged
merged 4 commits into from
Sep 14, 2017
+35 −24
Merged

Fix confusing error and docs wrt. virtual manifests #4492

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8139a5d
Clarify docs on workspace members
RalfJung Sep 14, 2017
4e5bfc9
update --help text for new implicit --all behavior
RalfJung Sep 14, 2017
56274af
fix confusing error for virtual manifest with no member
RalfJung Sep 14, 2017
682af71
get rid of Workspace::current() usage in cargo_{compile,doc}
RalfJung Sep 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/bin/bench.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ the current package is benchmarked. For more information on SPEC and its format,
see the `cargo help pkgid` command.

All packages in the workspace are benchmarked if the `--all` flag is supplied. The
`--all` flag may be supplied in the presence of a virtual manifest.
`--all` flag is automatically assumed for a virtual manifest.
Note that `--exclude` has to be specified in conjunction with the `--all` flag.

The --jobs argument affects the building of the benchmark executable but does
not affect how many jobs are used when running the benchmarks.
Expand Down
2 changes: 1 addition & 1 deletion src/bin/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ current package is built. For more information on SPEC and its format, see the
`cargo help pkgid` command.

All packages in the workspace are built if the `--all` flag is supplied. The
`--all` flag may be supplied in the presence of a virtual manifest.
`--all` flag is automatically assumed for a virtual manifest.
Note that `--exclude` has to be specified in conjunction with the `--all` flag.

Compilation can be configured via the use of profiles which are configured in
Expand Down
4 changes: 4 additions & 0 deletions src/bin/check.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ which indicates which package should be built. If it is not given, then the
current package is built. For more information on SPEC and its format, see the
`cargo help pkgid` command.

All packages in the workspace are checked if the `--all` flag is supplied. The
`--all` flag is automatically assumed for a virtual manifest.
Note that `--exclude` has to be specified in conjunction with the `--all` flag.

Compilation can be configured via the use of profiles which are configured in
the manifest. The default profile for this command is `dev`, but passing
the --release flag will use the `release` profile instead.
Expand Down
3 changes: 2 additions & 1 deletion src/bin/doc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ By default the documentation for the local package and all dependencies is
built. The output is all placed in `target/doc` in rustdoc's usual format.

All packages in the workspace are documented if the `--all` flag is supplied. The
`--all` flag may be supplied in the presence of a virtual manifest.
`--all` flag is automatically assumed for a virtual manifest.
Note that `--exclude` has to be specified in conjunction with the `--all` flag.

If the --package argument is given, then SPEC is a package id specification
which indicates which package should be documented. If it is not given, then the
Expand Down
3 changes: 2 additions & 1 deletion src/bin/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ current package is tested. For more information on SPEC and its format, see the
`cargo help pkgid` command.

All packages in the workspace are tested if the `--all` flag is supplied. The
`--all` flag may be supplied in the presence of a virtual manifest.
`--all` flag is automatically assumed for a virtual manifest.
Note that `--exclude` has to be specified in conjunction with the `--all` flag.

The --jobs argument affects the building of the test executable but does
not affect how many jobs are used when running the tests. The default value
Expand Down
5 changes: 2 additions & 3 deletions src/cargo/core/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,8 @@ impl<'cfg> Workspace<'cfg> {
/// indicating that something else should be passed.
pub fn current(&self) -> CargoResult<&Package> {
self.current_opt().ok_or_else(||
format!("manifest path `{}` is a virtual manifest, but this \
command requires running against an actual package in \
this workspace", self.current_manifest.display()).into()
format!("manifest path `{}` contains no package: The manifest is virtual, \
and the workspace has no members.", self.current_manifest.display()).into()
Copy link
Member

Choose a reason for hiding this comment

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

I would be surprised if only a single tests needs to be fixed after this change. However, if this is the case, that probably means that current function is almost not used anymore, and it might be a good idea to try to get rid of it altogether: probably all commands should work with virtual manifests just fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

cargo test says this is the only one that needs fixing.

The code flow is a little spaghetti-esque currently, but I didn't want to change that as I have no idea how this is all architectured. Before #4335, when calling cargo build, what happens is that Packages::from_flags returns Packages::Packages(&Vec::new()), which then leads in compile_ws (at

cargo/src/cargo/ops/cargo_compile.rs

Line 234 in bf0b81a

let root_package = ws.current()?;
) to calling current(), which errored on virtual manifests. However, it seems we can also end up in that branch of there is a workspace but it is empty.
With #4335 merged, that's in fact the only way to end up there, as all will always be set (unless -p is given). I have no idea if there are other ways to have compile_ws call current, or if other places call current; grepping for such calls is doesn't really work due to the rather generic name of the function.

Looking at into_package_id_specs (which has to return an empty list to end up in this branch of compile_ws), the only way it can return empty is if ws.members() is empty or if there was no -p flag and this is not a workspace -- so, it seems actually that in compile_ws, that branch will always trigger an error. If you want I can change that, and then see if there are other users of current remaining, but notice that I have no idea what I am doing here. The question is do you trust the test suite coverage enough to let me still do such changes.^^

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out my analysis is incorrect. For non-virtual packages, we can still get Packages::Packages(&[]), and then it still uses that other path. current will however of course always succeed, then.

I suppose one could change from_flags to not ever return Packages::Packages(&[]), but instead fill in the current package name there or so; not sure how that would look.

Copy link
Member

Choose a reason for hiding this comment

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

So turns out there are a handful usages left:

Function
    current
Found usages  (8 usages found)
    cargo.ops  (8 usages found)
        cargo_compile.rs  (1 usage found)
            234 let root_package = ws.current()?;
        cargo_doc.rs  (1 usage found)
            31 let root_package = ws.current()?;
        cargo_package.rs  (3 usages found)
            30 let pkg = ws.current()?;
            201 let pkg = ws.current()?;
            279 let pkg = ws.current()?;
        cargo_pkgid.rs  (1 usage found)
            13 None => ws.current()?.package_id(),
        cargo_run.rs  (1 usage found)
            17 0 => ws.current()?,
        registry.rs  (1 usage found)
            42 let pkg = ws.current()?;

Originally, ws.current() appeared as a stop gap to make workspace implementation feasible: with .current(), it was possible to make Cargo workspace aware incrementally. So it would be great to get rid of it altogether some time in the future, but probably not right now.

It seems to me that cargo publish still may hit this function from virtual manifest, and so this error message might be wrong in that case (and presumably this is not tested :( ).

So... I think we definitely should update the documentation, however, I am not sure that updating this particular error message is correct for all cases... The simplest tactical fix would be to add a if ws.members().is_empty() check, and emit either old or new error message.

The strategic fix (which should not be part of this PR) would be to look at the usages of current and try to get rid of them: I think those insidecargo_package are unnecessary because it seems to me that we'll have a current package higher in the call stack anyway, and those in cargo_compile, cargo_run and cargo_doc should probably be folded into into_package_id_specs function.

)
}

Expand Down
6 changes: 4 additions & 2 deletions src/doc/manifest.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,10 +457,12 @@ as:
```toml
[workspace]

# Optional key, inferred if not present
# Optional key, inferred from path dependencies if not present.
# Additional non-path dependencies that should be included must be given here.
# In particular, for a virtual manifest, all members have to be listed.
members = ["path/to/member1", "path/to/member2", "path/to/member3/*"]

# Optional key, empty if not present
# Optional key, empty if not present.
exclude = ["path1", "path/to/dir2"]
```

Expand Down
13 changes: 3 additions & 10 deletions tests/workspaces.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,20 +696,13 @@ fn virtual_build_no_members() {
let p = project("foo")
.file("Cargo.toml", r#"
[workspace]
"#)
.file("bar/Cargo.toml", r#"
[project]
name = "bar"
version = "0.1.0"
authors = []
"#)
.file("bar/src/main.rs", "fn main() {}");
"#);
p.build();
assert_that(p.cargo("build"),
execs().with_status(101)
.with_stderr("\
error: manifest path `[..]` is a virtual manifest, but this command \
requires running against an actual package in this workspace
error: manifest path `[..]` contains no package: The manifest is virtual, \
and the workspace has no members.
"));
}

Expand Down