Skip to content

Commit

Permalink
Fix bindings version warning and upgrade command. (#187)
Browse files Browse the repository at this point in the history 
* Fix bindings version warning and upgrade command.

This commit fixes the warning emitted when a mismatched bindings version crate
is detected such that it no longer warns when the bindings crate version is
compatible, but not the same version (i.e. `0.4.1` when cargo-component is
`0.4.0`).

Likewise, the `upgrade` command no longer attempts to downgrade the version of
the bindings crate when it would otherwise be compatible.

The upgrade tests no longer attempt the install, which require a long time to
just verify that a `cargo install cargo-component` would succeed, which isn't
terribly useful.

* Code review feedback.

* Remove outdated comment in upgrade command.
* Properly handle when the bindings crate is newer than cargo-component:
  * Emit a warning instructing the upgrade of cargo-component.
  * Skip updating `Cargo.toml` for `cargo component upgrade`.
  • Loading branch information
@peterhuene
peterhuene authored Nov 11, 2023
1 parent 047555b commit 8cfa63f
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 59 deletions.
64 changes: 43 additions & 21 deletions src/commands/upgrade.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use crate::{load_metadata, Config, BINDINGS_CRATE_NAME};
use anyhow::{Context, Result};
use cargo_component_core::command::CommonOptions;
use cargo_component_core::{command::CommonOptions, terminal::Colors};
use cargo_metadata::Metadata;
use clap::Args;
use semver::Version;
use std::{fs, path::PathBuf};
use toml_edit::{value, Document};

Expand Down Expand Up @@ -48,10 +49,6 @@ impl UpgradeCommand {
//
// (We can't tell whether or not cargo-install actually installed anything
// without scraping its output; it considers "already installed" as success.)
//
// Skip this in tests, but still delegate to a new instance of `cargo-component`
// so that we can exercise as much of the flow as practicable.
#[cfg(not(test))]
upgrade_self()?;
run_cargo_component_and_exit();
}
Expand All @@ -65,7 +62,6 @@ impl UpgradeCommand {
}
}

