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 Feb 4, 2021
1 parent eee9428 commit d8cee86
Show file tree
Hide file tree
Showing 13 changed files with 382 additions and 527 deletions.
12 changes: 12 additions & 0 deletions impl/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions impl/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ tempfile = "3.2.0"
tera = "1.6.1"
toml = "0.5.8"
url = "2.2.0"
derivative = "2.1"

[dev-dependencies]
flate2 = "1.0.19"
Expand Down
13 changes: 8 additions & 5 deletions impl/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::path::PathBuf;
use std::{collections::BTreeSet, path::PathBuf};

use crate::settings::CrateSettings;
use derivative::Derivative;
use semver::Version;
use serde::Serialize;

Expand All @@ -29,9 +30,11 @@ pub struct BuildableDependency {
pub is_proc_macro: bool,
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize)]
#[derive(Debug, Clone, Eq, PartialOrd, Ord, Serialize, Derivative)]
#[derivative(Hash, PartialEq)]
pub struct DependencyAlias {
pub target: String,
#[derivative(Hash = "ignore", PartialEq = "ignore")]
pub alias: String,
}

Expand Down Expand Up @@ -81,7 +84,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 +95,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 +113,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 @@ -432,7 +432,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
28 changes: 17 additions & 11 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 @@ -92,6 +97,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 @@ -248,22 +254,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 krate_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
// 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 = krate_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 d8cee86

Please sign in to comment.