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

fix: readme comparison #1496

Merged
merged 15 commits into from
Jun 6, 2024
Merged
7 changes: 7 additions & 0 deletions crates/release_plz/tests/all/helpers/test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ impl TestContext {
context
}

pub async fn merge_release_pr(&self) {
let opened_prs = self.opened_release_prs().await;
assert_eq!(opened_prs.len(), 1);
self.gitea.merge_pr_retrying(opened_prs[0].number).await;
self.repo.git(&["pull"]).unwrap();
}

fn generate_cargo_lock(&self) {
assert_cmd::Command::new("cargo")
.current_dir(self.repo.directory())
Expand Down
1 change: 1 addition & 0 deletions crates/release_plz/tests/all/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@ mod changelog;
mod completion_test;
mod helpers;
mod release;
mod release_pr;
11 changes: 2 additions & 9 deletions crates/release_plz/tests/all/release.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,7 @@ async fn release_plz_does_not_release_a_new_project_if_release_always_is_false()

// TODO: Gitea doesn't detect associated PRs. I don't know why.
// context.run_release_pr().success();
// let opened_prs = context.opened_release_prs().await;
// assert_eq!(opened_prs.len(), 1);
// context.gitea.merge_pr_retrying(opened_prs[0].number).await;
// context.repo.git(&["pull"]).unwrap();
// context.merge_release_pr().unwrap();

// // Running `release` releases the project
// // because the last commit belongs to a release PR.
Expand Down Expand Up @@ -157,11 +154,7 @@ async fn release_plz_releases_after_release_pr_merged() {
context.write_release_plz_toml(config);

context.run_release_pr().success();

let opened_prs = context.opened_release_prs().await;
assert_eq!(opened_prs.len(), 1);
context.gitea.merge_pr_retrying(opened_prs[0].number).await;
context.repo.git(&["pull"]).unwrap();
context.merge_release_pr().await;

let crate_name = &context.gitea.repo;

Expand Down
52 changes: 52 additions & 0 deletions crates/release_plz/tests/all/release_pr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
use cargo_utils::LocalManifest;

use crate::helpers::test_context::TestContext;

#[tokio::test]
#[ignore = "This test fails in CI, but works locally on MacOS. TODO: fix this."]
#[cfg_attr(not(feature = "docker-tests"), ignore)]
async fn release_plz_detects_edited_readme_cargo_toml_field() {
let context = TestContext::new().await;

context.run_release_pr().success();
context.merge_release_pr().await;

let expected_tag = "v0.1.0";

context.run_release().success();

let gitea_release = context.gitea.get_gitea_release(expected_tag).await;
assert_eq!(gitea_release.name, expected_tag);

move_readme(&context);

context.run_release_pr().success();
context.merge_release_pr().await;

let expected_tag = "v0.1.1";

context.run_release().success();

let gitea_release = context.gitea.get_gitea_release(expected_tag).await;
assert_eq!(gitea_release.name, expected_tag);
expect_test::expect![[r#"
### Other
- move readme"#]]
.assert_eq(&gitea_release.body);
}

fn move_readme(context: &TestContext) {
let readme = "README.md";
let new_readme = format!("NEW_{readme}");
let old_readme_path = context.repo_dir().join(readme);
let new_readme_path = context.repo_dir().join(&new_readme);
fs_err::rename(old_readme_path, new_readme_path).unwrap();

let cargo_toml_path = context.repo_dir().join("Cargo.toml");
let mut cargo_toml = LocalManifest::try_new(&cargo_toml_path).unwrap();
cargo_toml.data["package"]["readme"] = toml_edit::value(new_readme);
cargo_toml.write().unwrap();

context.repo.add_all_and_commit("move readme").unwrap();
context.repo.git(&["push"]).unwrap();
}
5 changes: 4 additions & 1 deletion crates/release_plz_core/src/next_ver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1044,6 +1044,9 @@ impl Updater<'_> {
current_commit_message.clone(),
));
}
// Go back to the previous commit.
// Keep in mind that the info contained in `package` might be outdated,
// because commits could contain changes to Cargo.toml.
if let Err(_err) = repository.checkout_previous_commit_at_paths(&paths_to_check) {
debug!("there are no other commits");
break;
Expand All @@ -1059,7 +1062,7 @@ impl Updater<'_> {
package_path: &Utf8Path,
registry_package_path: &Utf8Path,
) -> anyhow::Result<bool> {
if is_readme_updated(package_path, package, registry_package_path)? {
if is_readme_updated(&package.name, package_path, registry_package_path)? {
debug!("{}: README updated", package.name);
return Ok(false);
}
Expand Down
41 changes: 34 additions & 7 deletions crates/release_plz_core/src/package_compare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use cargo_metadata::{
camino::{Utf8Path, Utf8PathBuf},
Package,
};
use cargo_utils::get_manifest_metadata;
use tracing::debug;

use crate::{cargo::run_cargo, CARGO_TOML};
Expand Down Expand Up @@ -132,23 +133,35 @@ fn are_cargo_toml_equal(local_package: &Utf8Path, registry_package: &Utf8Path) -
/// - the local package doesn't have a `readme` field in the `Cargo.toml`.
/// - the package doesn't have a README at all.
pub fn is_readme_updated(
package_name: &str,
local_package_path: &Utf8Path,
package: &Package,
registry_package_path: &Utf8Path,
) -> anyhow::Result<bool> {
let local_package_readme_path = local_readme_override(package, local_package_path);
// Read again manifest metadata because the Cargo.toml might change on every commit.
let package = match read_package_metadata(package_name, local_package_path) {
Ok(package) => package,
Err(e) => {
tracing::warn!(
"cannot read package metadata of {package_name} in {local_package_path}: {e:?}"
);
return Ok(false);
}
};

let local_package_readme_path = local_readme_override(&package, local_package_path);
let are_readmes_equal = match local_package_readme_path {
Some(local_package_readme_path) => {
let registry_package_readme_path = registry_package_path.join("README.md");
if !registry_package_readme_path.exists() {
return Ok(true);
}
if let Err(e) =
are_files_equal(&local_package_readme_path, &registry_package_readme_path)
{
tracing::warn!("cannot compare README files: {e}");
match are_files_equal(&local_package_readme_path, &registry_package_readme_path) {
Ok(are_readmes_equal) => are_readmes_equal,
Err(e) => {
tracing::warn!("cannot compare README files: {e}");
true
}
}
true
}
None => true,
};
Expand Down Expand Up @@ -180,3 +193,17 @@ fn file_hash(file: &Utf8Path) -> io::Result<u64> {
let hash = hasher.finish();
Ok(hash)
}

fn read_package_metadata(
package_name: &str,
local_package_path: &Utf8Path,
) -> anyhow::Result<Package> {
let package = get_manifest_metadata(&local_package_path.join(CARGO_TOML))
.context("cannot read Cargo.toml")?
.workspace_packages()
.into_iter()
.find(|&p| p.name == package_name)
.cloned()
.context("cannot find package in Cargo.toml")?;
Ok(package)
}
11 changes: 10 additions & 1 deletion crates/release_plz_core/src/registry_packages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,19 @@ fn initialize_registry_package(packages: Vec<Package>) -> anyhow::Result<Vec<Reg
None
};
let git_repo = package_path.join(".git");
let commit_init = || git_in_dir(package_path, &["commit", "-m", "init"]);
if !git_repo.exists() {
git_in_dir(package_path, &["init"])?;
git_in_dir(package_path, &["add", "."])?;
git_in_dir(package_path, &["commit", "-m", "init"]).unwrap();
if let Err(e) = commit_init() {
if e.to_string().trim().starts_with("Author identity unknown") {
// we can use any email and name here, as this repository is only used
// to compare packages
git_in_dir(package_path, &["config", "user.email", "test@registry"])?;
git_in_dir(package_path, &["config", "user.name", "test"])?;
commit_init()?;
}
}
}
registry_packages.push(RegistryPackage { package: p, sha1 });
}
Expand Down