Skip to content

Commit

Permalink
Rework planning to use resolve tree and not packages
Browse files Browse the repository at this point in the history
There are a range of subtle and irritating bugs that stem from
attempting to work with renames from the package set provided by cargo
metadata.

Fortunately, recent versions of cargo metadata provide a limited version
of the resolve nodes, both as the original node ids and as a newer set
providing both rename and targeting information.

As such we can (hopefully) simplify crate -> dep node resolution, remove
a bunch of subtle aliasing bugs.

Fixes google#241, fixes google#269, fixes google#270, resolves google#144, resolves google#187
  • Loading branch information
GregBowyer committed Apr 23, 2021
1 parent ee2effe commit 8edc09d
Show file tree
Hide file tree
Showing 11 changed files with 419 additions and 551 deletions.
23 changes: 18 additions & 5 deletions impl/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::path::PathBuf;
use std::{collections::BTreeSet, path::PathBuf, hash::{Hash, Hasher}};

use crate::settings::CrateSettings;
use semver::Version;
Expand All @@ -29,12 +29,25 @@ pub struct BuildableDependency {
pub is_proc_macro: bool,
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize)]
#[derive(Debug, Clone, Eq, PartialOrd, Ord, Serialize)]
pub struct DependencyAlias {
pub target: String,
pub alias: String,
}

// We only want equality for the first member as it can have multiple names pointing at it.
impl Hash for DependencyAlias {
fn hash<H: Hasher>(&self, state: &mut H) {
self.target.hash(state);
}
}

impl PartialEq for DependencyAlias {
fn eq(&self, other: &Self) -> bool {
self.target == other.target
}
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize)]
pub struct BuildableTarget {
pub kind: String,
Expand Down Expand Up @@ -81,7 +94,7 @@ pub struct SourceDetails {
pub git_data: Option<GitRepo>,
}

#[derive(Debug, Clone, Serialize, PartialEq, Eq, PartialOrd, Ord)]
#[derive(Default, Debug, Clone, Serialize, PartialEq, Eq, PartialOrd, Ord)]
pub struct CrateDependencyContext {
pub dependencies: Vec<BuildableDependency>,
pub proc_macro_dependencies: Vec<BuildableDependency>,
Expand All @@ -92,7 +105,7 @@ pub struct CrateDependencyContext {
// build_data_dependencies can only be set when using cargo-raze as a library at the moment.
pub build_data_dependencies: Vec<BuildableDependency>,
pub dev_dependencies: Vec<BuildableDependency>,
pub aliased_dependencies: Vec<DependencyAlias>,
pub aliased_dependencies: BTreeSet<DependencyAlias>,
}

impl CrateDependencyContext {
Expand All @@ -110,7 +123,7 @@ impl CrateDependencyContext {
pub struct CrateTargetedDepContext {
pub target: String,
pub deps: CrateDependencyContext,
pub conditions: Vec<String>,
pub platform_targets: Vec<String>,
}

#[derive(Debug, Clone, Serialize)]
Expand Down
2 changes: 1 addition & 1 deletion impl/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ impl RazeMetadataFetcher {
let version = info.req();
let src_dir = self.fetch_crate_src(cargo_dir.as_ref(), &name, version)?;
checksums.insert(
package_ident(name, version),
package_ident(name, &version),
self.fetch_crate_checksum(name, version)?,
);
if let Some(dirname) = src_dir.file_name() {
Expand Down
32 changes: 19 additions & 13 deletions impl/src/planning.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use anyhow::Result;
use cargo_lock::Lockfile;

use crate::{
context::{CrateContext, WorkspaceContext},
context::{CrateContext, DependencyAlias, WorkspaceContext},
metadata::RazeMetadata,
settings::RazeSettings,
util::PlatformDetails,
Expand All @@ -32,8 +32,13 @@ use subplanners::WorkspaceSubplanner;
/// A ready-to-be-rendered build, containing renderable context for each crate.
#[derive(Debug)]
pub struct PlannedBuild {
/// The overall context for this workspace
pub workspace_context: WorkspaceContext,
/// The creates to build for
pub crate_contexts: Vec<CrateContext>,
/// And aliases that are defined at the workspace root
pub workspace_aliases: Vec<DependencyAlias>,
/// The version lock used if present
pub lockfile: Option<Lockfile>,
}

Expand Down Expand Up @@ -89,6 +94,7 @@ mod tests {
use super::*;
use cargo_metadata::PackageId;
use indoc::indoc;
use itertools::Itertools;
use semver::{Version, VersionReq};

fn dummy_resolve_dropping_metadata() -> RazeMetadata {
Expand Down Expand Up @@ -245,22 +251,22 @@ mod tests {
assert!(krate_position.is_some());

// Get crate context using computed position
let krate_context = crates_with_aliased_deps[krate_position.unwrap()].clone();
let crate_ctx = crates_with_aliased_deps[krate_position.unwrap()].clone();

// There are two default dependencies for cargo-raze-alias-test, log^0.4 and log^0.3
// However, log^0.3 is aliased to old_log_ while log^0.4 isn't aliased. Therefore, we
// should only see one aliased dependency (log^0.3 -> old_log_) which shows that the
// However, log^0.3 is aliased to old_log while log^0.4 isn't aliased. Therefore, we
// should only see one aliased dependency (log^0.3 -> old_log) which shows that the
// name and semver matching for aliased dependencies is working correctly
assert!(krate_context.default_deps.aliased_dependencies.len() == 1);
assert_eq!(
krate_context.default_deps.aliased_dependencies[0].target,
"@raze_test__log__0_3_9//:log"
);
assert_eq!(
krate_context.default_deps.aliased_dependencies[0].alias,
"old_log_"
);
let dep = crate_ctx
.default_deps
.aliased_dependencies
.iter()
.exactly_one()
.unwrap();
assert_eq!(dep.target, "@raze_test__log__0_3_9//:log");
assert_eq!(dep.alias, "old_log");
}

#[test]
fn test_plan_build_produces_proc_macro_dependencies() {
let mut settings = dummy_raze_settings();
Expand Down
Loading

0 comments on commit 8edc09d

Please sign in to comment.