feat: allow rust hooks to specify crate features #3235

Terseus · 2024-06-15T18:10:45Z

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

asottile

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

asottile · 2024-06-15T18:35:25Z

pre_commit/languages/rust.py

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


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

asottile · 2024-06-15T18:37:27Z

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

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 pre-commit#3230

asottile · 2024-06-15T20:49:20Z

pre_commit/languages/rust.py

+        if dep.startswith(FEATURE_PREFIX):
+            continue


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

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

asottile · 2024-06-15T20:49:45Z

pre_commit/languages/rust.py

@@ -130,6 +133,12 @@ def install_environment(
    cli_deps = {
        dep for dep in additional_dependencies if dep.startswith('cli:')
    }
+    # Features starts with "--feature="
+    features = {
+        dep.replace(FEATURE_PREFIX, '')


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 · 2024-06-15T20:50:17Z

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

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

Terseus · 2024-07-11T19:23:35Z

@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 · 2024-07-11T19:28:11Z

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 · 2024-07-11T20:33:48Z

@asottile

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

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

asottile · 2024-07-12T14:37:51Z

@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 · 2024-08-03T09:36:48Z

@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 · 2024-08-20T14:53:05Z

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 · 2024-08-20T15:40:11Z

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 · 2024-08-21T06:47:55Z

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 · 2024-08-21T12:54:22Z

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 · 2024-08-21T12:55:55Z

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 · 2024-08-21T13:02:28Z

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 · 2024-08-21T13:23:33Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow rust hooks to specify crate features #3235

feat: allow rust hooks to specify crate features #3235

Terseus commented Jun 15, 2024

asottile left a comment

asottile Jun 15, 2024

asottile commented Jun 15, 2024

asottile Jun 15, 2024

Terseus Jun 16, 2024

asottile Jun 15, 2024

Terseus Jun 16, 2024

asottile commented Jun 15, 2024

Terseus commented Jul 11, 2024

asottile commented Jul 11, 2024

Terseus commented Jul 11, 2024 •

edited

Loading

asottile commented Jul 12, 2024

Terseus commented Aug 3, 2024

2bndy5 commented Aug 20, 2024 •

edited

Loading

2bndy5 commented Aug 20, 2024

2bndy5 commented Aug 21, 2024 •

edited

Loading

asottile commented Aug 21, 2024

asottile commented Aug 21, 2024

chriskuehl commented Aug 21, 2024

2bndy5 commented Aug 21, 2024

feat: allow rust hooks to specify crate features #3235

Are you sure you want to change the base?

feat: allow rust hooks to specify crate features #3235

Conversation

Terseus commented Jun 15, 2024

asottile left a comment

Choose a reason for hiding this comment

asottile Jun 15, 2024

Choose a reason for hiding this comment

asottile commented Jun 15, 2024

asottile Jun 15, 2024

Choose a reason for hiding this comment

Terseus Jun 16, 2024

Choose a reason for hiding this comment

asottile Jun 15, 2024

Choose a reason for hiding this comment

Terseus Jun 16, 2024

Choose a reason for hiding this comment

asottile commented Jun 15, 2024

Terseus commented Jul 11, 2024

asottile commented Jul 11, 2024

Terseus commented Jul 11, 2024 • edited Loading

asottile commented Jul 12, 2024

Terseus commented Aug 3, 2024

2bndy5 commented Aug 20, 2024 • edited Loading

2bndy5 commented Aug 20, 2024

Why cargo add?

2bndy5 commented Aug 21, 2024 • edited Loading

asottile commented Aug 21, 2024

asottile commented Aug 21, 2024

chriskuehl commented Aug 21, 2024

2bndy5 commented Aug 21, 2024

Terseus commented Jul 11, 2024 •

edited

Loading

2bndy5 commented Aug 20, 2024 •

edited

Loading

Why `cargo add`?

2bndy5 commented Aug 21, 2024 •

edited

Loading