feat: allow rust hooks to specify crate features#3235

Terseus

Now the rust hooks can use the parameter additional_dependencies to receive build-time features for the crate.

The features must start with the string "--feature=" following the name of the feature; e.g. "--feature=full".

Fixes #3230

asottile

please rebase out the unrelated formatting changes -- those are not welcome

I think we can probably just filter out anything that starts with -- or doesn't have :?

asottile

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

I don't think this should be here at all?

You're right, the features never reach this function.

I don't think you should need a replace(...) -- we should be able to pass along arguments verbatim I would hope?

What I've implemented uses a custom syntax, so e.g. --feature=foo --feature=bar is translated as --features foo bar when invoking Cargo.
If I understood you correctly you want to receive arbitrary Cargo arguments in additional_dependencies so we could pass, for example, --features foo bar --locked.

Is that what you're suggesting?

In that case, every value in additional_dependencies after a flag will be interpreted as a Cargo argument, because after a --features flag we cannot distinguish between a feature name or a dependency name.

asottile

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

Terseus

@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.

asottile

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=... ?

Terseus

@asottile

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

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

asottile

@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 ?

Terseus

@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.

2bndy5

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

2bndy5

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?

2bndy5

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.

asottile

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

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.

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

chriskuehl

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.

2bndy5

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 😜

Terseus

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

asottile

@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

asottile reviewed Jun 15, 2024

View reviewed changes

Terseus force-pushed the feature/3230-pass-features-to-cargo branch from 19f46ae to bd153a6 Compare June 15, 2024 18:55

Terseus requested a review from asottile June 15, 2024 19:06

asottile reviewed Jun 15, 2024

View reviewed changes

allow rust hooks to specify arbitrary cargo install parameters …

558793b

We've replaced the previous "--feature=" format with a single parameter "--" which marks the start of the "cargo install" arguments, so now we can use "-- --features foo", "-- --locked", etc. Fixes pre-commit#3162

2bndy5 mentioned this pull request Aug 21, 2024

migrate rust language's addition-dependencies to use cargo install #3278

Open

hofbi mentioned this pull request Jun 7, 2025

fix: cargo install uses Cargo.lock for installation #3481

Closed

+                  # Features starts with "--feature="
+                  features = {
+                      dep.replace(FEATURE_PREFIX, '')
+                      for dep in additional_dependencies
+                      if dep.startswith(FEATURE_PREFIX)
+                  }

		if dep.startswith(FEATURE_PREFIX):
		continue

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: allow rust hooks to specify crate features#3235