Skip to content

Commit

Permalink
truncate comments at 80-90c; cleanup
Browse files Browse the repository at this point in the history
- remove unused method
- remove '-Z unstable-options'
- improve error message
- improve the way MSVC special cases are targetted in tests
- improve how executables are found on non MSVC
  • Loading branch information
Byron committed Feb 18, 2022
1 parent e4d284f commit 3de5799
Show file tree
Hide file tree
Showing 12 changed files with 153 additions and 134 deletions.
3 changes: 2 additions & 1 deletion src/cargo/core/compiler/artifact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use crate::CargoResult;
use std::collections::HashMap;
use std::ffi::OsString;

/// Return all environment variables for the given unit-dependencies if artifacts are present.
/// Return all environment variables for the given unit-dependencies
/// if artifacts are present.
pub fn get_env(
cx: &Context<'_, '_>,
dependencies: &[UnitDep],
Expand Down
5 changes: 3 additions & 2 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,9 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc<dyn Executor>) -> Car
let artifact = unit.artifact;

return Ok(Work::new(move |state| {
// Artifacts are in a different location than typical units, hence
// we must assure the crate- and target-dependent directory is present.
// Artifacts are in a different location than typical units,
// hence we must assure the crate- and target-dependent
// directory is present.
if artifact.is_true() {
paths::create_dir_all(&root)?;
}
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/core/compiler/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ pub struct UnitInner {
/// The `cfg` features to enable for this unit.
/// This must be sorted.
pub features: Vec<InternedString>,
// if `true`, the dependency is an artifact dependency, requiring special handling when calculating output directories,
// linkage and environment variables provided to builds.
// if `true`, the dependency is an artifact dependency, requiring special handling when
// calculating output directories, linkage and environment variables provided to builds.
pub artifact: IsArtifact,
/// Whether this is a standard library unit.
pub is_std: bool,
Expand Down
21 changes: 12 additions & 9 deletions src/cargo/core/compiler/unit_dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,8 @@ fn calc_artifact_deps(
{
has_artifact = true;
artifact_lib |= artifact.is_lib();
// Custom build scripts (build/compile) never get artifact dependencies, but the run-build-script step does (where it is handled).
// Custom build scripts (build/compile) never get artifact dependencies,
// but the run-build-script step does (where it is handled).
if !unit.target.is_custom_build() {
debug_assert!(
!unit.mode.is_run_custom_build(),
Expand Down Expand Up @@ -525,8 +526,9 @@ fn build_artifact_requirements_to_units(
state: &State<'_, '_>,
) -> CargoResult<Vec<UnitDep>> {
let mut ret = Vec::new();
// So, this really wants to be true for build dependencies, otherwise resolver = "2" will fail.
// It means that the host features will be separated from normal features, thus won't be unified with them.
// This really wants to be true for build dependencies, otherwise resolver = "2"
// will fail. // It means that the host features will be separated from normal
// features, thus won't be unified with them.
let host_features = true;
let unit_for = UnitFor::new_host(host_features, root_unit_compile_target);
for (dep_pkg_id, deps) in artifact_deps {
Expand All @@ -552,7 +554,8 @@ fn build_artifact_requirements_to_units(
Ok(ret)
}

/// `compile_kind` is the computed kind for the future artifact unit dependency. It must be computed by the caller.
/// `compile_kind` is the computed kind for the future artifact unit
/// dependency. It must be computed by the caller.
fn artifact_targets_to_unit_deps(
parent: &Unit,
parent_unit_for: UnitFor,
Expand All @@ -564,9 +567,10 @@ fn artifact_targets_to_unit_deps(
let ret = match_artifacts_kind_with_targets(parent, dep, artifact_pkg.targets())?
.into_iter()
.flat_map(|target| {
// We split target libraries into individual units, even though rustc is able to produce multiple
// kinds in an single invocation for the sole reason that each artifact kind has its own output directory,
// something we can't easily teach rustc for now.
// We split target libraries into individual units, even though rustc is able
// to produce multiple kinds in an single invocation for the sole reason that
// each artifact kind has its own output directory, something we can't easily
// teach rustc for now.
match target.kind() {
TargetKind::Lib(kinds) => Box::new(
kinds
Expand Down Expand Up @@ -627,9 +631,8 @@ fn match_artifacts_kind_with_targets<'a>(
};
if !found {
anyhow::bail!(
"Dependency `{} = \"{}\"` in crate `{}` requires a `{}` artifact to be present.",
"dependency `{}` in package `{}` requires a `{}` artifact to be present.",
artifact_dep.name_in_toml(),
artifact_dep.version_req(),
unit.pkg.name(),
artifact_kind
);
Expand Down
7 changes: 4 additions & 3 deletions src/cargo/core/compiler/unit_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,10 @@ pub struct UnitDep {
/// The name the parent uses to refer to this dependency.
pub extern_crate_name: InternedString,
/// If `Some`, the name of the dependency if renamed in toml.
/// It's particularly interesting to artifact dependencies which rely on it for naming their
/// environment variables. Note that the `extern_crate_name` cannot be used for this as it
/// also may be the build target itself, which isn't always the renamed dependency name.
/// It's particularly interesting to artifact dependencies which rely on it
/// for naming their environment variables. Note that the `extern_crate_name`
/// cannot be used for this as it also may be the build target itself,
/// which isn't always the renamed dependency name.
pub dep_name: Option<InternedString>,
/// Whether or not this is a public dependency.
pub public: bool,
Expand Down
7 changes: 4 additions & 3 deletions src/cargo/core/dependency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,8 +422,8 @@ impl Dependency {
self.inner.artifact.as_ref()
}

/// Dependencies are potential rust libs if they are not artifacts or they are an artifact
/// which allows to be seen as library.
/// Dependencies are potential rust libs if they are not artifacts or they are an
/// artifact which allows to be seen as library.
/// Previously, every dependency was potentially seen as library.
pub(crate) fn maybe_lib(&self) -> bool {
self.artifact().map(|a| a.is_lib).unwrap_or(true)
Expand All @@ -432,7 +432,8 @@ impl Dependency {

/// The presence of an artifact turns an ordinary dependency into an Artifact dependency.
/// As such, it will build one or more different artifacts of possibly various kinds
/// for making them available at build time for rustc invocations or runtime for build scripts.
/// for making them available at build time for rustc invocations or runtime
/// for build scripts.
///
/// This information represents a requirement in the package this dependency refers to.
#[derive(PartialEq, Eq, Hash, Clone, Debug)]
Expand Down
4 changes: 0 additions & 4 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,10 +469,6 @@ impl<'cfg> PackageSet<'cfg> {
})
}

pub fn get_one_without_download(&self, id: PackageId) -> Option<&Package> {
self.packages.get(&id).and_then(|slot| slot.borrow())
}

pub fn get_one(&self, id: PackageId) -> CargoResult<&Package> {
if let Some(pkg) = self.packages.get(&id).and_then(|slot| slot.borrow()) {
return Ok(pkg);
Expand Down
31 changes: 18 additions & 13 deletions src/cargo/core/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -940,18 +940,21 @@ pub struct UnitFor {
panic_setting: PanicSetting,

/// The compile kind of the root unit for which artifact dependencies are built.
/// This is required particularly for the `target = "target"` setting of artifact dependencies
/// which mean to inherit the `--target` specified on the command-line. However, that is a multi-value
/// argument and root units are already created to reflect one unit per --target. Thus we have to build
/// one artifact with the correct target for each of these trees.
/// Note that this will always be set as we don't initially know if there are artifacts that make use of it.
/// This is required particularly for the `target = "target"` setting of artifact
/// dependencies which mean to inherit the `--target` specified on the command-line.
/// However, that is a multi-value argument and root units are already created to
/// reflect one unit per --target. Thus we have to build one artifact with the
/// correct target for each of these trees.
/// Note that this will always be set as we don't initially know if there are
/// artifacts that make use of it.
root_compile_kind: CompileKind,

/// This is only set for artifact dependencies which have their `<target-triple>|target` set.
/// If so, this information is used as part of the key for resolving their features, allowing for target-dependent feature resolution
/// within the entire dependency tree.
/// Note that this target corresponds to the target used to build the units in that dependency tree, too, but this copy of it is
/// specifically used for feature lookup.
/// This is only set for artifact dependencies which have their
/// `<target-triple>|target` set.
/// If so, this information is used as part of the key for resolving their features,
/// allowing for target-dependent feature resolution within the entire dependency tree.
/// Note that this target corresponds to the target used to build the units in that
/// dependency tree, too, but this copy of it is specifically used for feature lookup.
artifact_target_for_features: Option<CompileTarget>,
}

Expand Down Expand Up @@ -1126,9 +1129,11 @@ impl UnitFor {
self.panic_setting
}

/// We might contain a parent artifact compile kind for features already, but will gladly accept the one of this dependency
/// as an override as it defines how the artifact is built.
/// If we are an artifact but don't specify a `target`, we assume the default compile kind that is suitable in this situation.
/// We might contain a parent artifact compile kind for features already, but will
/// gladly accept the one of this dependency as an override as it defines how
/// the artifact is built.
/// If we are an artifact but don't specify a `target`, we assume the default
/// compile kind that is suitable in this situation.
pub(crate) fn map_to_features_for(&self, dep_artifact: Option<&Artifact>) -> FeaturesFor {
FeaturesFor::from_for_host_or_artifact_target(
self.is_for_host_features(),
Expand Down
53 changes: 34 additions & 19 deletions src/cargo/core/resolver/features.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,12 @@ pub struct ResolvedFeatures {
/// Options for how the feature resolver works.
#[derive(Default)]
pub struct FeatureOpts {
/// Build deps and proc-macros will not share share features with other dep kinds, and so won't artifact targets.
/// In other terms, if true, features associated with certain kinds of dependencies will only be unified together.
/// If false, there is only one namespace for features, unifying all features across all dependencies, no matter
/// what kind.
/// Build deps and proc-macros will not share share features with other dep kinds,
/// and so won't artifact targets.
/// In other terms, if true, features associated with certain kinds of dependencies
/// will only be unified together.
/// If false, there is only one namespace for features, unifying all features across
/// all dependencies, no matter what kind.
decouple_host_deps: bool,
/// Dev dep features will not be activated unless needed.
decouple_dev_deps: bool,
Expand Down Expand Up @@ -99,7 +101,8 @@ pub enum ForceAllTargets {
/// Flag to indicate if features are requested for a build dependency or not.
#[derive(Copy, Clone, Debug, PartialEq, Eq, Ord, PartialOrd, Hash)]
pub enum FeaturesFor {
/// If `Some(target)` is present, we represent an artifact target. Otherwise any other normal or dev dependency.
/// If `Some(target)` is present, we represent an artifact target.
/// Otherwise any other normal or dev dependency.
NormalOrDevOrArtifactTarget(Option<CompileTarget>),
/// Build dependency or proc-macro.
HostDep,
Expand Down Expand Up @@ -789,21 +792,28 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
true
})
.flat_map(|dep| {
// Each `dep`endency can be built for multiple targets. For one, it may be a library target
// which is built as initially configured by `fk`. If it appears as build dependency,
// it must be built for the host.
// Each `dep`endency can be built for multiple targets. For one, it
// may be a library target which is built as initially configured
// by `fk`. If it appears as build dependency, it must be built
// for the host.
//
// It may also be an artifact dependency, which could be built either
// It may also be an artifact dependency,
// which could be built either
//
// - for a specified (aka 'forced') target, specified by `dep = { …, target = <triple>` }`
// - as an artifact for use in build dependencies that should build for whichever `--target`s are specified
// - for a specified (aka 'forced') target, specified by
// `dep = { …, target = <triple>` }`
// - as an artifact for use in build dependencies that should
// build for whichever `--target`s are specified
// - like a library would be built
//
// Generally, the logic for choosing a target for dependencies is unaltered and used to determine
// how to build non-artifacts, artifacts without target specification and no library, or an artifacts library.
// Generally, the logic for choosing a target for dependencies is
// unaltered and used to determine how to build non-artifacts,
// artifacts without target specification and no library,
// or an artifacts library.
//
// All this may result in a dependency being built multiple times for various targets which are either specified
// in the manifest or on the cargo command-line.
// All this may result in a dependency being built multiple times
// for various targets which are either specified in the manifest
// or on the cargo command-line.
let lib_fk = if fk == FeaturesFor::default() {
(self.track_for_host && (dep.is_build() || self.is_proc_macro(dep_id)))
.then(|| FeaturesFor::HostDep)
Expand Down Expand Up @@ -837,15 +847,20 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> {
});

let dep_fks = match artifact_target_keys {
// The artifact is also a library and does specify custom targets.
// The library's feature key needs to be used alongside the keys artifact targets.
// The artifact is also a library and does specify custom
// targets.
// The library's feature key needs to be used alongside
// the keys artifact targets.
Some((is_lib, Some(mut dep_fks))) if is_lib => {
dep_fks.push(lib_fk);
dep_fks
}
// The artifact is not a library, but does specify custom targets. Use only these targets feature keys.
// The artifact is not a library, but does specify
// custom targets.
// Use only these targets feature keys.
Some((_, Some(dep_fks))) => dep_fks,
// There is no artifact in the current dependency or there is no target specified on the artifact.
// There is no artifact in the current dependency
// or there is no target specified on the artifact.
// Use the standard feature key without any alteration.
Some((_, None)) | None => vec![lib_fk],
};
Expand Down
2 changes: 1 addition & 1 deletion src/doc/src/reference/unstable.md
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,7 @@ the command line) target.
Allow Cargo packages to depend on `bin`, `cdylib`, and `staticlib` crates,
and use the artifacts built by those crates at compile time.

Run `cargo` with `-Z unstable-options -Z bindeps` to enable this functionality.
Run `cargo` with `-Z bindeps` to enable this functionality.

**Example:** use _cdylib_ artifact in build script

Expand Down
Loading

0 comments on commit 3de5799

Please sign in to comment.