Skip to content

Commit

Permalink
Implement the 'frontend' of public-private dependencies
Browse files Browse the repository at this point in the history
This is part of rust-lang/rust#44663

This implements the 'frontend' portion of RFC 1977. Once PRs
rust-lang/rust#59335 and
rust-lang/crates.io#1685 are merged,
it will be possible to test the full public-private dependency feature:
marking a dependency a public, seeing exported_private_dependencies
warnings from rustc, and seeing pub-dep-reachability errors from Cargo.

Everything in this commit should be fully backwards-compatible - users
who don't enable the 'public-dependency' cargo feature won't notice any
changes.

Note that this commit does *not* implement the remaining two features of
the RFC:

* Choosing smallest versions when 'cargo publish' is run
* Turning exported_private_dependencies warnings into hard errors when
'cargo publish' is run

The former is a major change to Cargo's behavior, and should be done
in a separate PR with some kind of rollout plan.

The latter is described by the RFC as being enabled at 'some point in
the future'. This can be done via a follow-up PR.
  • Loading branch information
Aaron1011 committed Apr 26, 2019
1 parent c9330fe commit 87f1a4b
Show file tree
Hide file tree
Showing 11 changed files with 267 additions and 8 deletions.
5 changes: 5 additions & 0 deletions src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ impl<'a, 'cfg> BuildContext<'a, 'cfg> {
.extern_crate_name(unit.pkg.package_id(), dep.pkg.package_id(), dep.target)
}

pub fn is_public_dependency(&self, unit: &Unit<'a>, dep: &Unit<'a>) -> bool {
self.resolve
.is_public_dep(unit.pkg.package_id(), dep.pkg.package_id())
}

/// Whether a dependency should be compiled for the host or target platform,
/// specified by `Kind`.
pub fn dep_platform_activated(&self, dep: &Dependency, kind: Kind) -> bool {
Expand Down
20 changes: 19 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use log::debug;
use same_file::is_same_file;
use serde::Serialize;

use crate::core::Feature;
pub use self::build_config::{BuildConfig, CompileMode, MessageFormat};
pub use self::build_context::{BuildContext, FileFlavor, TargetConfig, TargetInfo};
use self::build_plan::BuildPlan;
Expand Down Expand Up @@ -966,22 +967,32 @@ fn build_deps_args<'a, 'cfg>(
}
}

let mut unstable_opts = false;

for dep in dep_targets {
if dep.mode.is_run_custom_build() {
cmd.env("OUT_DIR", &cx.files().build_script_out_dir(&dep));
}
if dep.target.linkable() && !dep.mode.is_doc() {
link_to(cmd, cx, unit, &dep)?;
link_to(cmd, cx, unit, &dep, &mut unstable_opts)?;
}
}

// This will only be set if we're already usign a feature
// requiring nightly rust
if unstable_opts {
cmd.arg("-Z").arg("unstable-options");
}


return Ok(());

fn link_to<'a, 'cfg>(
cmd: &mut ProcessBuilder,
cx: &mut Context<'a, 'cfg>,
current: &Unit<'a>,
dep: &Unit<'a>,
need_unstable_opts: &mut bool
) -> CargoResult<()> {
let bcx = cx.bcx;
for output in cx.outputs(dep)?.iter() {
Expand All @@ -996,6 +1007,13 @@ fn build_deps_args<'a, 'cfg>(
v.push(&path::MAIN_SEPARATOR.to_string());
v.push(&output.path.file_name().unwrap());
cmd.arg("--extern").arg(&v);

if current.pkg.manifest().features().require(Feature::public_dependency()).is_ok() {
if !bcx.is_public_dependency(current, dep) {
cmd.arg("--extern-private").arg(&v);
*need_unstable_opts = true
}
}
}
Ok(())
}
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,9 @@ features! {

// Declarative build scripts.
[unstable] metabuild: bool,

// Specifying the 'public' attribute on dependencies
[unstable] public_dependency: bool,
}
}

