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

flag for cargo add to add platform specific dependencies #11720

Closed
bindsdev opened this issue Feb 15, 2023 · 9 comments · Fixed by #11826
Closed

flag for cargo add to add platform specific dependencies #11720

bindsdev opened this issue Feb 15, 2023 · 9 comments · Fixed by #11826
Assignees
Labels
A-documenting-cargo-itself Area: Cargo's documentation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-add

Comments

@bindsdev
Copy link
Contributor

bindsdev commented Feb 15, 2023

Problem

Currently, there is no support for adding platform specific dependencies (such as [target.'cfg(windows)'.dependencies]) using cargo add.

Proposed Solution

Add a --cfg option that allows the user to specify the whole desired cfg predicate.

Examples:

  • cargo add windows --cfg windows
  • cargo add raw_cpuid --cfg any(target_arch = "x86", target_arch = "x86_64")

Notes

  • Currently, since cfg dependency specifications only are supported using platform predicates, one might think that the option should be named differently. But, in case other cfg pairs/options become supported in the future, the name of the flag allows for it to also include those without confusion for its name being named after the fact it specifies platform specific dependencies only.
@bindsdev bindsdev added the C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` label Feb 15, 2023
@weihanglo weihanglo changed the title --cfg option for adding platform specific dependencies flag for cargo add to add platform specific dependencies Feb 16, 2023
@bindsdev
Copy link
Contributor Author

@rustbot claim

@epage
Copy link
Contributor

epage commented Feb 17, 2023

@akabinds FYI this has not been marked as Accepted and there are likely things still left to resolve, like what the flag should be called. Starting on this now might lead to extra churn during review as we settle those kind of questions.

@bindsdev
Copy link
Contributor Author

@akabinds FYI this has not been marked as Accepted and there are likely things still left to resolve, like what the flag should be called. Starting on this now might lead to extra churn during review as we settle those kind of questions.

I understand. I just wanted to claim it because I want to implement it if it is accepted. But, I had a quick question. I was looking around the code base for relevant code to help implement this. I found that this feature seems to already have tests in the test suite using the existing --target flag:

#[cargo_test]
fn target_cfg() {
init_registry();
let project = Project::from_template(curr_dir!().join("in"));
let project_root = project.root();
let cwd = &project_root;
snapbox::cmd::Command::cargo_ui()
.arg("add")
.arg_line("my-package1 my-package2 --target cfg(unix)")
.current_dir(cwd)
.assert()
.success()
.stdout_matches_path(curr_dir!().join("stdout.log"))
.stderr_matches_path(curr_dir!().join("stderr.log"));
assert_ui().subset_matches(curr_dir!().join("out"), &project_root);
}

But, when attempting to add platform-specific dependencies this way in my own project to test, it seems to not work. Has this been implemented already, but not present in a release?

@epage
Copy link
Contributor

epage commented Feb 17, 2023

Huh, I thought we had this; I should have checked rather than assuming the issue author hadn't missed it.

But, when attempting to add platform-specific dependencies this way in my own project to test, it seems to not work. Has this been implemented already, but not present in a release?

Could you provide more details as to what didn't work?

@bindsdev
Copy link
Contributor Author

bindsdev commented Feb 17, 2023

Could you provide more details as to what didn't work?

My shell was giving me an obscure error after running it. It seems like all I had to do was wrap the cfg predicate in quotations and it worked fine. But, I ran this to test:

> cargo add --target cfg(windows) serde
zsh: unknown file attribute: i

If I did --target 'cfg(windows)', it worked fine.

I'm not sure if this workaround is needed on other shells or if this is even an issue on other shells.

@weihanglo
Copy link
Member

I believe different shells have different ways to escape things. If that works for you, seems like this issue is not really an issue as cargo-add already supports that?

@bindsdev
Copy link
Contributor Author

I believe different shells have different ways to escape things. If that works for you, seems like this issue is not really an issue as cargo-add already supports that?

Indeed. But should it not be at least documented that quotations might be needed based on the shell?

@epage
Copy link
Contributor

epage commented Feb 17, 2023

That gets into an interesting line of how much do we document how to use each users system vs users knowing it being an expectation in using cargo.

@weihanglo
Copy link
Member

It helps. However, there may be more flags needing that hint. If we start doing so, does that imply we require all possible flags having that hint for consistency? Personally I would probably avoid that.

We do have --package getting that hint already, though it is a bit different: the doc mentions * which may expand to valid shell glob, which makes it harder to debug. In contrast, target cfg syntax usually errors out before cargo get executed.

$ cargo build --bin *
error: unexpected argument 'CODE_OF_CONDUCT.md' found

Usage: cargo build [OPTIONS]

For more information, try '--help'.

$ cargo add tokio --target cfg(windows)
bash: syntax error near unexpected token `('

To me, even the hint in --package/--exclude is a bit too verbose 😆.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` Command-add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants