For `cargo install` we'll now show a more specific parse error for
semver, much like other parts of cargo.
This came out of my work on #12801. I was looking at what might be
appropriate to put in a `cargo-util-semver` crate and realized we have
the `ToSemver` trait that exists but doesn't do much, so I dropped it.
refactor(toml): Pull out the schema
### What does this PR try to resolve?
On its own, this PR is a net negative for readability / complexity. It moves all of the serde types related to manifest from `toml/mod.rs` to `toml/schema.rs`, leaving a lot of the functions and some trait implementations back in `toml/mod.rs` (some basic functions that made sense for the type on their own were also moved).
So why do this? This is an ooch towards having the schema broken out into a separate package (#12801). To do this, we need to
- Identify what all types need to be put in the package. This refactor highlights the dependence on `RustVersion` and `PackageIdSpec`
- Highlights what functionality we need to find a new home for
Follow up PRs would
- Find better homes for the logic in `toml/mod.rs`, moving us away from having `impl schema::Type` blocks.
- Pull out a `src/cargo/util_semver` package to own `PartialVersion` (at least) as prep for making a `cargo-util-semver` package
- Move `RustVersion` to `manifest-toml/schema.rs`, deciding what functionality needs to move with the type
- Move or copy `PackageIdSpec` into `manfest-toml/schema.rs`, deciding what functionality remain in `core/` and what moves over
- Move `toml/schema.rs` to `src/cargo/util_schema`
- Actually make `cargo-util-semver` and `cargo-util-manifest-schema` packages
So why do this now? This is a big change! By being incremental
- Reduce churn for me and others
- Be easier to review
- Collect feedback as we go on the whole plan to avoid more painful changes later
We *can* back this out if needed but the further we go, the more painful it will be.
### How should we test and review this PR?
### Additional information
fix(cli): Clarify --test is for targets, not test functions
We already refer to test targets as "test targets" instead of "tests" in `--test` but not `--tests` or in the error output. This makes it uniformly refer to them as "test targets", making it clearer that these aren't test functions.
Fixes#7864
We already refer to test targets as "test targets" instead of "tests" in
`--test` but not `--tests` or in the error output. This makes it
uniformly refer to them as "test targets", making it clearer that these
aren't test functions.
Fixes#7864
Add new packages to [workspace.members] automatically
### What does this PR try to resolve?
If a new package is created in a workspace, this change adds the package's path to the workspace's members list automatically.
It doesn't add the package to the list if the path is in the workspace's exclude list, or if the members list doesn't exist already. I noticed that a `cargo_new` test broke if I create the members list when it doesn't exist. This is because the workspace's manifest can be used for package templating. I think it's better to not break that use case.
Fixes#6378
### How should we test and review this PR?
I've included tests in the `cargo_new` suite.
When a user runs `cargo new` or `cargo init` within a workspace, Cargo will automatically add the new package to the members list in the workspace if necessary. The heuristic to add the new package is as follows:
- If there is no `members` list in the workspace yet, a new `members` list is created.
- If there is an `exclude` statement, Cargo checks if the new package should be excluded. If it doesn't match the `exclude` list, the package is added to the `members` list.
- If there is a glob expression in the `members` list that matches the new package, the package is not added to the `members` list.
- If the existent `members` list is sorted, Cargo tries to preserve the ordering when it adds the new package.
This change doesn't try to format the resulting `members` list in any way, leaving the formatting decissions to the user.
Signed-off-by: David Calavera <david.calavera@gmail.com>
To have a separate manifest API (#12801), we can't rely on interning
because it might be used in longer-lifetime applications.
I consulted https://github.com/rosetta-rs/string-rosetta-rs when
deciding on what string type to use for performance.
Originally, I hoped to entirely replacing string interning. For that, I
was looking at `arcstr` as it had a fast equality operator. However,
that is only helpful so long as the two strings we are comparing came
from the original source. Unsure how likely that is to happen (and
daunted by converting all of the `Copy`s into `Clone`s), I decided to
scale back.
Concerned about all of the small allocations when parsing a manifest, I
assumed I'd need a string type with small-string optimizations, like
`hipstr`, `compact_str`, `flexstr`, and `ecow`.
The first three give us more wiggle room and `hipstr` was the fastest of
them, so I went with that.
I then double checked macro benchmarks, and realized `hipstr` made no
difference and switched to `String` to keep things simple / with lower
dependencies.
When doing this, I had created a type alias (`TomlStr`) for the string
type so I could more easily swap it out if needed
(and not have to always write out a lifetime).
With just using `String`, I went ahead and dropped that.
I had problems getting the cargo benchmarks running, so I did a quick
and dirty benchmark that is end-to-end, covering fresh builds, clean
builds, and resolution. I ran these against a fresh clone of cargo's
code base.
```console
$ ../dump/cargo-12801-bench.rs run
Finished dev [unoptimized + debuginfo] target(s) in 0.07s
Running `target/debug/cargo -Zscript -Zmsrv-policy ../dump/cargo-12801-bench.rs run`
warning: `package.edition` is unspecified, defaulting to `2021`
Finished dev [unoptimized + debuginfo] target(s) in 0.04s
Running `/home/epage/.cargo/target/0a/7f4c1ab500f045/debug/cargo-12801-bench run`
$ hyperfine "../cargo-old check" "../cargo-new check"
Benchmark 1: ../cargo-old check
Time (mean ± σ): 119.3 ms ± 3.2 ms [User: 98.6 ms, System: 20.3 ms]
Range (min … max): 115.6 ms … 124.3 ms 24 runs
Benchmark 2: ../cargo-new check
Time (mean ± σ): 119.4 ms ± 2.4 ms [User: 98.0 ms, System: 21.1 ms]
Range (min … max): 115.7 ms … 123.6 ms 24 runs
Summary
../cargo-old check ran
1.00 ± 0.03 times faster than ../cargo-new check
$ hyperfine --prepare "cargo clean" "../cargo-old check" "../cargo-new check"
Benchmark 1: ../cargo-old check
Time (mean ± σ): 20.249 s ± 0.392 s [User: 157.719 s, System: 22.771 s]
Range (min … max): 19.605 s … 21.123 s 10 runs
Benchmark 2: ../cargo-new check
Time (mean ± σ): 20.123 s ± 0.212 s [User: 156.156 s, System: 22.325 s]
Range (min … max): 19.764 s … 20.420 s 10 runs
Summary
../cargo-new check ran
1.01 ± 0.02 times faster than ../cargo-old check
$ hyperfine --prepare "cargo clean && rm -f Cargo.lock" "../cargo-old check" "../cargo-new check"
Benchmark 1: ../cargo-old check
Time (mean ± σ): 21.105 s ± 0.465 s [User: 156.482 s, System: 22.799 s]
Range (min … max): 20.156 s … 22.010 s 10 runs
Benchmark 2: ../cargo-new check
Time (mean ± σ): 21.358 s ± 0.538 s [User: 156.187 s, System: 22.979 s]
Range (min … max): 20.703 s … 22.462 s 10 runs
Summary
../cargo-old check ran
1.01 ± 0.03 times faster than ../cargo-new check
```
I've wanted something like this myself. I dislike using `--open`
because I tend to move up to re-run my `cargo doc` run but then have to
edit it to remove `--open`.
Also makes it annoying when opening docs when `cargo doc` is wrapped by
a tool like `make`.
This was previously attempted in #5592:
- Unlike the request in #5562, this aligns with #5592 in always printing
rather than using a flag as this seems generally useful
- Unlike #5592, this prints as an alternative to "Opening" to keep
things light
- Unlike #5592, this prints afterwards as the link is only valid then
Fixes#5562
feat(toml): Allow version-less manifests
### What does this PR try to resolve?
Expected behavior with this PR:
- `package.version` defaults to `0.0.0`
- `package.publish` is defaulted to `version.is_some()`
This also updates "cargo script" to rely on this new behavior.
My motivation is to find ways to close the gap between "cargo script" and `Cargo.toml`. With "cargo script", we want to allow people to only write however much of a manifest is directly needed for the work they are doing (which includes having no manifest). Each difference between "cargo script" and `Cargo.toml` is a cost we have to pay in our documentation and a hurdle in a users understanding of what is happening.
There has been other interest in this which I also find of interest (from #9829):
- Lower boilerplate, whether for [cargo xtasks](https://github.com/matklad/cargo-xtask), nested packages (rust-lang/rfcs#3452), etc
- Unmet expectations from users because this field is primarily targeted at registry operations when they want it for their marketing version (#6583).
- Make "unpublished" packages stand out
This then unblocks unifying `package.publish` by making the field's default based on the presence of a version as inspired by the proposal in #9829. Without this change, we were trading one form of boilerplate (`version = "0.0.0"`) for another (`publish = false`).
Fixes#9829Fixes#12690Fixes#6153
### How should we test and review this PR?
The initial commit has test cases I thought would be relevant for this change and you can see how each commit affects those or existing test cases. Would definitely be interested in hearing of other troubling cases to test
Implementation wise, I made `MaybeWorkspaceVersion` deserializer trim spaces so I could more easily handle the field being an `Option`. This is in its own commit.
### Additional information
Alternatives considered
- Making the default version "stand out more" with it being something like `0.0.0+HEAD`. The extra noise didn't seem worth it and people would contend over what the metadata field *should be*
- Make the default version the lowest version possible (`0.0.0-0`?). Unsure if this will ever really matter especially since you can't publish
- Defer defaulting `package.publish` and instead error
- Further unifying more fields made this too compelling for me :)
- Put this behind `-Zscript` and make it a part of rust-lang/rfcs#3502
- Having an affect outside of that RFC, I wanted to make sure this got the attention it deserved rather than getting lost in the noise of a large RFC.
- Don't just default the version but make packages versionless
- I extended the concept of versionless to `PackageId`'s internals and saw no observable difference
- I then started to examine the idea of version being optional everywhere (via `PackageId`s API) and ... things got messy especially when starting to look at the resolver. This would have also added a lot of error checks / asserts for "the version must be set here". Overall, the gains seemed questionable and the cost high, so I held off.
Warn about crate name's format when creating new crate
### What does this PR try to resolve?
Warns about a crate's name during creation (`crate new ...`) if it doesn't follow the preferred snake_case format.
Fixes#2708
The warning message uses the language mentioned in [RFC 430](https://github.com/rust-lang/rfcs/blob/master/text/0430-finalizing-naming-conventions.md#general-naming-conventions).
### How should we test and review this PR?
Verified existing tests succeeded with updates. Added new tests to verify fix.
### Additional information
The link to [API naming guidelines](https://rust-lang.github.io/api-guidelines/naming.html) was not used since it still stays `unclear` for naming convention for crates.
fix(cli): Provide next steps for bad -Z flag
In #5546, they suggested we have a list of them but that would be hard to keep up and works well enough to just always mention it.
Fixes#5546
fix(remove): Preserve feature comments
### What does this PR try to resolve?
We've been having a hard time balancing leaving the feature list in a good looking start and preserving formatting. With our new formatting policy (#12836), we can just choose to preserve formatting instead.
Fixes#11743
### How should we test and review this PR?
The first commit copies an existing test. The second is where the fun begins, customizing the test for some weird cases. The follow up commits do the slow walk for improving it.
We ended up preserving some line-trailing comments because they come after the comma and toml_edit treats that as part of the prefix of the next item. Tracking removal of that was going to require us to determine if the newline existed in the suffix or in the next item's prefix and edit accordingly and I decided to skip that to keep this initial implementation simpler.
### Additional information
fix(help):Clarify install's positional
### What does this PR try to resolve?
- That a version is accepted
- That you are selecting from the source a package which led to part of
the confusion in #4830
I wonder if we should rename our `CRATE` value names to `PKG`/`PACKAGE`
While doing this, I decided to fix the inconsistency in how we handle value names in help.
### How should we test and review this PR?
### Additional information
Adjust `-Zcheck-cfg` for new rustc syntax and behavior
https://github.com/rust-lang/rust/pull/111072 introduced a new syntax for `rustc` `--check-cfg` argument. This PR adjust cargo `-Zcheck-cfg` for new that new syntax and behavior.
This PR removes all the `-Zcheck-cfg` options (`features`, `names`, `values`, `output`), as they don't make much sense now since with the new `rustc` behavior: `features`, `names` and `values` are all combine together and the `output` option was only here because the other were.
Now the new behavior from cargo is to always pass one `--check-cfg` argument to rustc for the `feature`s which implicitly enables well known names and values.
fix(replace): Partial-version spec support
### What does this PR try to resolve?
#12614 changed package ID specs to allow fields in the version number to be optional. This earliest branch with this change is `rust-1.74.0` (beta). While `@Eh2406` was investigating version metadata issues in #12772, problems with the partial version change were found
- `replace`s that specify version metadata were ignored **(fixed with this PR)**
- This also extends out to any other place a PackageIDSpec may show up, like `cargo check -p <name>`@<spec>``
- We explicitly kept the same semantics of version requirements that pre-releases require opt-in. If nothing else, this gives us more room to change semantics in the future if we ever fix the semantics for pre-release.
- `replace`s that don't specify version metadata when the `Cargo.lock` contained a version metadata, it would previously be ignored (with a warning) but now match **(unchanged with this PR)**
- When the version metadata in `Cargo.lock` differed from the overriding `Cargo.toml`, cargo would panic **(now an error in this PR)**
With this PR, we are acknowledging that we changed behavior in taking ignored replaces (because of differences with version metadata) and applying them. Seeing as version metadata is relatively rare, replaces are relatively rare, and differences in it for registries is unsupported, the impact seems very small.
The questions before us are
- Do we revert #12614 in `master` and `rust-1.74.0` or merge this PR into `master`
- If we merge this PR into `master`, do we cherry-pick this into `rust-1.74.0` or revert #12614, giving ourselves more time to find problems
### How should we test and review this PR?
The initial commit adds tests that pass as of #12614. Prior to #12614, these tests would have warned that the `replace` was unused and failed because `bar::bar` didn't exist. Each commit then changes the behavior (or not) and updates the corresponding test.
### Additional information
This commit removes all the -Zcheck-cfg options (features, names,
values, output), as they don't make much sense now since features, names
and values are all combine together for upstream rustc and the output
option was only here because the other were.
Now the new behavior from cargo is to always pass one `--check-cfg`
argument to rustc for the `feature`s which implicitly enables well known
names and values.
Print environment variables for build script executions with `-vv`
### What does this PR try to resolve?
When debugging complicated builds (I was trying to figure out how `cargo-miri` cross-compiles compiler_builtins without needing a C cross compiler), it's useful to see all the environment variables passed to the build script.
This is also consistent with other commands.
### How should we test and review this PR?
I tested it locally by creating a small crate with an empty `build.rs` and building it. Additionally, a test is included.
fix(cli): Suggest cargo-search on bad commands
This is a low-tech solution alternative to the options proposed in #4682
- Search `[[bin]]`s within all packages in the registry (which aren't tracked atm), suggesting to `cargo install` what is found
- Check if `cargo-<cmd>` is in the registry, suggesting `cargo install if it is
By suggesting `cargo search`, we are giving them a tool so they can verify if the package is what they want (a `cargo info` would help as a next step).
Is is needed?
- New users might not know of `cargo search` but they can search on crates.io
- New users might not be aware of the `cargo-<cmd>` naming pattern
Seems like this can still offer some benefit.
Fixes#4682
- That a version is accepted
- That you are selecting from the source a package which led to part of
the confusion in #4830
I wonder if we should rename our `CRATE` value names to `PKG`/`PACKAGE`
This is a low-tech solution alternative to the options proposed in #4682
- Search `[[bin]]`s within all packages in the registry (which aren't
tracked atm), suggesting to `cargo install` what is found
- Check if `cargo-<cmd>` is in the registry, suggesting `cargo install
if it is
By suggesting `cargo search`, we are giving them a tool so they can
verify if the package is what they want (a `cargo info` would help as a
next step).
Is is needed?
- New users might not know of `cargo search` but they can search on
crates.io
- New users might not be aware of the `cargo-<cmd>` naming pattern
Seems like this can still offer some benefit
Fixes#4682