Expand Down
25 changes: 25 additions & 0 deletions src/cargo/core/resolver/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use std::iter::FromIterator;
use url::Url;

use crate::core::{Dependency, PackageId, PackageIdSpec, Summary, Target};
use crate::core::dependency::Kind;
use crate::util::errors::CargoResult;
use crate::util::Graph;

Expand All @@ -29,6 +30,8 @@ pub struct Resolve {
checksums: HashMap<PackageId, Option<String>>,
metadata: Metadata,
unused_patches: Vec<PackageId>,
// A map from packages to a set of their public dependencies
public_dependencies: HashMap<PackageId, HashSet<PackageId>>,
}

impl Resolve {
Expand All @@ -41,6 +44,21 @@ impl Resolve {
unused_patches: Vec<PackageId>,
) -> Resolve {
let reverse_replacements = replacements.iter().map(|(&p, &r)| (r, p)).collect();
let public_dependencies = graph.iter().map(|p| {
let public_deps = graph.edges(p).flat_map(|(dep_package, deps)| {
let id_opt: Option<PackageId> = deps.iter().find(|d| d.kind() == Kind::Normal).and_then(|d| {
if d.is_public() {
Some(dep_package.clone())
} else {
None
}
});
id_opt
}).collect::<HashSet<PackageId>>();

(p.clone(), public_deps)
}).collect();

Resolve {
graph,
replacements,
Expand All @@ -50,6 +68,7 @@ impl Resolve {
unused_patches,
empty_features: HashSet::new(),
reverse_replacements,
public_dependencies
}
}

Expand Down Expand Up @@ -197,6 +216,12 @@ unable to verify that `{0}` is the same as when the lockfile was generated
self.features.get(&pkg).unwrap_or(&self.empty_features)
}

pub fn is_public_dep(&self, pkg: PackageId, dep: PackageId) -> bool {
self.public_dependencies.get(&pkg)
.map(|public_deps| public_deps.contains(&dep))
.unwrap_or_else(|| panic!("Unknown dependency {:?} for package {:?}", dep, pkg))
}

pub fn features_sorted(&self, pkg: PackageId) -> Vec<&str> {
let mut v = Vec::from_iter(self.features(pkg).iter().map(|s| s.as_ref()));
v.sort_unstable();
Expand Down
13 changes: 9 additions & 4 deletions src/cargo/core/workspace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use glob::glob;
use log::debug;
use url::Url;

use crate::core::features::Features;
use crate::core::profiles::Profiles;
use crate::core::registry::PackageRegistry;
use crate::core::{Dependency, PackageIdSpec, PackageId};
Expand Down Expand Up @@ -540,17 +541,21 @@ impl<'cfg> Workspace<'cfg> {
Ok(())
}

pub fn features(&self) -> &Features {
match self.root_maybe() {
MaybePackage::Package(p) => p.manifest().features(),
MaybePackage::Virtual(vm) => vm.features(),
}
}

/// Validates a workspace, ensuring that a number of invariants are upheld:
///
/// 1. A workspace only has one root.
/// 2. All workspace members agree on this one root as the root.
/// 3. The current crate is a member of this workspace.
fn validate(&mut self) -> CargoResult<()> {
// Validate config profiles only once per workspace.
let features = match self.root_maybe() {
MaybePackage::Package(p) => p.manifest().features(),
MaybePackage::Virtual(vm) => vm.features(),
};
let features = self.features();
let mut warnings = Vec::new();
self.config.profiles()?.validate(features, &mut warnings)?;
for warning in warnings {
Expand Down
11 changes: 10 additions & 1 deletion src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use crate::core::resolver::Method;
use crate::core::{
Package, PackageId, PackageIdSpec, PackageSet, Resolve, Source, SourceId, Verbosity, Workspace,
};
use crate::core::Feature;
use crate::ops;
use crate::sources::PathSource;
use crate::util::errors::{CargoResult, CargoResultExt};
Expand Down Expand Up @@ -590,6 +591,14 @@ fn run_verify(ws: &Workspace<'_>, tar: &FileLock, opts: &PackageOpts<'_>) -> Car
let pkg_fingerprint = hash_all(&dst)?;
let ws = Workspace::ephemeral(new_pkg, config, None, true)?;

let rustc_args = if pkg.manifest().features().require(Feature::public_dependency()).is_ok() {
// FIXME: Turn this on at some point in the future
//Some(vec!["-D exported_private_dependencies".to_string()])
None
} else {
None
};

let exec: Arc<dyn Executor> = Arc::new(DefaultExecutor);
ops::compile_ws(
&ws,
Expand All @@ -604,7 +613,7 @@ fn run_verify(ws: &Workspace<'_>, tar: &FileLock, opts: &PackageOpts<'_>) -> Car
required_features_filterable: true,
},
target_rustdoc_args: None,
target_rustc_args: None,
target_rustc_args: rustc_args,
local_rustdoc_args: None,
export_dir: None,
},
Expand Down
3 changes: 2 additions & 1 deletion src/cargo/ops/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::collections::HashSet;

use log::{debug, trace};

use crate::core::Feature;
use crate::core::registry::PackageRegistry;
use crate::core::resolver::{self, Method, Resolve};
use crate::core::{PackageId, PackageIdSpec, PackageSet, Source, SourceId, Workspace};
Expand Down Expand Up @@ -330,7 +331,7 @@ pub fn resolve_with_previous<'cfg>(
registry,
&try_to_use,
Some(ws.config()),
false, // TODO: use "public and private dependencies" feature flag
ws.features().require(Feature::public_dependency()).is_ok(),
)?;
resolved.register_used_patches(registry.patches());
if register_patches {
Expand Down
8 changes: 7 additions & 1 deletion src/cargo/sources/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ struct RegistryDependency<'a> {
kind: Option<Cow<'a, str>>,
registry: Option<Cow<'a, str>>,
package: Option<Cow<'a, str>>,
public: Option<bool>
}

impl<'a> RegistryDependency<'a> {
Expand All @@ -300,6 +301,7 @@ impl<'a> RegistryDependency<'a> {
kind,
registry,
package,
public
} = self;

let id = if let Some(registry) = &registry {
Expand All @@ -324,6 +326,9 @@ impl<'a> RegistryDependency<'a> {
None => None,
};

// All dependencies are private by default
let public = public.unwrap_or(false);

// Unfortunately older versions of cargo and/or the registry ended up
// publishing lots of entries where the features array contained the
// empty feature, "", inside. This confuses the resolution process much
Expand All @@ -341,7 +346,8 @@ impl<'a> RegistryDependency<'a> {
.set_default_features(default_features)
.set_features(features)
.set_platform(platform)
.set_kind(kind);
.set_kind(kind)
.set_public(public);

Ok(dep)
}
Expand Down
6 changes: 6 additions & 0 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ pub struct DetailedTomlDependency {
#[serde(rename = "default_features")]
default_features2: Option<bool>,
package: Option<String>,
public: Option<bool>
}

#[derive(Debug, Deserialize, Serialize)]
Expand Down Expand Up @@ -1461,6 +1462,11 @@ impl DetailedTomlDependency {
cx.features.require(Feature::rename_dependency())?;
dep.set_explicit_name_in_toml(name_in_toml);
}

if let Some(p) = self.public {
cx.features.require(Feature::public_dependency())?;
dep.set_public(p);
}
Ok(dep)
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ mod profile_config;
mod profile_overrides;
mod profile_targets;
mod profiles;
mod pub_priv;
mod publish;
mod publish_lockfile;
mod read_manifest;
Expand Down
Loading

0 comments on commit 87f1a4b

Please sign in to comment.