#[cfg(not(test))]
fn upgrade_self() -> Result<()> {
log::debug!("running self-upgrade using cargo-install");

Expand Down Expand Up @@ -114,7 +110,7 @@ fn run_cargo_component_and_exit() -> ! {
}

async fn upgrade_bindings(config: &Config, metadata: &Metadata, dry_run: bool) -> Result<()> {
let self_version = semver::VersionReq::parse(env!("CARGO_PKG_VERSION"))
let this_version = Version::parse(env!("CARGO_PKG_VERSION"))
.context("Failed to parse current cargo-component version")?;

for package in metadata.workspace_packages() {
Expand All @@ -124,22 +120,48 @@ async fn upgrade_bindings(config: &Config, metadata: &Metadata, dry_run: bool) -
.find(|dep| dep.name == "cargo-component-bindings")
else {
log::debug!(
"Workspace package {} doesn't depend on cargo-component-bindings",
package.name
"workspace package `{name}` doesn't depend on cargo-component-bindings",
name = package.name
);
continue;
};

if bindings_dep.req == self_version {
config.terminal().status(
"Skipping",
format!(
"package `{}` as it already uses the current bindings crate version",
package.name
),
)?;
continue;
}
let s = bindings_dep.req.to_string();
let version = match s.strip_prefix('^').unwrap_or(&s).parse::<Version>() {
Ok(v) => {
if this_version.major == v.major
&& (this_version.major > 0 || this_version.minor == v.minor)
{
config.terminal().status(
"Skipping",
format!(
"package `{name}` as it already uses a compatible bindings crate version",
name = package.name
),
)?;
continue;
}

if this_version.major > v.major
|| (this_version.major == v.major && this_version.minor > v.minor)
{
// cargo-component is newer, fall through to upgrade the project.
v
} else {
// cargo-component is older, warn about it
config.terminal().status_with_color(
"Skipping",
format!(
"package `{name}` is using bindings crate version {v} which is newer than cargo-component ({this_version})",
name = package.name
),
Colors::Yellow,
)?;
continue;
}
}
_ => continue,
};

let manifest_path = package.manifest_path.as_std_path();
let manifest = fs::read_to_string(manifest_path).with_context(|| {
Expand Down Expand Up @@ -167,7 +189,7 @@ async fn upgrade_bindings(config: &Config, metadata: &Metadata, dry_run: bool) -
format!(
"{path} from {from} to {to}",
path = manifest_path.display(),
from = bindings_dep.req,
from = version,
to = env!("CARGO_PKG_VERSION")
),
)?;
Expand All @@ -186,7 +208,7 @@ async fn upgrade_bindings(config: &Config, metadata: &Metadata, dry_run: bool) -
format!(
"{path} from {from} to {to}",
path = manifest_path.display(),
from = bindings_dep.req,
from = version,
to = env!("CARGO_PKG_VERSION")
),
)?;
Expand Down
51 changes: 31 additions & 20 deletions src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,27 +220,38 @@ pub fn load_metadata(

let metadata = command.exec().context("failed to load cargo metadata")?;

for package in &metadata.packages {
for dep in &package.dependencies {
if dep.rename.as_deref().unwrap_or(dep.name.as_str()) != BINDINGS_CRATE_NAME {
continue;
}

if ignore_version_mismatch
|| dep
.req
.matches(&Version::parse(env!("CARGO_PKG_VERSION")).unwrap())
{
continue;
if !ignore_version_mismatch {
let this_version = Version::parse(env!("CARGO_PKG_VERSION")).unwrap();
for package in &metadata.packages {
match package.dependencies.iter().find(|dep| {
dep.rename.as_deref().unwrap_or(dep.name.as_str()) == BINDINGS_CRATE_NAME
}) {
Some(bindings_crate) => {
let s = bindings_crate.req.to_string();
match s.strip_prefix('^').unwrap_or(&s).parse::<Version>() {
Ok(v) => {
if this_version.major == v.major
&& (this_version.major > 0 || this_version.minor == v.minor)
{
// Version should be compatible
continue;
}

if this_version.major > v.major
|| (this_version.major == v.major && this_version.minor > v.minor)
{
// cargo-component is newer, so warn about upgrading `Cargo.toml`
terminal.warn(format!("manifest `{path}` uses an older version of `{BINDINGS_CRATE_NAME}` ({v}) than cargo-component ({this_version}); use `cargo component upgrade --no-install` to update the manifest", path = package.manifest_path))?;
} else {
// cargo-component itself is out of date; warn to upgrade
terminal.warn(format!("manifest `{path}` uses a newer version of `{BINDINGS_CRATE_NAME}` ({v}) than cargo-component ({this_version}); use `cargo component upgrade` to upgrade to the latest version", path = package.manifest_path))?;
};
}
_ => continue,
}
}
None => continue,
}

terminal.warn(
format!(
"mismatched version of `{BINDINGS_CRATE_NAME}` detected in manifest `{path}`: please update the version in the manifest to {version}",
path = package.manifest_path,
version = env!("CARGO_PKG_VERSION")
)
)?;
}
}

Expand Down
27 changes: 23 additions & 4 deletions tests/build.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,7 +777,7 @@ fn it_errors_if_adapter_is_not_wasm() -> Result<()> {
}

#[test]
fn it_warns_on_mismatched_bindings_crate_version() -> Result<()> {
fn it_warns_on_outdated_bindings_crate_version() -> Result<()> {
let project = Project::new("foo")?;
project.update_manifest(|mut doc| {
doc["dependencies"][BINDINGS_CRATE_NAME] = value("0.2.0");
Expand All @@ -787,9 +787,28 @@ fn it_warns_on_mismatched_bindings_crate_version() -> Result<()> {
project
.cargo_component("build")
.assert()
.stderr(contains(
"warning: mismatched version of `cargo-component-bindings` detected in manifest",
))
.stderr(contains(format!(
"uses an older version of `{BINDINGS_CRATE_NAME}` (0.2.0) than cargo-component"
)))
.failure();

Ok(())
}

#[test]
fn it_warns_on_outdated_cargo_component_version() -> Result<()> {
let project = Project::new("foo")?;
project.update_manifest(|mut doc| {
doc["dependencies"][BINDINGS_CRATE_NAME] = value("1000.0.0");
Ok(doc)
})?;

project
.cargo_component("build")
.assert()
.stderr(contains(format!(
"uses a newer version of `{BINDINGS_CRATE_NAME}` (1000.0.0) than cargo-component"
)))
.failure();

Ok(())
Expand Down
57 changes: 43 additions & 14 deletions tests/upgrade.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ fn upgrade_single_crate_already_current_is_no_op() -> Result<()> {
let project = Project::with_root(&root, "component", "")?;

project
.cargo_component("upgrade")
.cargo_component("upgrade --no-install")
.assert()
.success()
.stderr(contains(
"Skipping package `component` as it already uses the current bindings crate version",
"Skipping package `component` as it already uses a compatible bindings crate version",
));

Ok(())
Expand All @@ -41,7 +41,7 @@ fn upgrade_single_crate_upgrades_bindings_dep() -> Result<()> {
let project = Project::with_root(&root, "component", "")?;
project.update_manifest(|mut doc| {
// Set arbitrary old version of bindings crate.
doc["dependencies"][BINDINGS_CRATE_NAME] = value("0.1");
doc["dependencies"][BINDINGS_CRATE_NAME] = value("0.1.0");
Ok(doc)
})?;

Expand All @@ -52,28 +52,28 @@ fn upgrade_single_crate_upgrades_bindings_dep() -> Result<()> {
let manifest = project.read_manifest()?;
assert_eq!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some("0.1")
Some("0.1.0")
);
assert_ne!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some(env!("CARGO_PKG_VERSION"))
);

project
.cargo_component("upgrade")
.cargo_component("upgrade --no-install")
.assert()
.success()
.stderr(contains("Updated "))
.stderr(contains(format!(
"from ^0.1 to {}",
"from 0.1.0 to {}",
env!("CARGO_PKG_VERSION")
)));

// It should have actually written the upgrade.
let manifest = project.read_manifest()?;
assert_ne!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some("0.1")
Some("0.1.0")
);
assert_eq!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Expand All @@ -82,23 +82,52 @@ fn upgrade_single_crate_upgrades_bindings_dep() -> Result<()> {

// A repeated upgrade should recognize that there is no change required.
project
.cargo_component("upgrade")
.cargo_component("upgrade --no-install")
.assert()
.success()
.stderr(contains(
"Skipping package `component` as it already uses the current bindings crate version",
"Skipping package `component` as it already uses a compatible bindings crate version",
));

Ok(())
}

#[test]
fn skip_packages_newer_than_cargo_component() -> Result<()> {
let root = create_root()?;
let project = Project::with_root(&root, "component", "")?;
project.update_manifest(|mut doc| {
// Set arbitrary future version of cargo-component
doc["dependencies"][BINDINGS_CRATE_NAME] = value("1000.0.0");
Ok(doc)
})?;

project
.cargo_component("upgrade --no-install")
.assert()
.success()
.stderr(contains("Skipping "))
.stderr(contains(
"using bindings crate version 1000.0.0 which is newer than cargo-component",
));

// It should have not updated the manifest
let manifest = project.read_manifest()?;
assert_eq!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some("1000.0.0")
);

Ok(())
}

#[test]
fn upgrade_dry_run_does_not_alter_manifest() -> Result<()> {
let root = create_root()?;
let project = Project::with_root(&root, "component", "")?;
project.update_manifest(|mut doc| {
// Set arbitrary old version of bindings crate.
doc["dependencies"][BINDINGS_CRATE_NAME] = value("0.1");
doc["dependencies"][BINDINGS_CRATE_NAME] = value("0.1.0");
Ok(doc)
})?;

Expand All @@ -109,29 +138,29 @@ fn upgrade_dry_run_does_not_alter_manifest() -> Result<()> {
let manifest = project.read_manifest()?;
assert_eq!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some("0.1")
Some("0.1.0")
);
assert_ne!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some(env!("CARGO_PKG_VERSION"))
);

project
.cargo_component("upgrade --dry-run")
.cargo_component("upgrade --no-install --dry-run")
.assert()
.success()
.stderr(contains("Would update "))
.stderr(contains("Updated ").not())
.stderr(contains(format!(
"from ^0.1 to {}",
"from 0.1.0 to {}",
env!("CARGO_PKG_VERSION")
)));

// It should NOT have written the upgrade.
let manifest = project.read_manifest()?;
assert_eq!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Some("0.1")
Some("0.1.0")
);
assert_ne!(
manifest["dependencies"][BINDINGS_CRATE_NAME].as_str(),
Expand Down

0 comments on commit 8cfa63f

Please sign in to comment.