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.

Sign up for GitHub

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

Jump to bottom

Be more permissive while packaging unpublishable crates. #14408

Merged
merged 5 commits into from
Aug 20, 2024
Merged

Be more permissive while packaging unpublishable crates. #14408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7919bda
Add misbehaving test for packaging unpublishable workspace
jneem Aug 15, 2024
37812e3
Split infer_registry into inference and validation parts.
jneem Aug 19, 2024
b3860a4
Move infer_registry into a more public spot
jneem Aug 19, 2024
a9987f0
Use the shared source building in package
jneem Aug 19, 2024
96d4d6f
Allow unpublishable crates to be packaged
jneem Aug 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 66 additions & 100 deletions src/cargo/ops/cargo_package.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use std::collections::{BTreeSet, HashMap, HashSet};
use std::collections::{BTreeSet, HashMap};
use std::fs::{self, File};
use std::io::prelude::*;
use std::io::SeekFrom;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use std::task::Poll;

use super::RegistryOrIndex;
use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor};
use crate::core::dependency::DepKind;
use crate::core::manifest::Target;
Expand All @@ -15,8 +14,9 @@ use crate::core::resolver::HasDevUnits;
use crate::core::{Feature, PackageIdSpecQuery, Shell, Verbosity, Workspace};
use crate::core::{Package, PackageId, PackageSet, Resolve, SourceId};
use crate::ops::lockfile::LOCKFILE_NAME;
use crate::ops::registry::{infer_registry, RegistryOrIndex};
use crate::sources::registry::index::{IndexPackage, RegistryDependency};
use crate::sources::{PathSource, SourceConfigMap, CRATES_IO_REGISTRY};
use crate::sources::{PathSource, CRATES_IO_REGISTRY};
use crate::util::cache_lock::CacheLockMode;
use crate::util::context::JobsConfig;
use crate::util::errors::CargoResult;
Expand Down Expand Up @@ -174,97 +174,6 @@ fn create_package(
return Ok(dst);
}

/// Determine which registry the packages are for.
///
/// The registry only affects the built packages if there are dependencies within the
/// packages that we're packaging: if we're packaging foo-bin and foo-lib, and foo-bin
/// depends on foo-lib, then the foo-lib entry in foo-bin's lockfile will depend on the
/// registry that we're building packages for.
fn infer_registry(
gctx: &GlobalContext,
pkgs: &[&Package],
reg_or_index: Option<RegistryOrIndex>,
) -> CargoResult<SourceId> {
let reg_or_index = match reg_or_index {
Some(r) => r,
None => {
if pkgs[1..].iter().all(|p| p.publish() == pkgs[0].publish()) {
// If all packages have the same publish settings, we take that as the default.
match pkgs[0].publish().as_deref() {
Some([unique_pkg_reg]) => RegistryOrIndex::Registry(unique_pkg_reg.to_owned()),
None | Some([]) => RegistryOrIndex::Registry(CRATES_IO_REGISTRY.to_owned()),
Some([reg, ..]) if pkgs.len() == 1 => {
// For backwards compatibility, avoid erroring if there's only one package.
// The registry doesn't affect packaging in this case.
RegistryOrIndex::Registry(reg.to_owned())
}
Some(regs) => {
let mut regs: Vec<_> = regs.iter().map(|s| format!("\"{}\"", s)).collect();
regs.sort();
regs.dedup();
// unwrap: the match block ensures that there's more than one reg.
let (last_reg, regs) = regs.split_last().unwrap();
bail!(
"--registry is required to disambiguate between {} or {} registries",
regs.join(", "),
last_reg
)
}
}
} else {
let common_regs = pkgs
.iter()
// `None` means "all registries", so drop them instead of including them
// in the intersection.
.filter_map(|p| p.publish().as_deref())
.map(|p| p.iter().collect::<HashSet<_>>())
.reduce(|xs, ys| xs.intersection(&ys).cloned().collect())
.unwrap_or_default();
if common_regs.is_empty() {
bail!("conflicts between `package.publish` fields in the selected packages");
} else {
bail!(
"--registry is required because not all `package.publish` settings agree",
);
}
}
}
};

// Validate the registry against the packages' allow-lists. For backwards compatibility, we
// skip this if only a single package is being published (because in that case the registry
// doesn't affect the packaging step).
if pkgs.len() > 1 {
if let RegistryOrIndex::Registry(reg_name) = &reg_or_index {
for pkg in pkgs {
if let Some(allowed) = pkg.publish().as_ref() {
if !allowed.iter().any(|a| a == reg_name) {
bail!(
"`{}` cannot be packaged.\n\
The registry `{}` is not listed in the `package.publish` value in Cargo.toml.",
pkg.name(),
reg_name
);
}
}
}
}
}

let sid = match reg_or_index {
RegistryOrIndex::Index(url) => SourceId::for_registry(&url)?,
RegistryOrIndex::Registry(reg) if reg == CRATES_IO_REGISTRY => SourceId::crates_io(gctx)?,
RegistryOrIndex::Registry(reg) => SourceId::alt_registry(gctx, &reg)?,
};

// Load source replacements that are built-in to Cargo.
let sid = SourceConfigMap::empty(gctx)?
.load(sid, &HashSet::new())?
.replaced_source_id();

Ok(sid)
}

/// Packages an entire workspace.
///
/// Returns the generated package files. If `opts.list` is true, skips
Expand Down Expand Up @@ -293,19 +202,28 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Vec<Fi
// below, and will be validated during the verification step.
}

let deps = local_deps(pkgs.iter().map(|(p, f)| ((*p).clone(), f.clone())));
let just_pkgs: Vec<_> = pkgs.iter().map(|p| p.0).collect();
let publish_reg = infer_registry(ws.gctx(), &just_pkgs, opts.reg_or_index.clone())?;
debug!("packaging for registry {publish_reg}");

// The publish registry doesn't matter unless there are local dependencies,
// so only try to get one if we need it. If they explicitly passed a
// registry on the CLI, we check it no matter what.
let sid = if deps.has_no_dependencies() && opts.reg_or_index.is_none() {
None
} else {
let sid = get_registry(ws.gctx(), &just_pkgs, opts.reg_or_index.clone())?;
debug!("packaging for registry {}", sid);
Some(sid)
};

let mut local_reg = if ws.gctx().cli_unstable().package_workspace {
let reg_dir = ws.target_dir().join("package").join("tmp-registry");
Some(TmpRegistry::new(ws.gctx(), reg_dir, publish_reg)?)
sid.map(|sid| TmpRegistry::new(ws.gctx(), reg_dir, sid))
.transpose()?
} else {
None
};

let deps = local_deps(pkgs.iter().map(|(p, f)| ((*p).clone(), f.clone())));

// Packages need to be created in dependency order, because dependencies must
// be added to our local overlay before we can create lockfiles that depend on them.
let sorted_pkgs = deps.sort();
Expand All @@ -325,7 +243,9 @@ 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(ws, &pkg, &tarball)?;
if pkg.publish() != &Some(Vec::new()) {
local_reg.add_package(ws, &pkg, &tarball)?;
}
}
outputs.push((pkg, opts, tarball));
}
Expand All @@ -343,6 +263,46 @@ pub fn package(ws: &Workspace<'_>, opts: &PackageOpts<'_>) -> CargoResult<Vec<Fi
Ok(outputs.into_iter().map(|x| x.2).collect())
}

/// Determine which registry the packages are for.
///
/// The registry only affects the built packages if there are dependencies within the
/// packages that we're packaging: if we're packaging foo-bin and foo-lib, and foo-bin
/// depends on foo-lib, then the foo-lib entry in foo-bin's lockfile will depend on the
/// registry that we're building packages for.
fn get_registry(
gctx: &GlobalContext,
pkgs: &[&Package],
reg_or_index: Option<RegistryOrIndex>,
) -> CargoResult<SourceId> {
let reg_or_index = match reg_or_index.clone() {
Some(r) => Some(r),
None => infer_registry(pkgs)?,
};

// Validate the registry against the packages' allow-lists.
let reg = reg_or_index
.clone()
.unwrap_or_else(|| RegistryOrIndex::Registry(CRATES_IO_REGISTRY.to_owned()));
if let RegistryOrIndex::Registry(reg_name) = reg {
for pkg in pkgs {
if let Some(allowed) = pkg.publish().as_ref() {
// If allowed is empty (i.e. package.publish is false), we let it slide.
// This allows packaging unpublishable packages (although packaging might
// fail later if the unpublishable package is a dependency of something else).
if !allowed.is_empty() && !allowed.iter().any(|a| a == &reg_name) {
bail!(
"`{}` cannot be packaged.\n\
The registry `{}` is not listed in the `package.publish` value in Cargo.toml.",
pkg.name(),
reg_name
);
}
}
}
}
Ok(ops::registry::get_source_id(gctx, reg_or_index.as_ref())?.replacement)
}

/// Just the part of the dependency graph that's between the packages we're packaging.
/// (Is the package name a good key? Does it uniquely identify packages?)
#[derive(Clone, Debug, Default)]
Expand All @@ -359,6 +319,12 @@ impl LocalDependencies {
.map(|name| self.packages[&name].clone())
.collect()
}

pub fn has_no_dependencies(&self) -> bool {
epage marked this conversation as resolved.
Show resolved Hide resolved
self.graph
.iter()
.all(|node| self.graph.edges(node).next().is_none())
}
}

/// Build just the part of the dependency graph that's between the given packages,
Expand Down
53 changes: 51 additions & 2 deletions src/cargo/ops/registry/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use cargo_credential::{Operation, Secret};
use crates_io::Registry;
use url::Url;

use crate::core::{PackageId, SourceId};
use crate::core::{Package, PackageId, SourceId};
use crate::sources::source::Source;
use crate::sources::{RegistrySource, SourceConfigMap};
use crate::util::auth;
Expand Down Expand Up @@ -191,7 +191,7 @@ fn registry(
///
/// The return value is a pair of `SourceId`s: The first may be a built-in replacement of
/// crates.io (such as index.crates.io), while the second is always the original source.
fn get_source_id(
pub(crate) fn get_source_id(
gctx: &GlobalContext,
reg_or_index: Option<&RegistryOrIndex>,
) -> CargoResult<RegistrySourceIds> {
Expand Down Expand Up @@ -322,3 +322,52 @@ pub(crate) struct RegistrySourceIds {
/// User-defined source replacement is not applied.
pub(crate) replacement: SourceId,
}

/// If this set of packages has an unambiguous publish registry, find it.
pub(crate) fn infer_registry(pkgs: &[&Package]) -> CargoResult<Option<RegistryOrIndex>> {
// Ignore "publish = false" packages while inferring the registry.
let publishable_pkgs: Vec<_> = pkgs
.iter()
.filter(|p| p.publish() != &Some(Vec::new()))
.collect();

let Some((first, rest)) = publishable_pkgs.split_first() else {
return Ok(None);
};

// If all packages have the same publish settings, we take that as the default.
if rest.iter().all(|p| p.publish() == first.publish()) {
match publishable_pkgs[0].publish().as_deref() {
Some([unique_pkg_reg]) => {
Ok(Some(RegistryOrIndex::Registry(unique_pkg_reg.to_owned())))
}
None | Some([]) => Ok(None),
Some(regs) => {
let mut regs: Vec<_> = regs.iter().map(|s| format!("\"{}\"", s)).collect();
regs.sort();
regs.dedup();
// unwrap: the match block ensures that there's more than one reg.
let (last_reg, regs) = regs.split_last().unwrap();
bail!(
"--registry is required to disambiguate between {} or {} registries",
regs.join(", "),
last_reg
)
}
}
} else {
let common_regs = publishable_pkgs
.iter()
// `None` means "all registries", so drop them instead of including them
// in the intersection.
.filter_map(|p| p.publish().as_deref())
.map(|p| p.iter().collect::<HashSet<_>>())
.reduce(|xs, ys| xs.intersection(&ys).cloned().collect())
.unwrap_or_default();
if common_regs.is_empty() {
bail!("conflicts between `package.publish` fields in the selected packages");
} else {
bail!("--registry is required because not all `package.publish` settings agree",);
}
}
}
Loading