feat(toml): Allow version-less manifests #12786

epage · 2023-10-06T19:03:37Z

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, nested packages (RFC: Nested Cargo packages rfcs#3452), etc
Unmet expectations from users because this field is primarily targeted at registry operations when they want it for their marketing version (Possibility to set the package version dynamically #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 #9829
Fixes #12690
See also #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 RFC: cargo-script 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 PackageIds 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.

Unsure if we want to generalize the trim policy but this will make it much easier to support an optional version field in future commits since we can just wrap the type in `Option` Strangely, `UntaggedEnumDeserializer` isn't causing bad error messages, so we can keep using it.

rustbot · 2023-10-06T19:03:43Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

epage · 2023-10-06T19:04:09Z

@rfcbot fcp merge

See the PR description for the change proposal

rfcbot · 2023-10-06T19:04:10Z

Team member @epage has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

src/cargo/ops/registry/publish.rs

arlosi · 2023-10-11T16:28:00Z

src/cargo/util/toml/mod.rs

        };

+        if version.is_none() && publish != Some(vec![]) {
+            bail!("`package.version` must be specified if `package.publish = true`");


This error could be confusing if publish = ['my-registry']

Hopefully the new message is a little better:

package.publish requires package.version be specified

This defaults the version to `0.0.0` for most of cargo. It is an error to lack a version and have a package publishable. That means you have to add `publish = false`.

Before the default was hardcoded to `true`. The problem was that means that to remove the `package.version` boilerplate, you had to add `package.publish = false` boilerplate. To make the errors easier to understand in this situation, I err on the side of encouraging people to put `publish = true` in their manifests. By making this change, we also unblock "cargo script" / `Cargo.toml` unifying the handling of `package.publish`.

weihanglo

Implementation-wise looks good to me.

weihanglo · 2023-10-17T15:27:39Z

src/doc/src/reference/manifest.md

@@ -109,6 +109,8 @@ resolve dependencies, and for guidelines on setting your own version. See the
 [SemVer compatibility] chapter for more details on exactly what constitutes a
 breaking change.

+This field is optional and defaults to `0.0.0`.


Just a random idea. Wonder if we can have a more structural way to show the information of manifest key definition, such as how configuration doc currently looks like.

I feel like each approach works well for some cases and not for others. Wish we could have a code fence with links. I feel like that could allow the best of both worlds.

weihanglo · 2023-10-17T15:37:50Z

src/cargo/util/toml/mod.rs

+            Some(VecStringOrBool::Bool(true)) => None,
+            None => version.is_none().then_some(vec![]),
        };

+        if version.is_none() && publish != Some(vec![]) {
+            bail!("`package.publish` requires `package.version` be specified");
+        }


This always bugs me. Now it looks like

publish = true -> None

not specified -> []

which is a good thing as it exactly aligns to the logic here:

cargo/src/cargo/core/package.rs

Lines 170 to 175 in 87f4b1b

/// Returns `None` if the package is set to publish.

/// Returns `Some(allowed_registries)` if publishing is limited to specified

/// registries or if package is set to not publish.

pub fn publish(&self) -> &Option<Vec<String>> {

self.manifest().publish()

}

I wonder if we could have a type wrapper to make the logic more clear with some methods.

We likely should refactor this to use a custom enum, rather than Option.

rfcbot · 2023-10-17T15:42:07Z

🔔 This is now entering its final comment period, as per the review above. 🔔

weihanglo · 2023-10-17T16:33:07Z

Not really on topic, but somehow relevant. #5979 proposes to remove package.version filed in Cargo.lock for workspace members.

rfcbot · 2023-10-27T15:43:07Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

weihanglo · 2023-10-27T16:04:52Z

🎉 🎉 🎉

@bors r+

bors · 2023-10-27T16:04:54Z

📌 Commit 7ac49a9 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-10-27T16:09:14Z

⌛ Testing commit 7ac49a9 with merge 307486e...

bors · 2023-10-27T17:01:00Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 307486e to master...

Update cargo 8 commits in df3509237935f9418351b77803df7bc05c009b3d..708383d620e183a9ece69b8fe930c411d83dee27 2023-10-24 23:09:01 +0000 to 2023-10-27 21:09:26 +0000 - feat(doc): Print the generated docs links (rust-lang/cargo#12859) - feat(toml): Allow version-less manifests (rust-lang/cargo#12786) - Remove outdated option to `-Zcheck-cfg` warnings (rust-lang/cargo#12884) - Remove duplicate binaries during install (rust-lang/cargo#12868) - refactor(shell): Write at once rather than in fragments (rust-lang/cargo#12880) - docs(ref): Link to docs.rs metadata table (rust-lang/cargo#12879) - docs(contrib): Describe how to add a new package (rust-lang/cargo#12878) - move up looking at index summary enum (rust-lang/cargo#12749) r? ghost

doc: `[package]` doesn't require `version` field Since #12786 (in 1.75) cargo doesn't require the version field as it defaults to 0.0.0. A bin crate with the following toml builds successfully with `cargo build`: ```toml [package] name = "foo" ``` This PR slightly rewords the `[package]` section of `src/doc/src/reference/manifest.md` to reflect this.

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * For an external LLVM, set dependency of llvm >= 16, in accordance with the upstream changes. * Mark that on NetBSD we now need >= 9.0, so 8.x is no longer supported. * On NetBSD/sparc64 10.x, we now need GCC 12 to build the embedded LLVM, which is version 17; apparently GCC 10.4 or 10.5 mis-compiles it, resulting in an illegal instruction fault during the build. Ref. rust-lang/rust#117231 Upstream changes: Version 1.75.0 (2023-12-28) ========================== - [Stabilize `async fn` and return-position `impl Trait` in traits.] (rust-lang/rust#115822) - [Allow function pointer signatures containing `&mut T` in `const` contexts.] (rust-lang/rust#116015) - [Match `usize`/`isize` exhaustively with half-open ranges.] (rust-lang/rust#116692) - [Guarantee that `char` has the same size and alignment as `u32`.] (rust-lang/rust#116894) - [Document that the null pointer has the 0 address.] (rust-lang/rust#116988) - [Allow partially moved values in `match`.] (rust-lang/rust#103208) - [Add notes about non-compliant FP behavior on 32bit x86 targets.] (rust-lang/rust#113053) - [Stabilize ratified RISC-V target features.] (rust-lang/rust#116485) Compiler -------- - [Rework negative coherence to properly consider impls that only partly overlap.] (rust-lang/rust#112875) - [Bump `COINDUCTIVE_OVERLAP_IN_COHERENCE` to deny, and warn in dependencies.] (rust-lang/rust#116493) - [Consider alias bounds when computing liveness in NLL.] (rust-lang/rust#116733) - [Add the V (vector) extension to the `riscv64-linux-android` target spec.] (rust-lang/rust#116618) - [Automatically enable cross-crate inlining for small functions] (rust-lang/rust#116505) - Add several new tier 3 targets: - [`csky-unknown-linux-gnuabiv2hf`] (rust-lang/rust#117049) - [`i586-unknown-netbsd`] (rust-lang/rust#117170) - [`mipsel-unknown-netbsd`] (rust-lang/rust#117356) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Override `Waker::clone_from` to avoid cloning `Waker`s unnecessarily.] (rust-lang/rust#96979) - [Implement `BufRead` for `VecDeque<u8>`.] (rust-lang/rust#110604) - [Implement `FusedIterator` for `DecodeUtf16` when the inner iterator does.] (rust-lang/rust#110729) - [Implement `Not, Bit{And,Or}{,Assign}` for IP addresses.] (rust-lang/rust#113747) - [Implement `Default` for `ExitCode`.] (rust-lang/rust#114589) - [Guarantee representation of None in NPO] (rust-lang/rust#115333) - [Document when atomic loads are guaranteed read-only.] (rust-lang/rust#115577) - [Broaden the consequences of recursive TLS initialization.] (rust-lang/rust#116172) - [Windows: Support sub-millisecond sleep.] (rust-lang/rust#116461) - [Fix generic bound of `str::SplitInclusive`'s `DoubleEndedIterator` impl] (rust-lang/rust#100806) - [Fix exit status / wait status on non-Unix `cfg(unix)` platforms.] (rust-lang/rust#115108) Stabilized APIs --------------- - [`Atomic*::from_ptr`] (https://doc.rust-lang.org/stable/core/sync/atomic/struct.AtomicUsize.html#method.from_ptr) - [`FileTimes`] (https://doc.rust-lang.org/stable/std/fs/struct.FileTimes.html) - [`FileTimesExt`] (https://doc.rust-lang.org/stable/std/os/windows/fs/trait.FileTimesExt.html) - [`File::set_modified`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_modified) - [`File::set_times`] (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.set_times) - [`IpAddr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/enum.IpAddr.html#method.to_canonical) - [`Ipv6Addr::to_canonical`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_canonical) - [`Option::as_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_slice) - [`Option::as_mut_slice`] (https://doc.rust-lang.org/stable/core/option/enum.Option.html#method.as_mut_slice) - [`pointer::byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_add) - [`pointer::byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset) - [`pointer::byte_offset_from`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_offset_from) - [`pointer::byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.byte_sub) - [`pointer::wrapping_byte_add`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_add) - [`pointer::wrapping_byte_offset`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_offset) - [`pointer::wrapping_byte_sub`] (https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.wrapping_byte_sub) These APIs are now stable in const contexts: - [`Ipv6Addr::to_ipv4_mapped`] (https://doc.rust-lang.org/stable/core/net/struct.Ipv6Addr.html#method.to_ipv4_mapped) - [`MaybeUninit::assume_init_read`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.assume_init_read) - [`MaybeUninit::zeroed`] (https://doc.rust-lang.org/stable/core/mem/union.MaybeUninit.html#method.zeroed) - [`mem::discriminant`] (https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html) - [`mem::zeroed`] (https://doc.rust-lang.org/stable/core/mem/fn.zeroed.html) Cargo ----- - [Add new packages to `[workspace.members]` automatically.] (rust-lang/cargo#12779) - [Allow version-less `Cargo.toml` manifests.] (rust-lang/cargo#12786) - [Make browser links out of HTML file paths.] (rust-lang/cargo#12889) Rustdoc ------- - [Accept less invalid Rust in rustdoc.] (rust-lang/rust#117450) - [Document lack of object safety on affected traits.] (rust-lang/rust#113241) - [Hide `#[repr(transparent)]` if it isn't part of the public ABI.] (rust-lang/rust#115439) - [Show enum discriminant if it is a C-like variant.] (rust-lang/rust#116142) Compatibility Notes ------------------- - [FreeBSD targets now require at least version 12.] (rust-lang/rust#114521) - [Formally demote tier 2 MIPS targets to tier 3.] (rust-lang/rust#115238) - [Make misalignment a hard error in `const` contexts.] (rust-lang/rust#115524) - [Fix detecting references to packed unsized fields.] (rust-lang/rust#115583) - [Remove support for compiler plugins.] (rust-lang/rust#116412)

The package clap now requires rustc 1.74 or newer, and moving to 1.75 keeps with our N-2 versioning policy. This will also allow us to use the cargo feature supporting version-less manifests, which is nice for our xtasks. See: - rust-lang/cargo#12786

epage added 2 commits October 6, 2023 12:21

test(toml): Verify existing version-less behavior

5c9a126

epage added the T-cargo Team: Cargo label Oct 6, 2023

rustbot assigned weihanglo Oct 6, 2023

rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-interacts-with-crates.io Area: interaction with registries A-manifest Area: Cargo.toml issues Command-publish S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 6, 2023

rfcbot added proposed-final-comment-period An FCP proposal has started, but not yet signed off. disposition-merge FCP with intent to merge labels Oct 6, 2023

epage force-pushed the version branch 2 times, most recently from 9295ea6 to 0a58fce Compare October 6, 2023 21:13

epage mentioned this pull request Oct 9, 2023

Change the package.publish default to false on an edition boundary #12690

Closed

epage force-pushed the version branch from 0a58fce to 3bcef36 Compare October 9, 2023 20:33

arlosi reviewed Oct 11, 2023

View reviewed changes

epage added 4 commits October 11, 2023 13:13

feat(toml): Allow versionless packages

1923b5b

This defaults the version to `0.0.0` for most of cargo. It is an error to lack a version and have a package publishable. That means you have to add `publish = false`.

fix(embedded): Rely on new defaulted version field

cc9be44

fix(embedded): Rely on package.publish default for scripts

7ac49a9

epage force-pushed the version branch from 3bcef36 to 020d341 Compare October 11, 2023 18:17

weihanglo reviewed Oct 17, 2023

View reviewed changes

rfcbot added final-comment-period FCP — a period for last comments before action is taken and removed proposed-final-comment-period An FCP proposal has started, but not yet signed off. labels Oct 17, 2023

epage force-pushed the version branch from 020d341 to 58b175e Compare October 18, 2023 16:44

rfcbot added to-announce and removed final-comment-period FCP — a period for last comments before action is taken labels Oct 27, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2023

bors merged commit 307486e into rust-lang:master Oct 27, 2023
20 checks passed

epage mentioned this pull request Oct 27, 2023

cargo publish should be opt-in #6153

Open

epage deleted the version branch October 27, 2023 17:08

weihanglo mentioned this pull request Oct 28, 2023

Update cargo rust-lang/rust#117302

Merged

weihanglo mentioned this pull request Oct 28, 2023

Cargo package version from git tag #12144

Closed

ehuss added this to the 1.75.0 milestone Nov 6, 2023

dgavedissian mentioned this pull request Nov 21, 2023

Various improvements for app startup Limit-Theory-Redux/ltheory#190

Merged

taiki-e mentioned this pull request Dec 10, 2023

codegen fails due to missing Cargo.toml package.version field taiki-e/install-action#298

Closed

cuviper mentioned this pull request Jan 2, 2024

Upgrade to cargo 0.76.0 (MSRV 1.73) kbknapp/cargo-outdated#379

Merged

kp-mariappan-ramasamy mentioned this pull request Jan 3, 2024

[auto] Update Rust toolchain to 1.75.0 expressvpn/wolfssl-rs#88

Merged

faern mentioned this pull request Jan 5, 2024

Remove version and publish from cargo metadata mullvad/mullvadvpn-app#5660

Merged

epage mentioned this pull request Jan 22, 2024

Expand manifest validation for all settings #4377

Open

stevenengler mentioned this pull request Feb 4, 2024

doc: [package] doesn't require version field #13390

Merged

taiki-e mentioned this pull request Feb 24, 2024

Add wasm-bindgen taiki-e/install-action#383

Merged

markdingram mentioned this pull request Mar 21, 2024

Fails when package.version not set (Cargo 1.75+) LukeMathWalker/cargo-chef#265

Closed

elasticdog mentioned this pull request Mar 26, 2024

Bump MSRV to the Rust 1.75 release EarthmanMuons/spellout#211

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(toml): Allow version-less manifests #12786

feat(toml): Allow version-less manifests #12786

epage commented Oct 6, 2023 •

edited

rustbot commented Oct 6, 2023

epage commented Oct 6, 2023

rfcbot commented Oct 6, 2023 •

edited by ehuss

arlosi Oct 11, 2023

epage Oct 11, 2023

weihanglo left a comment

weihanglo Oct 17, 2023

epage Oct 17, 2023

weihanglo Oct 17, 2023

epage Oct 17, 2023

rfcbot commented Oct 17, 2023

weihanglo commented Oct 17, 2023

rfcbot commented Oct 27, 2023

weihanglo commented Oct 27, 2023

bors commented Oct 27, 2023

bors commented Oct 27, 2023

bors commented Oct 27, 2023

	/// Returns `None` if the package is set to publish.
	/// Returns `Some(allowed_registries)` if publishing is limited to specified
	/// registries or if package is set to not publish.
	pub fn publish(&self) -> &Option<Vec<String>> {
	self.manifest().publish()
	}

feat(toml): Allow version-less manifests #12786

feat(toml): Allow version-less manifests #12786

Conversation

epage commented Oct 6, 2023 • edited

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Oct 6, 2023

epage commented Oct 6, 2023

rfcbot commented Oct 6, 2023 • edited by ehuss

arlosi Oct 11, 2023

Choose a reason for hiding this comment

epage Oct 11, 2023

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Oct 17, 2023

Choose a reason for hiding this comment

epage Oct 17, 2023

Choose a reason for hiding this comment

weihanglo Oct 17, 2023

Choose a reason for hiding this comment

epage Oct 17, 2023

Choose a reason for hiding this comment

rfcbot commented Oct 17, 2023

weihanglo commented Oct 17, 2023

rfcbot commented Oct 27, 2023

weihanglo commented Oct 27, 2023

bors commented Oct 27, 2023

bors commented Oct 27, 2023

bors commented Oct 27, 2023

epage commented Oct 6, 2023 •

edited

rfcbot commented Oct 6, 2023 •

edited by ehuss