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

Switch to use manifest-path for cargo multi #2

Merged
merged 2 commits into from
Nov 8, 2016
Merged

Conversation

cswindle
Copy link
Owner

Instead of setting the current directory, provide manifest-path to the command. This is to ensure that any references to files in the output are relative to the current directory.

Copy link

@bossmc bossmc left a comment

Choose a reason for hiding this comment

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

Seems generally inelegant to me (though it looks like it works). Mostly aesthetic comments inline.

// Take a clone of the commands so that the manifest can be passed to the
// cargo command, this is to any references to files in the output are relative
// to the current directory.
let mut commands = commands.clone();
Copy link

Choose a reason for hiding this comment

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

You can take commands by move to avoid having to do this clone.

banner = banner + " " + arg;
}
}
let banner = format!("Executing {} {}", CARGO, commands.join(" "));

announce(&banner);
Copy link

Choose a reason for hiding this comment

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

Won't this be wrong? It won't have --manifest-file in it unlike the real deal.


for arg in commands {
cargo_cmd.arg(arg);
}
Copy link

Choose a reason for hiding this comment

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

Actually, can't you just do:

cargo_cmd,arg("--manifest-path");
cargo_cmd.arg(...path sruff...);
for arg in &commands {
    cargo_cmd.arg(arg);
}

Then you don't need to own commands at all.

@cswindle cswindle merged commit a514c94 into master Nov 8, 2016
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