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

fix(publish): Don't strip non-dev features #14325

Merged
merged 8 commits into from
Jul 31, 2024
13 changes: 9 additions & 4 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Vec<Fi
} else {
let tarball = create_package(ws, &pkg, ar_files, local_reg.as_ref())?;
if let Some(local_reg) = local_reg.as_mut() {
local_reg.add_package(&pkg, &tarball)?;
local_reg.add_package(ws, &pkg, &tarball)?;
}
outputs.push((pkg, opts, tarball));
}
Expand Down Expand Up @@ -893,7 +893,7 @@ fn tar(
.iter()
.map(|ar_file| ar_file.rel_path.clone())
.collect::<Vec<_>>();
let publish_pkg = prepare_for_publish(pkg, ws, &included)?;
let publish_pkg = prepare_for_publish(pkg, ws, Some(&included))?;

let mut uncompressed_size = 0;
for ar_file in ar_files {
Expand Down Expand Up @@ -1299,7 +1299,12 @@ impl<'a> TmpRegistry<'a> {
self.root.join("index")
}

fn add_package(&mut self, package: &Package, tar: &FileLock) -> CargoResult<()> {
fn add_package(
&mut self,
ws: &Workspace<'_>,
package: &Package,
tar: &FileLock,
) -> CargoResult<()> {
debug!(
"adding package {}@{} to local overlay at {}",
package.name(),
Expand All @@ -1317,7 +1322,7 @@ impl<'a> TmpRegistry<'a> {
tar_copy.flush()?;
}

let new_crate = super::registry::prepare_transmit(self.gctx, package, self.upstream)?;
let new_crate = super::registry::prepare_transmit(self.gctx, ws, package, self.upstream)?;

tar.file().seek(SeekFrom::Start(0))?;
let cksum = cargo_util::Sha256::new()
Expand Down
60 changes: 23 additions & 37 deletions src/cargo/ops/registry/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
//! [1]: https://doc.rust-lang.org/nightly/cargo/reference/registry-web-api.html#publish

use std::collections::BTreeMap;
use std::collections::BTreeSet;
use std::collections::HashSet;
use std::fs::File;
use std::time::Duration;
Expand All @@ -21,7 +20,6 @@ use crate::core::dependency::DepKind;
use crate::core::manifest::ManifestMetadata;
use crate::core::resolver::CliFeatures;
use crate::core::Dependency;
use crate::core::FeatureValue;
use crate::core::Package;
use crate::core::PackageIdSpecQuery;
use crate::core::SourceId;
Expand All @@ -35,7 +33,7 @@ use crate::sources::CRATES_IO_REGISTRY;
use crate::util::auth;
use crate::util::cache_lock::CacheLockMode;
use crate::util::context::JobsConfig;
use crate::util::interning::InternedString;
use crate::util::toml::prepare_for_publish;
use crate::util::Progress;
use crate::util::ProgressStyle;
use crate::CargoResult;
Expand Down Expand Up @@ -185,6 +183,7 @@ pub fn publish(ws: &Workspace<'_>, opts: &PublishOpts<'_>) -> CargoResult<()> {
.status("Uploading", pkg.package_id().to_string())?;
transmit(
opts.gctx,
ws,
pkg,
tarball.file(),
&mut registry,
Expand Down Expand Up @@ -324,16 +323,16 @@ fn verify_dependencies(

pub(crate) fn prepare_transmit(
gctx: &GlobalContext,
pkg: &Package,
ws: &Workspace<'_>,
local_pkg: &Package,
registry_id: SourceId,
) -> CargoResult<NewCrate> {
let deps = pkg
let included = None; // don't filter build-targets
Copy link
Member

Choose a reason for hiding this comment

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

it is unclear why the argument affects build target filtering.

let publish_pkg = prepare_for_publish(local_pkg, ws, included)?;

let deps = publish_pkg
.dependencies()
.iter()
.filter(|dep| {
// Skip dev-dependency without version.
dep.is_transitive() || dep.specified_req()
})
.map(|dep| {
// If the dependency is from a different registry, then include the
// registry in the dependency.
Expand Down Expand Up @@ -378,7 +377,7 @@ pub(crate) fn prepare_transmit(
})
})
.collect::<CargoResult<Vec<NewCrateDependency>>>()?;
let manifest = pkg.manifest();
let manifest = publish_pkg.manifest();
let ManifestMetadata {
ref authors,
ref description,
Expand All @@ -395,53 +394,39 @@ pub(crate) fn prepare_transmit(
ref rust_version,
} = *manifest.metadata();
let rust_version = rust_version.as_ref().map(ToString::to_string);
let readme_content = readme
let readme_content = local_pkg
.manifest()
.metadata()
.readme
.as_ref()
.map(|readme| {
paths::read(&pkg.root().join(readme))
.with_context(|| format!("failed to read `readme` file for package `{}`", pkg))
paths::read(&local_pkg.root().join(readme)).with_context(|| {
format!("failed to read `readme` file for package `{}`", local_pkg)
})
})
.transpose()?;
if let Some(ref file) = *license_file {
if !pkg.root().join(file).exists() {
if let Some(ref file) = local_pkg.manifest().metadata().license_file {
if !local_pkg.root().join(file).exists() {
bail!("the license file `{}` does not exist", file)
}
}

let deps_set = deps
.iter()
.map(|dep| dep.name.clone())
.collect::<BTreeSet<String>>();

let string_features = match manifest.resolved_toml().features() {
Some(features) => features
.iter()
.map(|(feat, values)| {
(
feat.to_string(),
values
.iter()
.filter(|fv| {
let feature_value = FeatureValue::new(InternedString::new(fv));
match feature_value {
FeatureValue::Dep { dep_name }
| FeatureValue::DepFeature { dep_name, .. } => {
deps_set.contains(&dep_name.to_string())
}
_ => true,
}
})
.map(|fv| fv.to_string())
.collect(),
values.iter().map(|fv| fv.to_string()).collect(),
)
})
.collect::<BTreeMap<String, Vec<String>>>(),
None => BTreeMap::new(),
};

Ok(NewCrate {
name: pkg.name().to_string(),
vers: pkg.version().to_string(),
name: publish_pkg.name().to_string(),
vers: publish_pkg.version().to_string(),
deps,
features: string_features,
authors: authors.clone(),
Expand All @@ -463,13 +448,14 @@ pub(crate) fn prepare_transmit(

fn transmit(
gctx: &GlobalContext,
ws: &Workspace<'_>,
pkg: &Package,
tarball: &File,
registry: &mut Registry,
registry_id: SourceId,
dry_run: bool,
) -> CargoResult<()> {
let new_crate = prepare_transmit(gctx, pkg, registry_id)?;
let new_crate = prepare_transmit(gctx, ws, pkg, registry_id)?;

// Do not upload if performing a dry run
if dry_run {
Expand Down
45 changes: 26 additions & 19 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2551,15 +2551,16 @@ fn unused_dep_keys(
}
}

/// Make the [`Package`] self-contained so its ready for packaging
pub fn prepare_for_publish(
Copy link
Member

Choose a reason for hiding this comment

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

It is pretty obscure what the included argument is for. Could we document this function as well as arguments to make it clear? Something like

Suggested change
pub fn prepare_for_publish(
/// Makes the given [`Package`] into a self-contained, publish-ready `Package`.
///
/// The `included` argument is for optionally checking if a given path in a field is reachable.
///
/// See below for more details on what will be processed:
///
/// * [`prepare_toml_for_publish`]
/// * [`to_real_manifest`]
pub fn prepare_for_publish(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_real_manifest is just to reuse the Package / Summary generation logic. At that point, I don't feel like calling out prepare_toml_for_publish really gains much as its name is so tied to this and it immediately follows it (in code, in rustdoc side bar which is alphabetical, and in rustdocs items which are in code-order).

I'm trying a rename of included to see if that clarifies it. I feel weird saying too much more without also specifying all of the other behavior.

me: &Package,
ws: &Workspace<'_>,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
) -> CargoResult<Package> {
let contents = me.manifest().contents();
let document = me.manifest().document();
let original_toml =
prepare_toml_for_publish(me.manifest().resolved_toml(), ws, me.root(), included)?;
prepare_toml_for_publish(me.manifest().resolved_toml(), ws, me.root(), packaged_files)?;
let resolved_toml = original_toml.clone();
let features = me.manifest().unstable_features().clone();
let workspace_config = me.manifest().workspace_config().clone();
Expand Down Expand Up @@ -2591,7 +2592,7 @@ fn prepare_toml_for_publish(
me: &manifest::TomlManifest,
ws: &Workspace<'_>,
package_root: &Path,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
) -> CargoResult<manifest::TomlManifest> {
let gctx = ws.gctx();

Expand All @@ -2608,7 +2609,8 @@ fn prepare_toml_for_publish(
package.workspace = None;
if let Some(StringOrBool::String(path)) = &package.build {
let path = paths::normalize_path(Path::new(path));
let build = if included.contains(&path) {
let included = packaged_files.map(|i| i.contains(&path)).unwrap_or(true);
let build = if included {
let path = path
.into_os_string()
.into_string()
Expand Down Expand Up @@ -2712,14 +2714,16 @@ fn prepare_toml_for_publish(
}

let lib = if let Some(target) = &me.lib {
prepare_target_for_publish(target, included, "library", ws.gctx())?
prepare_target_for_publish(target, packaged_files, "library", ws.gctx())?
} else {
None
};
let bin = prepare_targets_for_publish(me.bin.as_ref(), included, "binary", ws.gctx())?;
let example = prepare_targets_for_publish(me.example.as_ref(), included, "example", ws.gctx())?;
let test = prepare_targets_for_publish(me.test.as_ref(), included, "test", ws.gctx())?;
let bench = prepare_targets_for_publish(me.bench.as_ref(), included, "benchmark", ws.gctx())?;
let bin = prepare_targets_for_publish(me.bin.as_ref(), packaged_files, "binary", ws.gctx())?;
let example =
prepare_targets_for_publish(me.example.as_ref(), packaged_files, "example", ws.gctx())?;
let test = prepare_targets_for_publish(me.test.as_ref(), packaged_files, "test", ws.gctx())?;
let bench =
prepare_targets_for_publish(me.bench.as_ref(), packaged_files, "benchmark", ws.gctx())?;

let all = |_d: &manifest::TomlDependency| true;
let mut manifest = manifest::TomlManifest {
Expand Down Expand Up @@ -2877,7 +2881,7 @@ fn prepare_toml_for_publish(

fn prepare_targets_for_publish(
targets: Option<&Vec<manifest::TomlTarget>>,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
context: &str,
gctx: &GlobalContext,
) -> CargoResult<Option<Vec<manifest::TomlTarget>>> {
Expand All @@ -2887,7 +2891,8 @@ fn prepare_targets_for_publish(

let mut prepared = Vec::with_capacity(targets.len());
for target in targets {
let Some(target) = prepare_target_for_publish(target, included, context, gctx)? else {
let Some(target) = prepare_target_for_publish(target, packaged_files, context, gctx)?
else {
continue;
};
prepared.push(target);
Expand All @@ -2902,19 +2907,21 @@ fn prepare_targets_for_publish(

fn prepare_target_for_publish(
target: &manifest::TomlTarget,
included: &[PathBuf],
packaged_files: Option<&[PathBuf]>,
context: &str,
gctx: &GlobalContext,
) -> CargoResult<Option<manifest::TomlTarget>> {
let path = target.path.as_ref().expect("previously resolved");
let path = normalize_path(&path.0);
if !included.contains(&path) {
let name = target.name.as_ref().expect("previously resolved");
gctx.shell().warn(format!(
"ignoring {context} `{name}` as `{}` is not included in the published package",
path.display()
))?;
return Ok(None);
if let Some(packaged_files) = packaged_files {
if !packaged_files.contains(&path) {
let name = target.name.as_ref().expect("previously resolved");
gctx.shell().warn(format!(
"ignoring {context} `{name}` as `{}` is not included in the published package",
path.display()
))?;
return Ok(None);
}
}

let mut target = target.clone();
Expand Down
4 changes: 2 additions & 2 deletions tests/testsuite/inheritable_workspace_fields.rs
Original file line number Diff line number Diff line change
Expand Up @@ -752,11 +752,11 @@ You may press ctrl-c to skip waiting; the crate should be available shortly.
"homepage": "https://www.rust-lang.org",
"keywords": ["cli"],
"license": "MIT",
"license_file": "../LICENSE",
"license_file": "LICENSE",
"links": null,
"name": "bar",
"readme": "README.md",
"readme_file": "../README.md",
"readme_file": "README.md",
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a side effect that fixes another bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I considered tweaking things to not have this happen but it seemed more correct now.

"repository": "https://github.com/example/example",
"rust_version": "1.60",
"vers": "1.2.3"
Expand Down
Loading