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

Imply --all argument for virtual manifest. #4021

Closed
matklad opened this issue May 10, 2017 · 2 comments · Fixed by #4335
Closed

Imply --all argument for virtual manifest. #4021

matklad opened this issue May 10, 2017 · 2 comments · Fixed by #4335
Labels
E-easy Experience: Easy

Comments

@matklad
Copy link
Member

matklad commented May 10, 2017

If I run cargo build in a workspace, I often get:

error: manifest path `/home/user/projects/fall/Cargo.toml` is a virtual manifest, but this command requires running against an actual package in this workspace

The fix is to run cargo build --all instead. I think Cargo can figure this out automatically?

If the current package is virtual, and we are running one of --all enabled commands, --all should be the default.

The fix would involve changing this logic in all relevant commands (there's quite a bit of duplication:( ).

@matklad matklad added the E-easy Experience: Easy label May 10, 2017
@alexcrichton
Copy link
Member

👍

@rsertelon
Copy link
Contributor

rsertelon commented May 10, 2017

I've started to try to implement this. However, as I said in #4016, I'm a rust beginner and my code might not be idiomatic ;)

What I've tried for now is creating a method in cargo::core::Workspace to know if it's virtual or not, then simply used this method in the check for the --all flag. Like so:

pub fn is_virtual(&self) -> bool {
    match *self.packages.get(&self.current_manifest) {
        MaybePackage::Package(..) => false,
        MaybePackage::Virtual(..) => true
    }
}

and then instead of

let spec = if options.flag_all {
 // ...
}

I have this:

let spec = if options.flag_all || ws.is_virtual() {
 // ...
}

Does it seem sound to you? Or is there a better way for the is_virtual() method?

At first I thought of using the existing current_opt() method (and use the is_none() method of Option), but I thought the meaning of it wouldn't be explicit enough and might cause bugs somehow one day...

rsertelon added a commit to rsertelon/cargo that referenced this issue May 10, 2017
debris pushed a commit to debris/cargo that referenced this issue Jul 27, 2017
bors added a commit that referenced this issue Jul 28, 2017
Rebased and fixed 4025: Apply --all if workspace is virtual

- fixes #4021
- rebased #4025
- fixed issue issue described by @matklad in #4025 (review)
- added test `build_virtual_manifest_one_project` which covers the fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Experience: Easy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants