Skip to content

Commit

Permalink
Auto merge of #14408 - tweag:infer-registry-ignore-unpublishable, r=e…
Browse files Browse the repository at this point in the history 
…page

Be more permissive while packaging unpublishable crates.

This PR allows for packaging workspaces that include `publish = false` crates, in some circumstances:

- unpublishable crates are ignored when inferring the publish registry
- when checking whether the publish registry is valid, we ignore unpublishable crates
- we don't put unpublishable crates in the local registry overlay, so if any workspace crates depend on an unpublishable crate then they will fail to verify.

This PR also contains a refactor, moving the registry inference logic to `registry/mod.rs`, where it will be reused by the upcoming publish-workspace feature. I put the refactor and the logic changes in different commits.

Fixes #14356
  • Loading branch information
@bors
bors committed Aug 20, 2024
2 parents 1f4ef10 + 96d4d6f commit b2430df
Show file tree
Hide file tree
Showing 3 changed files with 281 additions and 103 deletions.
166 changes: 66 additions & 100 deletions src/cargo/ops/cargo_package.rs
View file
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 {
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
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

0 comments on commit b2430df

Please sign in to comment.