Skip to content

cargo install should respect [patch] from user config #12855

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

Open
loynoir opened this issue Oct 19, 2023 · 11 comments
Open

cargo install should respect [patch] from user config #12855

loynoir opened this issue Oct 19, 2023 · 11 comments
Labels
A-configuration Area: cargo config files and env vars A-patch Area: [patch] table override C-bug Category: bug Command-install

Comments

@loynoir
Copy link

loynoir commented Oct 19, 2023

Problem

I want to install XXX with a patch.

/home/user/.cargo/config.toml

[patch.crates-io]
XXX = { path = '/home/user/git-cloned-XXX-with-patch' }

cargo install XXX is still installing the original version.

Steps

No response

Possible Solution(s)

cargo install should respect patch

Notes

No response

Version

cargo 1.73.0
@loynoir loynoir added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Oct 19, 2023
@epage epage changed the title cargo install should respect patch cargo install should respect [patch] from user config Oct 19, 2023
@epage epage added Command-install A-configuration Area: cargo config files and env vars A-patch Area: [patch] table override labels Oct 19, 2023
@weihanglo
Copy link
Member

Cargo generally starts a config discovery from the current working directory.

However, it is not the case for cargo install. cargo install as a user-level not project-level command (at least for now), it reload config file from you CARGO_HOME.

It makes sense to me for cargo install to respect [patch] from user config, as it already respects build.rustflags config. ~/.cargo/config.toml is often seen as a config for the user. Yet I didn't look closer enough to identify why it doesn't work.

@weihanglo weihanglo added S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Mar 1, 2024
@ehuss ehuss added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review and removed S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Apr 8, 2025
@ehuss
Copy link
Contributor

ehuss commented Apr 8, 2025

The cargo team discussed this, and we didn't have any specific reasons to not support that.

There was some confusion as to why this isn't working, but I just looked and understood the reason is that patch only works on dependencies during resolution. cargo install fetching the top-level thing (the thing you are installing) does not use the resolver for anything, and thus patch is completely out of the equation. There would need to be special-case code for it to look at the patch table and to try to resolve it. I'm not sure how complex or difficult that would be (the mechanisms used here are quite different). If it is unusually difficult, then we may not want to support that.

I'm marking this accepted conditional on how complex the solution might be.

@epage
Copy link
Contributor

epage commented Apr 9, 2025

Oh, I had missed that this was for patching what you install and not what it depends on.

@loynoir could you help me understand the use case for this? I would have assumed that if you want to install something from somewhere else, you would just specify that to cargo install but that you asked for this means I am missing something.

@loynoir
Copy link
Author

loynoir commented Apr 10, 2025

According to my memory, I was installing cli tool.

And that cli tool need to download asset using maybe-non-reqwest builder with hard code timeout, which leads to cli failure within my side.

So, I git clone repo, and patch that hard code timeout number.

@loynoir
Copy link
Author

loynoir commented Apr 10, 2025

The original real world problem is patch hard-code-timeout-cli.

So, the best solution is not supporting path to patched large dir, but supporting patch-dir/XXX.patch pattern.

I currently don't have good example of famous nodejs repo using repo/patches/XXX.patch pattern supported by pnpm and yarn.

https://github.com/configu/configu/blob/main/patches/giget.patch

@epage
Copy link
Contributor

epage commented Apr 10, 2025

If I'm understanding correctly, you were installing a tool that you needed to patch.

What is the reason for wanting to use [patch] with cargo install some-cli instead of cargo install --path ../some-cli-patched?

patch-dir/XXX.patch pattern.

I currently don't have good example of famous nodejs repo using repo/patches/XXX.patch pattern supported by pnpm and yarn.

Yes, there is interest in supporting patch files and we are tracking that in #4648. However, they would likely be managed through [patch] and so if something doesn't work with [patch] it probably won't work with patch files. I'm not sure what semantics you are fully trying to convey with "patch-dir pattern".

@loynoir
Copy link
Author

loynoir commented Apr 12, 2025

What is the reason for wanting to use [patch] with cargo install some-cli instead of cargo install --path ../some-cli-patched?

Too long, and not able to reuse reasonable hard code timeout patch.

I'm not sure what semantics you are fully trying to convey with "patch-dir pattern".

Zero config pnpm and yarn auto apply every patches/XXX.patch when install.

This is pnpm yarn project layout default.

Would be nice into cargo project layout default.

https://doc.rust-lang.org/cargo/guide/project-layout.html

@epage
Copy link
Contributor

epage commented Apr 14, 2025

Too long, and not able to reuse reasonable hard code timeout patch.

I feel like I'm missing something about that last part. How is cargo install XXX with a patch more reusable than cargo install --path XXX or cargo install --git XXX?

Zero config pnpm and yarn auto apply every patches/XXX.patch when install.

Could you describe this solution within pnpm for someone without any node background and then share how you think that could then be applied to cargo?

@loynoir
Copy link
Author

loynoir commented Apr 14, 2025

I feel like I'm missing something about that last part. How is cargo install XXX with a patch more reusable than cargo install --path XXX or cargo install --git XXX?

Within devcontainer

$ cargo install --path /workspaces/repo/workaround/optional-dx-cli-with-timeout-patch

Within new devcontainer

$ cargo install --path /workspaces/repo/workaround/optional-dx-cli-with-timeout-patch

I would prefer longer config, shorter cli for reuse.

Don't need to remember the path.

Patch transparently.

Decrease mind burden.

[patch.crates-io]
optional-dx-cli = { path = '/workspaces/repo/workaround/optional-dx-cli-with-timeout-patch' }

Within devcontainer

$ cargo install optional-dx-cli

Within new devcontainer

$ cargo install optional-dx-cli

