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

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

Merged
merged 6 commits into from Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/cargo/ops/registry/publish.rs
Expand Up @@ -103,7 +103,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
if allowed_registries.is_empty() {
bail!(
"`{}` cannot be published.\n\
`package.publish` is set to `false` or an empty list in Cargo.toml and prevents publishing.",
`package.publish` must be set to `true` or a non-empty list in Cargo.toml to publish.",
pkg.name(),
);
} else if !allowed_registries.contains(&reg_name) {
Expand Down
12 changes: 0 additions & 12 deletions src/cargo/util/toml/embedded.rs
Expand Up @@ -6,8 +6,6 @@ use crate::Config;

const DEFAULT_EDITION: crate::core::features::Edition =
crate::core::features::Edition::LATEST_STABLE;
const DEFAULT_VERSION: &str = "0.0.0";
const DEFAULT_PUBLISH: bool = false;
const AUTO_FIELDS: &[&str] = &["autobins", "autoexamples", "autotests", "autobenches"];

pub fn expand_manifest(
Expand Down Expand Up @@ -123,9 +121,6 @@ fn expand_manifest_(
package
.entry("name".to_owned())
.or_insert(toml::Value::String(name));
package
.entry("version".to_owned())
.or_insert_with(|| toml::Value::String(DEFAULT_VERSION.to_owned()));
package.entry("edition".to_owned()).or_insert_with(|| {
let _ = config.shell().warn(format_args!(
"`package.edition` is unspecified, defaulting to `{}`",
Expand All @@ -136,9 +131,6 @@ fn expand_manifest_(
package
.entry("build".to_owned())
.or_insert_with(|| toml::Value::Boolean(false));
package
.entry("publish".to_owned())
.or_insert_with(|| toml::Value::Boolean(DEFAULT_PUBLISH));
for field in AUTO_FIELDS {
package
.entry(field.to_owned())
Expand Down Expand Up @@ -621,8 +613,6 @@ autotests = false
build = false
edition = "2021"
name = "test-"
publish = false
version = "0.0.0"

[profile.release]
strip = true
Expand Down Expand Up @@ -651,8 +641,6 @@ autotests = false
build = false
edition = "2021"
name = "test-"
publish = false
version = "0.0.0"

[profile.release]
strip = true
Expand Down
55 changes: 33 additions & 22 deletions src/cargo/util/toml/mod.rs
Expand Up @@ -550,11 +550,17 @@ impl TomlManifest {
let version = package
.version
.clone()
.resolve("version", || inherit()?.version())?;
.map(|version| version.resolve("version", || inherit()?.version()))
.transpose()?;

package.version = MaybeWorkspace::Defined(version.clone());
package.version = version.clone().map(MaybeWorkspace::Defined);

let pkgid = package.to_package_id(source_id, version)?;
let pkgid = package.to_package_id(
source_id,
version
.clone()
.unwrap_or_else(|| semver::Version::new(0, 0, 0)),
)?;

let edition = if let Some(edition) = package.edition.clone() {
let edition: Edition = edition
Expand Down Expand Up @@ -1006,9 +1012,14 @@ impl TomlManifest {
let publish = match publish {
Some(VecStringOrBool::VecString(ref vecstring)) => Some(vecstring.clone()),
Some(VecStringOrBool::Bool(false)) => Some(vec![]),
None | Some(VecStringOrBool::Bool(true)) => None,
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");
}
Comment on lines +1015 to +1021
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

/// 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if summary.features().contains_key("default-features") {
warnings.push(
"`default-features = [\"..\"]` was found in [features]. \
Expand Down Expand Up @@ -1659,8 +1670,7 @@ pub struct TomlPackage {
edition: Option<MaybeWorkspaceString>,
rust_version: Option<MaybeWorkspaceRustVersion>,
name: InternedString,
#[serde(deserialize_with = "version_trim_whitespace")]
version: MaybeWorkspaceSemverVersion,
version: Option<MaybeWorkspaceSemverVersion>,
authors: Option<MaybeWorkspaceVecString>,
build: Option<StringOrBool>,
metabuild: Option<StringOrVec>,
Expand Down Expand Up @@ -1709,22 +1719,6 @@ impl TomlPackage {
}
}

fn version_trim_whitespace<'de, D>(deserializer: D) -> Result<MaybeWorkspaceSemverVersion, D::Error>
where
D: de::Deserializer<'de>,
{
UntaggedEnumVisitor::new()
.expecting("SemVer version")
.string(
|value| match value.trim().parse().map_err(de::Error::custom) {
Ok(parsed) => Ok(MaybeWorkspace::Defined(parsed)),
Err(e) => Err(e),
},
)
.map(|value| value.deserialize().map(MaybeWorkspace::Workspace))
.deserialize(deserializer)
}

/// This Trait exists to make [`MaybeWorkspace::Workspace`] generic. It makes deserialization of
/// [`MaybeWorkspace`] much easier, as well as making error messages for
/// [`MaybeWorkspace::resolve`] much nicer
Expand Down Expand Up @@ -1793,6 +1787,23 @@ impl<T, W: WorkspaceInherit> MaybeWorkspace<T, W> {

//. This already has a `Deserialize` impl from version_trim_whitespace
type MaybeWorkspaceSemverVersion = MaybeWorkspace<semver::Version, TomlWorkspaceField>;
impl<'de> de::Deserialize<'de> for MaybeWorkspaceSemverVersion {
fn deserialize<D>(d: D) -> Result<Self, D::Error>
where
D: de::Deserializer<'de>,
{
UntaggedEnumVisitor::new()
.expecting("SemVer version")
.string(
|value| match value.trim().parse().map_err(de::Error::custom) {
Ok(parsed) => Ok(MaybeWorkspace::Defined(parsed)),
Err(e) => Err(e),
},
)
.map(|value| value.deserialize().map(MaybeWorkspace::Workspace))
.deserialize(d)
}
}

type MaybeWorkspaceString = MaybeWorkspace<String, TomlWorkspaceField>;
impl<'de> de::Deserialize<'de> for MaybeWorkspaceString {
Expand Down
19 changes: 10 additions & 9 deletions src/doc/src/reference/manifest.md
Expand Up @@ -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`. The field is required for publishing packages.

[Resolver]: resolver.md
[SemVer compatibility]: semver.md

Expand Down Expand Up @@ -470,23 +472,22 @@ if any of those files change.

### The `publish` field

The `publish` field can be used to prevent a package from being published to a
package registry (like *crates.io*) by mistake, for instance to keep a package
private in a company.

The `publish` field can be used to control which registries names the package
may be published to:
```toml
[package]
# ...
publish = false
publish = ["some-registry-name"]
```

The value may also be an array of strings which are registry names that are
allowed to be published to.

To prevent a package from being published to a registry (like crates.io) by mistake,
for instance to keep a package private in a company,
you can omit the [`version`](#the-version-field) field.
If you'd like to be more explicit, you can disable publishing:
```toml
[package]
# ...
publish = ["some-registry-name"]
publish = false
```

If publish array contains a single registry, `cargo publish` command will use
Expand Down
3 changes: 0 additions & 3 deletions src/doc/src/reference/unstable.md
Expand Up @@ -1221,9 +1221,6 @@ at the start of the infostring at the top of the file.

Inferred / defaulted manifest fields:
- `package.name = <slugified file stem>`
- `package.version = "0.0.0"` to [call attention to this crate being used in unexpected places](https://matklad.github.io/2021/08/22/large-rust-workspaces.html#Smaller-Tips)
- `package.publish = false` to avoid accidental publishes, particularly if we
later add support for including them in a workspace.
- `package.edition = <current>` to avoid always having to add an embedded
manifest at the cost of potentially breaking scripts on rust upgrades
- Warn when `edition` is unspecified to raise awareness of this
Expand Down
23 changes: 23 additions & 0 deletions tests/testsuite/check.rs
Expand Up @@ -1496,3 +1496,26 @@ fn check_unused_manifest_keys() {
)
.run();
}

#[cargo_test]
fn versionless_package() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
description = "foo"
"#,
)
.file("src/lib.rs", "")
.build();
p.cargo("check")
.with_stderr(
"\
[CHECKING] foo v0.0.0 ([CWD])
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
)
.run();
}