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

Add the ability to rename a crate inherited from the workspace #12546

Open
fluiderson opened this issue Aug 23, 2023 · 15 comments
Open

Add the ability to rename a crate inherited from the workspace #12546

fluiderson opened this issue Aug 23, 2023 · 15 comments
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@fluiderson
Copy link

Problem

Consider the following part with workspace dependencies from root Cargo.toml:

[workspace.dependencies]
itoa = "1"
itoa-old = { version = "0.4", package = "itoa" }

And parts with dependencies from 2 workspace members:

# member-1/Cargo.toml
[dependencies]
itoa.workspace = true
# member-2/Cargo.toml
[dependencies]
itoa = { workspace = true, package = "itoa-old" }

Before checking/compiling member-2, Cargo prints this warning:

warning: /home/user/ws/member-2/Cargo.toml: unused manifest key: dependencies.itoa.package

And hence ignores the package key from member-2's Cargo.toml.

So, unless I missed something in the documentation, Cargo can't rename dependencies inherited from the workspace.

Proposed Solution

Make Cargo read the package key in inherited dependencies, and rename them as it does with usual dependencies.

Notes

No response

@fluiderson fluiderson added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Aug 23, 2023
@epage
Copy link
Contributor

epage commented Aug 23, 2023

When renaming, in addition to the dependency name, you specify a package of a different name. What I'm not too sure about is taking the package field and treating it as a dependency name to look up the actual package name.

@fluiderson
Copy link
Author

I mean, in the case of an inherited dependency, Cargo could use the package field as a pointer to the package name in the workspace, not to the actual package name.

Something like this:

flowchart RL
    subgraph Workspace
    itoa-old -- "package = #quot;itoa#quot;" --> itoaN[itoa 0.4]
    end
    subgraph member-2
    itoa -- "package = #quot;itoa-old#quot;" --> itoa-old
    end
Loading

@epage
Copy link
Contributor

epage commented Aug 24, 2023

My concern is that that isn't the package's name in the workspace but a dependency's name and I feel a little uncomfortable co-mingling the two.

@VlaDexa
Copy link

VlaDexa commented Aug 24, 2023

It's not necessary to mingle the names, the problem is that you can't rename the crates at all, if you are trying to inherit them from the workspace.

@real-felix
Copy link

I have a related issue (I think). I have a bunch of crates in my workspace:

# ./Cargo.toml
foo = { git = "https://github.com/awasome/qux", branch = "master" }
bar = { git = "https://github.com/awasome/qux", branch = "master" }

Then in a crate:

# ./my-crate/Cargo.toml
bar = { package = "cool-bar", workspace = true }

But the current cargo won't accept this.

I'm ready to having a try at this, if it's not too hard for a newcomer. I assume it's not, it's should be fairly simple to add, right?

@fluiderson
Copy link
Author

fluiderson commented Sep 21, 2023

Yes, it may be simple, but I guess this change should've a better design. As was stated above, using the package field for referencing not only the actual package name but also the name of a dependency may be confusing. Perhaps we can introduce a new field? Something like:

[dependencies]
itoa = { workspace = true, name = "itoa-old" }

And @Boiethios, seems like bar & cool-bar in your second listing should be swapped.

@epage
Copy link
Contributor

epage commented Sep 21, 2023

Haven't thought this through but one idea

[dependencies]
itoa = { workspace = "itoa-old" }

@avkonst
Copy link

avkonst commented Oct 11, 2023

Haven't thought this through but one idea

[dependencies]
itoa = { workspace = "itoa-old" }