Could you describe this solution within pnpm for someone without any node background and then share how you think that could then be applied to cargo?

I don't have small popular real world patch example near my hand.

According to my memory, I would suggest like below.

https://yarnpkg.com/cli/patch

https://yarnpkg.com/cli/patch-commit

https://github.com/search?q=org%3Ayarnpkg%20path%3Apatches&type=code

Given background

  • In ideal world, project depends on hello@cratesio:1.0.0 and world@cratesio:1.0.0, and project does not depends on foo@*

  • project path location is /workspaces/repo

  • hello@cratesio:1.0.0 depends on foo@cratesio:1.0.0

  • world@cratesio:1.0.0 depends on foo@cratesio:2.0.0

  • foo@cratesio:1.0.0 and foo@cratesio:2.0.0 have bug need to patch.

Start patching

$ cargo yarn patch foo
[Show hint like]
`foo` have multiple versions, select one
$ cargo yarn patch foo@cratesio:1.0.0
[Show hint like]
`foo@cratesio:1.0.0.zip` now decompressed to `/tmp/xfs-foo-cratesio:1.0.0-hash/`.
Please modify `/tmp/xfs-foo-cratesio:1.0.0/`.
After that, run `cargo yarn patch-commit /tmp/xfs-foo-cratesio:1.0.0-hash/`
...
$ cargo yarn patch foo@cratesio:2.0.0
...
$ cargo yarn patch-commit /tmp/xfs-foo-cratesio:1.0.0-hash/
[Show hint like]
Writing `/workspaces/repo/patches/foo-cratesio:1.0.0-hash.patch`
Writing `/workspaces/repo/Cargo.lock`
...
$ cargo yarn patch-commit /tmp/xfs-foo-cratesio:2.0.0-hash/
...
.
├── Cargo.lock
├── Cargo.toml
├── src/
│   ├── lib.rs
│   ├── main.rs
│   └── bin/
│       ├── named-executable.rs
│       ├── another-executable.rs
│       └── multi-file-executable/
│           ├── main.rs
│           └── some_module.rs
├── benches/
│   ├── large-input.rs
│   └── multi-file-bench/
│       ├── main.rs
│       └── bench_module.rs
├── examples/
│   ├── simple.rs
│   └── multi-file-example/
│       ├── main.rs
│       └── ex_module.rs
├── patches/
│   ├── foo-cratesio:1.0.0-hash.patch
│   └── foo-cratesio:2.0.0-hash.patch
└── tests/
    ├── some-integration-tests.rs
    └── multi-file-test/
        ├── main.rs
        └── test_module.rs

Before yarn patch commit, Cargo.toml

  • project depends on hello@cratesio:1.0.0 and world@cratesio:1.0.0

  • hello@cratesio:1.0.0 depends on foo@cratesio:1.0.0

  • world@cratesio:1.0.0 depends on foo@cratesio:2.0.0

After yarn patch commit, Cargo.toml not changed, but Cargo.lock

  • project depends on hello@patch:hash and world@patch:hash

  • hello@patch:hash depends on foo@cratesio:1.0.0#/workspaces/repo/patches/foo-cratesio:1.0.0-hash.patch

  • world@patch:hash depends on foo@cratesio:2.0.0#/workspaces/repo/patches/foo-cratesio:2.0.0-hash.patch

@epage
Copy link
Contributor

epage commented Apr 14, 2025

Within devcontainer

Thanks for expanding on the use case!

If I'm understanding correctly, you are creating many dev containers that all have access to your host system's ~/.cargo/config.toml and the patched repo and you want a convenient way of installing the same set of patched binaries to these dev containers as you spin them up.

Maybe I'm missing something but this seems relatively specialized and there are alternative ways of resolving (scripts, aliases). Granted, there still might be value due to consistency / level of surprised.

After yarn patch commit, Cargo.toml not changed, but Cargo.lock

This is showing the workflow for patching a dependency. For installing a binary, I assume you would want us to also have a ~/.cargo/patches to affect cargo install X. Like patches in ~/.cargo/config.toml, I assume this would also apply to all dependencies.

For local patches, I wonder if we should load from ROOT/patches or ROOT/.cargo/patches and if we should consider it project configuration like Cargo.tomls [patches] (which would then be weird to have in ~/.cargo) or environment configuration like .cargo/config.tomls [patches] (which would then not always be applied).

To reiterate, this would all be blocked on #4648.

@epage
Copy link
Contributor

epage commented Apr 14, 2025

Hmm, thinking more about the experience of [patch] applying to the lookup of foo with cargo install foo

(1) If a user puts a [patch] in ~/.cargo/config.toml, then

  • Every workspace that doesn't use that [patch] will warn about an unused patch on every command, not just when changing dependencies
  • The unused patch will be mentioned in Cargo.lock which is not ideal when that file is committed
  • cargo install --locked will be broken

See #15130

So it seems like patching dependencies of cargo installed packages is not well supported and I worry about making more use of ~/.cargo/config.tomls [patch] table with the current state its in.

We could provide an install-specific config location but then we'd get these warnings between different cargo-installed commands. We could have a [install.patch].

(2) The experience of lining up a patch with dependencies is not great. If we kept the existing behavior for cargo install, we'd look for the latest version of the package if one wasn't specified and then look for a patch that matches that version. So if a new version comes out, the patch won't be applied. This is where unused patch warnings can be helpful except how do we know which warnings to show and which not to, even with [install.patch]. The warning can also be easily lost in the volume of output from cargo install. We could make cargo install special and look for patches and pick from their versions. This lack of consistency could be confusing though.

@ehuss ehuss removed the S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review label Apr 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-patch Area: [patch] table override C-bug Category: bug Command-install
Projects
None yet
Development

No branches or pull requests

4 participants