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

Next step for deprecations in Cargo #13629

Open
4 of 13 tasks
weihanglo opened this issue Mar 22, 2024 · 2 comments
Open
4 of 13 tasks

Next step for deprecations in Cargo #13629

weihanglo opened this issue Mar 22, 2024 · 2 comments
Labels
A-cli Area: Command-line interface, option parsing, etc. A-edition-next Area: may require a breaking change over an edition A-manifest Area: Cargo.toml issues C-cleanup Category: cleanup within the codebase S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@weihanglo
Copy link
Member

weihanglo commented Mar 22, 2024

2024 Deprecations

Problem

Cargo has been emitting a bunch deprecation message for a (long?) while. Since edition 2024 is approaching (1.82.0 anticipated), and a Cargo linting system is on the way (#13621), we might want to take advantage of this timing to evaluate whether this deprecations could be turned into a hard error.

The main risk here is CI automation might fail. Unlike rustc and clippy lints, people hardly noticed Cargo warnings becuase because there was no way to set -D on them.

Deprecations

2024 Edition

These are being tracked in rust-lang/rust#123754

dev_dependencies/build_dependencies/default_features/crate_types/proc_macro

"conflicting between `{new_path}` and `{old_path}` in the `{name}` {kind}.\n
`{old_path}` is ignored and not recommended for use in the future"

  • Warn since: 1.61 (2022-05-19) but only when conflicting with dash ones
  • may become a hard error? NO
  • Risk: Old crates fail to compile. Might need an edition boundary.
  • Warning on conflicting keys #10316
  • Next step:
    • Error on 2024 edition

good old [project] (replaced with [package])

"manifest at `{}` contains both `project` and `package`, \
this could become a hard error in the future",

default-features in inherited dependencies

"`default-features` is ignored for {label}, since `default-features` was \
{ws_def_feat} for `workspace.dependencies.{label}`, \
this could become a hard error in the future"

Independent of Edition

Approved changes

Proposed changes

plugin support

"support for rustc plugins has been removed from rustc. \
library `{}` should not specify `plugin = true`",
name_or_panic(lib)
));
warnings.push(format!(
"support for `plugin = true` will be removed from cargo in the future"

dependency without path, version, git, workspace specified

"dependency ({}) specified without \
providing a local path, Git repository, version, or \
workspace dependency to use. This will be considered an \
error in future versions",
name_in_toml

Note:

  • this affects [dependencies] as well as [patch], [replace], including in virtual workspaces and config where Editions don't apply
  • hard error makes the format more future proof for adding additional sources (otherwise old cargos would do wildly bad things)

license-file and readme pointing to a non-existent file

"{manifest_key_name} `{}` does not appear to exist{}.\n\
Please update the {manifest_key_name} setting in the manifest at `{}`\n\
This may become a hard error in the future.",

--release is ignored when paired with --profile

"the `--release` flag should not be specified with the `--profile` flag\n\
The `--release` flag will be ignored.\n\
This was historically accepted, but will become an error \
in a future release."

  • Warn since: 1.57.0 (2021-12-02)
  • may become a hard error? ✅
  • Risk: Break people's build. Easy to fix by themselves.
  • Stabilize named profiles #9943
  • Next step:
    • Hard error

cargo read-manifest

Deprecated, use `<cyan,bold>cargo metadata --no-deps</>` instead.\

  • Warn since: Merged on 2016-10-03.
  • Become a hard error? NO
  • Risk: Automations might still rely on it. cargo metadata --no-deps should cover its use cases.
  • Document that read_manifest command is deprecated #3150
  • Next step:
    • Needs further investigation (unclear why this was deprecated and if it still fills a unique niche)

[replace] is deprecated

> **Note**: `[replace]` is deprecated. You should use the
> [`[patch]`](#the-patch-section) table instead.

  • Warn since: 1.42.0 (2020-03-12)
  • may become a hard error? NO
  • Risk: Can we patch [patch] to fully replace [repalce]?
  • Various doc updates #7733
  • Next step (proposed)
    • Deprecation warning using lint system (since restricted Carog.toml)
    • Maybe with future edition turn it into forbid (based on people's reaction to warning)

old cargo tree flags: --all, no-dev-dependencies, --no-indent, --prefix-depth, --all-targets

"The `cargo tree` --all flag has been changed to --no-dedupe, \
and may be removed in a future version.\n\
If you are looking to display all workspace members, use the --workspace flag.",

path override modifying dependency graph

This is currently allowed but is known to produce buggy behavior with spurious
recompiles and changes to the crate graph. Path overrides unfortunately were
never intended to support this feature, so for now this message is just a
warning. In the future, however, this message will become a hard error.

  • Warn since: Merged on 2016-10-05 (1.14 or so)
  • may become a hard error? ✅ (was a bug)
  • Risk: Assuming people seldom use it?
  • Warn about path overrides that won't work #3136
  • Next step (proposed):
    • Turn into error because it was a bug with buggy behavior and people using this went into it knowing its bad (since they likely started post-1.14), and we've not been getting necro-posts for people saying they need this (like depending on bins)

rustc-cdylib-link-arg used in non-cdylib target

"{}{} was specified in the build script of {}, \
but that package does not contain a cdylib target\n\
\n\
Allowing this was an unintended change in the 1.50 \
release, and may become an error in the future. \
For more information, see \
<https://github.com/rust-lang/cargo/issues/9562>.",

user-defined alias is shadowing an external subcommand found

cargo/src/bin/cargo/cli.rs

Lines 321 to 323 in f0ae765

user-defined alias `{}` is shadowing an external subcommand found at: `{}`
This was previously accepted but is being phased out; it will become a hard error in a future release.
For more information, see issue #10049 <https://github.com/rust-lang/cargo/issues/10049>.",

output artifact name collisions

"Consider changing their names to be unique or compiling them separately.\n\
This may become a hard error in the future; see \
<https://github.com/rust-lang/cargo/issues/6313>.";

  • Warn since: 1.32.0 (2019-01-17)
  • may become a hard error? ✅
  • Risk: Workaround could be quite complicated.
  • Check for duplicate output filenames. #6308
  • Next step (proposed):
    • Maybe in a future Edition, switch to forbid by default?
    • Or just warn because this is implementation-defined for when a collision may occur and new collisions may come up in the future

Bail out when trying to link to a library that is not linkable

"The package `{}` \
provides no linkable target. The compiler might raise an error while compiling \
`{}`. Consider adding 'dylib' or 'rlib' to key `crate-type` in `{}`'s \
Cargo.toml. This warning might turn into a hard error in the future.",

Ambiguous package name in git dependency

// We can assume a package with publish = false isn't intended to be seen
// by users so we can hide the warning about those since the user is unlikely
// to care about those cases.
if pkg.publish().is_none() {
let _ = gctx.shell().warn(format!(
"skipping duplicate package `{}` found at `{}`",
pkg.name(),
path.display()
));
}

No further steps

deprecate --all (alias to --workspace)

flag("all", "Alias for --workspace (deprecated)")

  • Warn since: 1.39.0 (2019-11-07)
  • may become a hard error? NO
  • Risk: Automations might still rely on it. Some users use it exclusively due to brevity (see also Add short flag -w for --workspace #11554)
  • Rename --all to --workspace #7241
  • Next step (proposed):
    • CLI, so no further steps
    • Not doing runtime warning because of pervasive use by people who really like it for brevity

Deprecate .cargo/config

"`{}` is deprecated in favor of `{filename_without_extension}.toml`",
possible.display(),
))?;
self.shell().note(
format!("if you need to support cargo 1.38 or earlier, you can symlink `{filename_without_extension}` to `{filename_without_extension}.toml`"),

@weihanglo weihanglo added C-cleanup Category: cleanup within the codebase S-needs-team-input Status: Needs input from team on whether/how to proceed. labels Mar 22, 2024
@epage

This comment was marked as resolved.

@epage
Copy link
Contributor

epage commented Apr 16, 2024

We discussed some of these in today's team meeting and updated the next steps for those items. For things that weren't discussed I noted that the "Next Steps" are "proposed".

bors added a commit that referenced this issue Apr 16, 2024
fix(toml): Error on `[project]` in Edition 2024

### What does this PR try to resolve?

`[package]` was added in 86b2a2a in the pre-1.0 days but `[project]` was never removed nor did we warn on its use until recently in #11135.  So likely we can't remove it completely but we can remove it in Edition 2024+.

This includes `cargo fix` support which is hard coded directly into the `cargo fix` command.

This is part of
- #13629
- rust-lang/rust#123754

While we haven't signed off on everything in #13629, I figured this (and a couple other changes) are not very controversial.

### How should we test and review this PR?

Per commit.  Tests are added to show the behavior changes, including in `cargo fix`.

### Additional information
epage added a commit to epage/cargo that referenced this issue Apr 18, 2024
This is part of rust-lang#13629

This turns deps like
```toml
foo = { optional = true }
```
from `version="*"` deps with a warning into errors.
This breaking change was deemed acceptable because this behavior has
been considered a bug from the beginning.
We have gotten little to no feedback about people wanting this behavior.

This improves our forwards-compatibility story as we can add new
dependency sources and they won't be considered a wildcard registry
dependency on older cargo.
bors added a commit that referenced this issue Apr 18, 2024
fix(toml)!: Disallow source-less dependencies

### What does this PR try to resolve?

This is part of #13629 addressing “dependency without path, version, git, workspace specified”.

This turns deps like
```toml
foo = { optional = true }
```
from `version="*"` deps with a warning into errors. This breaking change was deemed acceptable because this behavior has been considered a bug from the beginning.
We have gotten little to no feedback about people wanting this behavior.

This improves our forwards-compatibility story as we can add new dependency sources and they won't be considered a wildcard registry dependency on older cargo.

### How should we test and review this PR?

I removed the `cargo_add` test because it is redundant.

We no longer get the “unused key” warnings because those warnings get deferred to after this error gets reported.

### Additional information
bors added a commit that referenced this issue Apr 22, 2024
fix(toml): Report `_` fied variants (e.g. `dev_dependencies`) as deprecated

### What does this PR try to resolve?

This is prep for removing them in the 2024 Edition and is part of rust-lang/rust#123754 and #13629

This doesn't include 2024 Edition work because there is a risk of conflict with other work going on these areas.

This changes us from
- When using `-` and `_` variants: deprecated, will error some time
- Otherwise, nothing

To
- When using `-` and `_` variants: unused field warning
- When using only `_`: deprecation, will be removed in 2024

I decided to model this as an unused field warning because that is what this is and that is how any other use of `_` works.  We might hard error during a transition period but I'd eventually want these to just make the code act like anything else in the end.

### How should we test and review this PR?

### Additional information
bors added a commit that referenced this issue Apr 24, 2024
fix(toml): Be more forceful with underscore/dash redundancy

### What does this PR try to resolve?

This is prep for removing them in the 2024 Edition and is part of rust-lang/rust#123754 and #13629

During #13783, I had considered making the 2024 edition behavior a "unused key" warning.  However, the work and code mess to pipe the data through correctly handle the two fields in all cases didn't seem worth it (and a hard error might be better to help users transition).

### How should we test and review this PR?

### Additional information
bors added a commit that referenced this issue Apr 24, 2024
fix(toml): Don't double-warn when underscore is used in workspace dep

### What does this PR try to resolve?

This is prep for removing them in the 2024 Edition and is part of rust-lang/rust#123754 and #13629

Particularly, I wanted to make sure I didn't make things worse and in doing so found there was room for improvement.

### How should we test and review this PR?

### Additional information
epage added a commit to epage/cargo that referenced this issue Apr 25, 2024
epage added a commit to epage/cargo that referenced this issue Apr 26, 2024
bors added a commit that referenced this issue Apr 26, 2024
fix(toml): Remove underscore field support in 2024

### What does this PR try to resolve?

This is part of the 2024 Edition and is part of rust-lang/rust#123754 and #13629

### How should we test and review this PR?

### Additional information
@weihanglo weihanglo added the S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review label May 15, 2024
@weihanglo weihanglo removed the S-needs-team-input Status: Needs input from team on whether/how to proceed. label May 15, 2024
bors added a commit that referenced this issue May 29, 2024
fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors

### What does this PR try to resolve?

In this PR:
- Changed the warning to a hard error and modified the associated test function;
- Removed what should have been a redundant test function:`publish::publish_with_missing_readme`;
- Since `cargo publish` is preceded by the execution of `cargo package`, the error message in the test `function bad_license_file` needs to be modified.

issue: #13629 (comment).

### Additional information

It seems that this is not enough, the current situation is that `cargo package` warns if `package.readme` is an empty string or the wrong file location, but if I cancel `package.readme`, no warning is generated.

I'm wondering if I should judge `package.readme&licence` when executing `cargo package` and return an error if it doesn't exist?

As this has not been done before, your advice is sought.
bors added a commit that referenced this issue May 29, 2024
fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors

### What does this PR try to resolve?

In this PR:
- Changed the warning to a hard error and modified the associated test function;
- Removed what should have been a redundant test function:`publish::publish_with_missing_readme`;
- Since `cargo publish` is preceded by the execution of `cargo package`, the error message in the test `function bad_license_file` needs to be modified.

issue: #13629 (comment).

### Additional information

It seems that this is not enough, the current situation is that `cargo package` warns if `package.readme` is an empty string or the wrong file location, but if I cancel `package.readme`, no warning is generated.

I'm wondering if I should judge `package.readme&licence` when executing `cargo package` and return an error if it doesn't exist?

As this has not been done before, your advice is sought.
bors added a commit that referenced this issue Jun 9, 2024
fix(toml): Convert warnings that `licence` and `readme` files do not exist into errors

### What does this PR try to resolve?

In this PR:
- Changed the warning to a hard error and modified the associated test function;
- Removed what should have been a redundant test function:`publish::publish_with_missing_readme`;
- Since `cargo publish` is preceded by the execution of `cargo package`, the error message in the test `function bad_license_file` needs to be modified.

issue: #13629 (comment).

### Additional information

It seems that this is not enough, the current situation is that `cargo package` warns if `package.readme` is an empty string or the wrong file location, but if I cancel `package.readme`, no warning is generated.

I'm wondering if I should judge `package.readme&licence` when executing `cargo package` and return an error if it doesn't exist?

As this has not been done before, your advice is sought.
bors added a commit that referenced this issue Jun 9, 2024
fix: using `--release/debug` and `--profile` together becomes an error

### What does this PR try to resolve?

part of #13629

issue #13629 (comment)
bors added a commit that referenced this issue Jun 9, 2024
fix(toml): remove `lib.plugin` key support and make it warning

### What does this PR try to resolve?

Remove `lib.plugin` key, making it an "unused key" warning.

Remove some of the tests, which should look useless (I hope I'm understanding this

- [x] Remove key, and related tests.
- [x] Adjust the documentation about the plugin.
- [ ] Some of the comments and function names have not yet finished being modified.

part of #13629

Closes #13246
bors added a commit that referenced this issue Jun 9, 2024
fix(toml): remove `lib.plugin` key support and make it warning

### What does this PR try to resolve?

Remove `lib.plugin` key, making it an "unused key" warning.

Remove some of the tests, which should look useless (I hope I'm understanding this

- [x] Remove key, and related tests.
- [x] Adjust the documentation about the plugin.
- [ ] Some of the comments and function names have not yet finished being modified.

part of #13629

Closes #13246
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. A-edition-next Area: may require a breaking change over an edition A-manifest Area: Cargo.toml issues C-cleanup Category: cleanup within the codebase S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
Development

No branches or pull requests

2 participants