Skip to content

Commit

Permalink
Auto merge of #13518 - baby230211:fix/strip-feature-dev-dep, r=epage
Browse files Browse the repository at this point in the history
fix: strip feature dep when dep is dev dep

### What does this PR try to resolve?
This change aims to strip features dependencies without a version key to be published.
If a dev-dependency is missing the version, it will be stripped from the packaged manifest.

The features table may contains the deps in following places.
- Target
  - normal
  - dev
  - build
  - normal-and-dev
- normal
- dev
- build
- normal-and-dev

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

See the initial commit, it shows current behavior that will cause error when feature has deps that point to dev_dep and doesn't have a version specified.

Title | orignal toml | published toml |
---- | ---- | ---- |
Before |   feature = ["dev-dep/feature"]  <br/> [dev-dependencies] <br/> dev-dep = { .., features: ["feature"] }   |   feature = ["dev-dep/feature"] <br/> [dev-dependencies] <br/> dev-dep = { .., features: ["feature"] } |
After |  feature = ["dev-dep/feature"]  <br/> [dev-dependencies] <br/> dev-dep = { .., features: ["feature"] }  | feature = [] <br/> [dev-dependencies] ```

Fix: #12225
  • Loading branch information
bors committed Mar 14, 2024
2 parents 601670a + 75130eb commit 403fbe2
Show file tree
Hide file tree
Showing 3 changed files with 369 additions and 5 deletions.
23 changes: 22 additions & 1 deletion src/cargo/ops/registry/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
//! [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 @@ -20,6 +21,7 @@ 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 @@ -33,6 +35,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::Progress;
use crate::util::ProgressStyle;
use crate::CargoResult;
Expand Down Expand Up @@ -412,13 +415,31 @@ fn transmit(
return Ok(());
}

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

let string_features = match manifest.original().features() {
Some(features) => features
.iter()
.map(|(feat, values)| {
(
feat.to_string(),
values.iter().map(|fv| fv.to_string()).collect(),
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(),
)
})
.collect::<BTreeMap<String, Vec<String>>>(),
Expand Down
53 changes: 49 additions & 4 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ use crate::AlreadyPrintedError;
use anyhow::{anyhow, bail, Context as _};
use cargo_platform::Platform;
use cargo_util::paths;
use cargo_util_schemas::manifest;
use cargo_util_schemas::manifest::RustVersion;
use cargo_util_schemas::manifest::{self, TomlManifest};
use itertools::Itertools;
use lazycell::LazyCell;
use pathdiff::diff_paths;
Expand All @@ -21,7 +21,7 @@ use crate::core::compiler::{CompileKind, CompileTarget};
use crate::core::dependency::{Artifact, ArtifactTarget, DepKind};
use crate::core::manifest::{ManifestMetadata, TargetSourcePath, Warnings};
use crate::core::resolver::ResolveBehavior;
use crate::core::{find_workspace_root, resolve_relative_path, CliUnstable};
use crate::core::{find_workspace_root, resolve_relative_path, CliUnstable, FeatureValue};
use crate::core::{Dependency, Manifest, PackageId, Summary, Target};
use crate::core::{Edition, EitherManifest, Feature, Features, VirtualManifest, Workspace};
use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, WorkspaceRootConfig};
Expand Down Expand Up @@ -316,7 +316,7 @@ pub fn prepare_for_publish(
}
}
let all = |_d: &manifest::TomlDependency| true;
return Ok(manifest::TomlManifest {
let mut manifest = manifest::TomlManifest {
package: Some(package),
project: None,
profile: me.profile.clone(),
Expand Down Expand Up @@ -366,7 +366,52 @@ pub fn prepare_for_publish(
badges: me.badges.clone(),
cargo_features: me.cargo_features.clone(),
lints: me.lints.clone(),
});
};
strip_features(&mut manifest);
return Ok(manifest);

fn strip_features(manifest: &mut TomlManifest) {
fn insert_dep_name(
dep_name_set: &mut BTreeSet<manifest::PackageName>,
deps: Option<&BTreeMap<manifest::PackageName, manifest::InheritableDependency>>,
) {
let Some(deps) = deps else {
return;
};
deps.iter().for_each(|(k, _v)| {
dep_name_set.insert(k.clone());
});
}
let mut dep_name_set = BTreeSet::new();
insert_dep_name(&mut dep_name_set, manifest.dependencies.as_ref());
insert_dep_name(&mut dep_name_set, manifest.dev_dependencies());
insert_dep_name(&mut dep_name_set, manifest.build_dependencies());
if let Some(target_map) = manifest.target.as_ref() {
target_map.iter().for_each(|(_k, v)| {
insert_dep_name(&mut dep_name_set, v.dependencies.as_ref());
insert_dep_name(&mut dep_name_set, v.dev_dependencies());
insert_dep_name(&mut dep_name_set, v.build_dependencies());
});
}
let features = manifest.features.as_mut();

let Some(features) = features else {
return;
};

features.values_mut().for_each(|feature_deps| {
feature_deps.retain(|feature_dep| {
let feature_value = FeatureValue::new(InternedString::new(feature_dep));
match feature_value {
FeatureValue::Dep { dep_name } | FeatureValue::DepFeature { dep_name, .. } => {
let k = &manifest::PackageName::new(dep_name.to_string()).unwrap();
dep_name_set.contains(k)
}
_ => true,
}
});
});
}

fn map_deps(
gctx: &GlobalContext,
Expand Down
Loading

0 comments on commit 403fbe2

Please sign in to comment.