GeezerVison

we don't do conventional commits either, please just use a normal commit message

ideally I'd also like this solution to solve #3162

@asottile I've pushed a new version that addresses #3162 too.

Now instead of using the syntax "--feature=" in "additional_dependencies" we take all the arguments after an "--" as arbitrary arguments for "cargo install".

Please check it out when you can.

hmmm can it work if we just treat things starting with -- as options instead? or are there things in cargo that explicitly don't work with --foo=... ?

@asottile

or are there things in cargo that explicitly don't work with --foo=... ?

Features, they have the syntax "--features feat1 feat2 feat3".

@asottile

or are there things in cargo that explicitly don't work with --foo=... ?

Features, they have the syntax "--features feat1 feat2 feat3".

afaict that's not valid:

$ cargo install exa --features git vendored-openssl
    Updating crates.io index
  Downloaded exa v0.10.1
  Downloaded 1 crate (136.6 KB) in 0.10s
    Updating crates.io index
error: could not find `vendored-openssl` in registry `crates-io` with version `*`
  Installing exa v0.10.1
    Updating crates.io index
...

it'd have to be --features 'foo bar' or --features foo,bar -- which maybe we can just require specifically --features=foo,bar ?

@asottile
You're right, it's --features "git vendored-openssl" or --features git,vendored-openssl, however I don't see what can we gain by accepting only --features=foo,bar.

The current implementation can accept any cargo install param, long or short; I think that's okay.

The current implementation can accept any cargo install param, long or short;

What about --manifest-path <path/to/Cargo.toml> or --bin <bin-name>? The current command does not seem well suited to cargo workspaces:

An unexpected error has occurred: CalledProcessError: command: ('\\.cargo\\bin\\cargo.EXE', 'install', '--bins', '--root', '\\AppData\\Local\\Temp\\tmpyqboytse\\repoczr9yu0q\\rustenv-system', '--path', '.')
return code: 101
stdout: (none)
stderr:
    error: found a virtual manifest at `\AppData\Local\Temp\tmpyqboytse\repoczr9yu0q\Cargo.toml` instead of a package manifest

I tried pre-commit try-repo using changes in this PR on my rust project (a cargo "virtual" workspace), but it didn't work because all additional-dependencies are passed to cargo add. I need a way to change the args to cargo install.

Why cargo add?

Personally speaking, it doesn't make sense to pass features to the cargo add command because a well-formed Cargo.toml can adjust the dependency tree (of a binary build) according to the enabled/disabled features passed to a cargo install command. AFAIK, the cargo add command is strictly for project development, I don't think it has any usage in project deployment.

If a rust-based binary executable needs to make shell calls to other cargo-built binaries, then the cargo install is more suitable for that (or maybe a custom build script that performs cargo install when PRE_COMMIT env var is set). Adding libs to a project's manifest doesn't seem to achieve anything for deployment. Am I missing a use case or something?

This is exactly what the feature selection for cargo install aims to resolve. I really think there is a misconception that led to using cargo add. cargo install builds binaries. cargo add simply adds dependency specs to a manifest (Cargo.toml), it doesn't build binaries.

in 2024 yeah cargo add here doesn't make much sense here -- but it's evolved over time and was originally added in 2018 when features were available but not necessarily ubiquitous. the idea is that some packages had arbitrary pluggability and adding dependencies would unlock that -- in practice I don't think many actually implemented things this way and went for a more "static" pluggability -- but at least that was the idea

I'm all for killing the cargo add bits -- but some investigation should be done first to make sure nothing public is using those functionalities and then a deprecation and removal period. it's also a little off topic for this patch

You're right, it's --features "git vendored-openssl" or --features git,vendored-openssl, however I don't see what can we gain by accepting only --features=foo,bar.

personally I'd rather be more constrained such that if we need to do something later we have the opportunity to do so rather than being stuck with "allow arbitrary arguments" decision today. not to mention completely arbitrary arguments likely allows one to break out of the intended isolation

pre-commit/pre_commit/languages/rust.py

Lines 124 to 129 in 0f8f383

	# Unlike e.g. Python, if we just `cargo install` a library, it won't be
	# used for compilation. And if we add a crate providing a binary to the
	# `Cargo.toml`, the binary won't be built.
	#
	# Because of this, we allow specifying "cli" dependencies by prefixing
	# with 'cli:'.

This is exactly what the feature selection for cargo install aims to resolve. I really think there is a misconception that led to using cargo add. cargo install builds binaries. cargo add simply adds dependency specs to a manifest (Cargo.toml), it doesn't build binaries.

The reason behind this originally was to allow you to override specific library versions used to build the hook binary if needed, similar to how you can use additional_dependencies to pin a transitive library version for Python. It wasn't really intended for feature selection. That said, I certainly haven't kept up with Cargo in the six years since this was merged (and maybe it was even wrong in the first place) so I don't object to any changes.

I opened #3278. Thanks for the historic rationale. The cargo docs don't really preserve historic info like that, so I was completely unaware. I've only been using rust for about a year, but you could probably tell that already 😜

@asottile Should we close this PR in favor of #3278? If so, please feel free to do it.

@asottile Should we close this PR in favor of #3278? If so, please feel free to do it.

I think they're a little unrelated to each other