This is what I tried without checking docs, assuming it works as expected. But it did not :(

@avkonst
Copy link

avkonst commented Oct 11, 2023

Can this solution be applied and merged? #12546 (comment)

@fluiderson
Copy link
Author

I can try to implement this, but someone from the team should approve this design first.

@avkonst
Copy link

avkonst commented Oct 16, 2023

Do you know who to ping from the design team?

@fluiderson
Copy link
Author

@epage?

@epage
Copy link
Contributor

epage commented Oct 16, 2023

If you want to see the relevant team, it is here (yes, I'm on it).

My idea was just for brainstorming purposes. People immediately jumped on it without giving it any critical thought. That critical thought is what is needed to drive this forward. In thinking about this a bit more, I at least would first want to see what other needs we come up with for workspace inheritance to make sure we don't design ourselves into a corner, Here, we propose making workspace accept a dependency name but could we want another meaning from that more generally? Its too early to say.

Example future expansions that could possibly get in the way

  • Workspace inheritance
  • Named groups of inheritable content (rather than monolithic inheritable content)
  • Other fields needing slightly different specialization and the two not playing well together

I would also be interested in any thoughts from @Muscraft considering they've put a lot of thought into this since they worked on it.

@epage epage added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-triage Status: This issue is waiting on initial triage. labels Oct 16, 2023
@epage
Copy link
Contributor

epage commented Oct 16, 2023

btw the obvious workaround for this is to just use the rename within your package (I assume that works)

# member-2/Cargo.toml
[dependencies]
itoa-old.workspace = true

For people pressing this, the most relevant information would be why the workaround is not sufficient in your specific case.

@epage epage added the A-workspace-inheritance Area: workspace inheritance RFC 2906 label Oct 16, 2023
@avkonst
Copy link

avkonst commented Oct 16, 2023 via email

fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this issue May 18, 2024
Changes:
- Pull all dependencies to the workspace and add a CI check.
- Fixup the crates that had differing dependency aliases.
- Add CI check to keep deps in the workspace
- Add Taplo config for TOML formatting (follow up: add CI check)

This is a preliminary to updating to the new SDK version as we currently
use conflicting aliases for a few packages while packages are not able
to rename workspace deps, see
rust-lang/cargo#12546

This is a No-OP change, as can be verified with this:
```bash
git checkout oty-fix-dependencies
cargo tree -e features > oty.tree

git checkout origin/main
cargo tree -e features > main.tree

diff main.tree oty.tree
```

- [x] Does not require a CHANGELOG entry

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: ordian <noreply@reusable.software>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this issue Jun 5, 2024
Inherited workspace dependencies cannot be renamed by the crate using
them (see [1](rust-lang/cargo#12546),
[2](https://stackoverflow.com/questions/76792343/can-inherited-dependencies-in-rust-be-aliased-in-the-cargo-toml-file)).
Since we want to use inherited workspace dependencies everywhere, we
first need to unify all aliases that we use for a dependency throughout
the workspace.
The umbrella crate is currently excluded from this procedure, since it
should be able to export the crates by their original name without much
hassle.

For example: one crate may alias `parity-scale-codec` to `codec`, while
another crate does not alias it at all. After this change, all crates
have to use `codec` as name. The problematic combinations were:
- conflicting aliases: most crates aliases as `A` but some use `B`.
- missing alias: most of the crates alias a dep but some dont.
- superfluous alias: most crates dont alias a dep but some do.

The script that i used first determines whether most crates opted to
alias a dependency or not. From that info it decides whether to use an
alias or not. If it decided to use an alias, the most common one is used
everywhere.

To reproduce, i used
[this](https://github.com/ggwpez/substrate-scripts/blob/master/uniform-crate-alias.py)
python script in combination with
[this](https://github.com/ggwpez/zepter/blob/38ad10585fe98a5a86c1d2369738bc763a77057b/renames.json)
error output from Zepter.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this issue Aug 2, 2024
Inherited workspace dependencies cannot be renamed by the crate using
them (see [1](rust-lang/cargo#12546),
[2](https://stackoverflow.com/questions/76792343/can-inherited-dependencies-in-rust-be-aliased-in-the-cargo-toml-file)).
Since we want to use inherited workspace dependencies everywhere, we
first need to unify all aliases that we use for a dependency throughout
the workspace.
The umbrella crate is currently excluded from this procedure, since it
should be able to export the crates by their original name without much
hassle.

For example: one crate may alias `parity-scale-codec` to `codec`, while
another crate does not alias it at all. After this change, all crates
have to use `codec` as name. The problematic combinations were:
- conflicting aliases: most crates aliases as `A` but some use `B`.
- missing alias: most of the crates alias a dep but some dont.
- superfluous alias: most crates dont alias a dep but some do.

The script that i used first determines whether most crates opted to
alias a dependency or not. From that info it decides whether to use an
alias or not. If it decided to use an alias, the most common one is used
everywhere.

To reproduce, i used
[this](https://github.com/ggwpez/substrate-scripts/blob/master/uniform-crate-alias.py)
python script in combination with
[this](https://github.com/ggwpez/zepter/blob/38ad10585fe98a5a86c1d2369738bc763a77057b/renames.json)
error output from Zepter.

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

5 participants