mmstick / cargo-deb

A cargo subcommand that generates Debian packages from information in Cargo.toml
615 stars 50 forks source link

All crates are built in all cases (feature and target restrictions are not applied) #115

Open librelois opened 4 years ago

librelois commented 4 years ago

This behaviour is problematic.

I have a workspace with a lot of lib crates and one binary crate. I use cargo-deb with the --manisfest-path option to pack my binary crate.

I need to package for several target architectures (including amd64 and arm), and some lib crates in my workspace can only compile on some specific architectures.

Cargo-deb tries to compile all the crates of my workspace in any case, so the packaging failed.

I think the cause is here in the code : https://github.com/mmstick/cargo-deb/blob/master/src/lib.rs#L86-L90

/// Builds a release binary with `cargo build --release`
pub fn cargo_build(options: &Config, target: Option<&str>, other_flags: &[String], verbose: bool) -> CDResult<()> {
    let mut cmd = Command::new("cargo");
    cmd.current_dir(&options.manifest_dir);
    cmd.arg("build").args(&["--release", "--all"]);

Why don't you use the --manifest-path cargo option? Why do you add the --all option?

I think my problem would be solved by using the cargo manifest-path option and removing the call to the --all option

kornelski commented 4 years ago

I'm not sure why --all is there. It could be just a simplification/workaround for compatibility with workspaces.

I'm slightly worried that there may be crates that have assets table in Cargo.toml that depend on products from other crates in the workspace.

Is passing through --manisfest-path enough? Should it pass -p or --lib/--bin and such?

librelois commented 4 years ago

I'm not sure why --all is there. It could be just a simplification/workaround for compatibility with workspaces.

What is certain is that the removal of the --all option has solved my problem. But it forces me to use my cargo-deb fork: https://github.com/librelois/cargo-deb/commit/a7e2ad2957143aec31c19199a8e41d6ee723bb22

The cleanest way would be to delete the --all call, this flag can already be injected by users who need it. But it's a breaking change.

A non-breaking solution would be to keep the --all call by default but add an option to cargo-deb to disable the --all call.

I'm slightly worried that there may be crates that have assets table in Cargo.toml that depend on products from other crates in the workspace.

All the crates that a crate needs, must be in the Cargo.toml, in this case they are built even without the call to the --all option.

In the case of cargo-deb, the --all option only makes sense if you want to package all the binary crates in the workspace in a single command. However, this is not necessarily the case.

Is passing through --manisfest-path enough?

It's much cleaner than modifying the current_dir, so it would be nice to do it, but it's not enough.

Should it pass -p or --lib/--bin and such?

I tested with the -p option and it's not enough. What is your idea with --lib/--bin?

kornelski commented 4 years ago

In the case of cargo-deb, the --all option only makes sense if you want to package all the binary crates in the workspace in a single command.

I suspect that may be the case

I tested with the -p option and it's not enough. What is your idea with --lib/--bin?

I mean to ensure everything is built, instead of passing --all, cargo-deb should probably specify what exactly Cargo is supposed build.

librelois commented 4 years ago

I suspect that may be the case

Yes, sometimes, but not systematically :)

I mean to ensure everything is built, instead of passing --all, cargo-deb should probably specify what exactly Cargo is supposed build.

What is necessary and sufficient to build should be specified in the Cargo.toml file. Even if it means adding this information in [package.metadata.deb] section.