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

Implement the 'frontend' of public-private dependencies #6772

Merged
merged 11 commits into from
Apr 26, 2019
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
11 changes: 9 additions & 2 deletions src/cargo/core/compiler/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,11 +315,13 @@ pub fn prepare_target<'a, 'cfg>(
/// A compilation unit dependency has a fingerprint that is comprised of:
/// * its package ID
/// * its extern crate name
/// * its public/private status
/// * its calculated fingerprint for the dependency
#[derive(Clone)]
struct DepFingerprint {
pkg_id: u64,
name: String,
public: bool,
fingerprint: Arc<Fingerprint>,
}

Expand Down Expand Up @@ -420,7 +422,7 @@ impl Serialize for DepFingerprint {
where
S: ser::Serializer,
{
(&self.pkg_id, &self.name, &self.fingerprint.hash()).serialize(ser)
(&self.pkg_id, &self.name, &self.public, &self.fingerprint.hash()).serialize(ser)
}
}

Expand All @@ -429,10 +431,11 @@ impl<'de> Deserialize<'de> for DepFingerprint {
where
D: de::Deserializer<'de>,
{
let (pkg_id, name, hash) = <(u64, String, u64)>::deserialize(d)?;
let (pkg_id, name, public, hash) = <(u64, String, bool, u64)>::deserialize(d)?;
Ok(DepFingerprint {
pkg_id,
name,
public,
fingerprint: Arc::new(Fingerprint {
memoized_hash: Mutex::new(Some(hash)),
..Fingerprint::new()
Expand Down Expand Up @@ -850,11 +853,13 @@ impl hash::Hash for Fingerprint {
for DepFingerprint {
pkg_id,
name,
public,
fingerprint,
} in deps
{
pkg_id.hash(h);
name.hash(h);
public.hash(h);
// use memoized dep hashes to avoid exponential blowup
h.write_u64(Fingerprint::hash(fingerprint));
}
Expand Down Expand Up @@ -900,6 +905,7 @@ impl DepFingerprint {
) -> CargoResult<DepFingerprint> {
let fingerprint = calculate(cx, dep)?;
let name = cx.bcx.extern_crate_name(parent, dep)?;
let public = cx.bcx.is_public_dependency(parent, dep);

// We need to be careful about what we hash here. We have a goal of
// supporting renaming a project directory and not rebuilding
Expand All @@ -920,6 +926,7 @@ impl DepFingerprint {
Ok(DepFingerprint {
pkg_id,
name,
public,
fingerprint,
})
}
Expand Down
25 changes: 23 additions & 2 deletions 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 @@ -995,7 +1006,17 @@ fn build_deps_args<'a, 'cfg>(
v.push(cx.files().out_dir(dep));
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() &&
!bcx.is_public_dependency(current, dep) {

cmd.arg("--extern-private");
*need_unstable_opts = true;
} else {
cmd.arg("--extern");
}

cmd.arg(&v);
}
Ok(())
}
Expand Down
4 changes: 4 additions & 0 deletions src/cargo/core/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,10 @@ impl Dependency {

/// Sets whether the dependency is public.
pub fn set_public(&mut self, public: bool) -> &mut Dependency {
if public {
// Setting 'public' only makes sense for normal dependencies
assert_eq!(self.kind(), Kind::Normal);
}
Rc::make_mut(&mut self.inner).public = public;
self
}
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| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure I understand, this calculates the direct (not transitive) public dependencies for each package. Yes?
Why the the d.kind() == Kind::Normal?

Copy link
Member Author

Choose a reason for hiding this comment

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

The other kinds are Development and Build, neither of which can ever be exposed as transitive dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Should we have an error if you try a Cargo.toml that has a Development or Build dep with public set to true? Then add asserts in the set_public to make sure we never let it slip in?

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()])
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for my ignorance. I don't know what this will do or why it is off now. Can you elaborate?

Copy link
Member Author

@Aaron1011 Aaron1011 Apr 18, 2019

Choose a reason for hiding this comment

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

This turns all exported_private_dependencies warnings into hard errors when publishing. According to the RFC, 'cargo publish might change this warning into an error in its lint step'

I thought it was best to leave it off for now, so that we don't imemdiately break publishing for people who enable this feature.

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
11 changes: 11 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,16 @@ 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())?;

if dep.kind() != Kind::Normal {
bail!("'public' specifier can only be used on regular dependencies, not {:?} dependencies", dep.kind());
}

dep.set_public(p);
}
Ok(dep)
}
}
Expand Down
17 changes: 17 additions & 0 deletions src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,20 @@ conflicting binaries from another package.
Additionally, a new flag `--no-track` is available to prevent `cargo install`
from writing tracking information in `$CARGO_HOME` about which packages are
installed.

### public-dependency
* Tracking Issue: [#44663](https://github.com/rust-lang/rust/issues/44663)

The 'public-dependency' features allows marking dependencies as 'public'
or 'private'. When this feature is enabled, additional information is passed to rustc to allow
the 'exported_private_dependencies' lint to function properly.

This requires the appropriate key to be set in `cargo-features`:

```toml
cargo-features = ["public-dependency"]

[dependencies]
my_dep = { version = "1.2.3", public = true }
private_dep = "2.0.0" # Will be 'private' by default
```
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