From 995cd0cd6642b50f78e46fa9fdd1522e93045534 Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Fri, 26 Sep 2025 15:35:01 -0500 Subject: [PATCH] Don't preserve existing files when performing an upload Preserving `_link`, in particular, is problematic, as that preserves the link to the original package (akin to `keeplink`), which has caused numerous issues, such as: - Changes to the origin can result in build conflicts, which currently result in the runner waiting forever due to the bad state. - Broken links inhibit the ability to get the `xsrcmd5`, which is needed for monitoring the build logs. But keeping files around is of questionable utility regardless, and `osc-plugin-dput` doesn't even do that anymore. For simplicity, just remove this entire feature altogether. --- Cargo.lock | 4 +- src/handler.rs | 6 +-- src/monitor.rs | 4 +- src/prune.rs | 29 ++++++++++--- src/upload.rs | 116 ++++++++++++------------------------------------- 5 files changed, 57 insertions(+), 102 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8528cb5..6971eb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1547,7 +1547,7 @@ checksum = "a4895175b425cb1f87721b59f0f286c2092bd4af812243672510e1ac53e2e0ad" [[package]] name = "open-build-service-api" version = "0.1.0" -source = "git+https://github.com/collabora/open-build-service-rs#a069b8e215c062e8cd9ec33e10cdf1fdb398f7fa" +source = "git+https://github.com/collabora/open-build-service-rs#bd9f78ec0a26fcd9c4b23dfc8b8b1db299713fd1" dependencies = [ "base16ct", "bytes", @@ -1565,7 +1565,7 @@ dependencies = [ [[package]] name = "open-build-service-mock" version = "0.1.0" -source = "git+https://github.com/collabora/open-build-service-rs#a069b8e215c062e8cd9ec33e10cdf1fdb398f7fa" +source = "git+https://github.com/collabora/open-build-service-rs#bd9f78ec0a26fcd9c4b23dfc8b8b1db299713fd1" dependencies = [ "base16ct", "base64ct", diff --git a/src/handler.rs b/src/handler.rs index 91205cb..1e7b526 100644 --- a/src/handler.rs +++ b/src/handler.rs @@ -1343,11 +1343,7 @@ mod tests { .await ); - if build_info.is_branched { - dir.linkinfo.into_iter().next().unwrap().xsrcmd5 - } else { - dir.srcmd5 - } + dir.srcmd5 } ); diff --git a/src/monitor.rs b/src/monitor.rs index 0180c28..96dc9b0 100644 --- a/src/monitor.rs +++ b/src/monitor.rs @@ -345,7 +345,9 @@ mod tests { TEST_PACKAGE_2.to_owned(), MockBranchOptions { srcmd5: branch_srcmd5.clone(), - xsrcmd5: branch_xsrcmd5.clone(), + link_resolution: MockLinkResolution::Available { + xsrcmd5: branch_xsrcmd5.clone(), + }, ..Default::default() }, ); diff --git a/src/prune.rs b/src/prune.rs index 7d6ed3e..b1d1ca4 100644 --- a/src/prune.rs +++ b/src/prune.rs @@ -5,6 +5,25 @@ use tracing::info; use crate::retry_request; +async fn is_originally_branched( + client: &obs::Client, + project: &str, + package: &str, +) -> Result { + // Replacing the files in a package without setting keeplink will clear the + // linkinfo, so in order to determine if this was once branched, fetch the + // very first revision and check for linkinfo there. + retry_request!( + client + .project(project.to_owned()) + .package(package.to_owned()) + .list(Some("1")) + .await + .wrap_err("Failed to list package @ revision 1") + ) + .map(|first_rev| !first_rev.linkinfo.is_empty()) +} + #[tracing::instrument(skip(client))] pub async fn prune_branch( client: &obs::Client, @@ -14,6 +33,11 @@ pub async fn prune_branch( ) -> Result<()> { // Do a sanity check to make sure this project & package are actually // linked (i.e. we're not going to be nuking the main repository). + ensure!( + is_originally_branched(client, project, package).await?, + "Rejecting attempt to prune a non-branched package" + ); + let dir = retry_request!( client .project(project.to_owned()) @@ -23,11 +47,6 @@ pub async fn prune_branch( .wrap_err("Failed to list package") )?; - ensure!( - !dir.linkinfo.is_empty(), - "Rejecting attempt to prune a non-branched package" - ); - if let Some(expected_rev) = expected_rev { if dir.rev.as_deref() != Some(expected_rev) { info!( diff --git a/src/upload.rs b/src/upload.rs index 3551a58..03d5d44 100644 --- a/src/upload.rs +++ b/src/upload.rs @@ -1,10 +1,8 @@ -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; -use bytes::Bytes; use camino::{Utf8Path, Utf8PathBuf}; use color_eyre::eyre::{Context, Result, ensure, eyre}; use derivative::*; -use futures_util::{Stream, TryStreamExt}; use gitlab_runner::outputln; use md5::{Digest, Md5}; use open_build_service_api as obs; @@ -20,19 +18,6 @@ type Md5String = String; const META_NAME: &str = "_meta"; -async fn collect_byte_stream( - stream: impl Stream>, -) -> Result> { - let mut data = vec![]; - stream - .try_for_each(|chunk| { - data.extend_from_slice(&chunk); - futures_util::future::ready(Ok(())) - }) - .await?; - Ok(data) -} - pub fn compute_md5(data: &[u8]) -> String { base16ct::lower::encode_string(&Md5::digest(data)) } @@ -168,42 +153,6 @@ impl ObsDscUploader { } } - #[instrument(skip(self))] - async fn find_files_to_remove( - &self, - files: &HashMap, - ) -> Result> { - let mut to_remove = HashSet::new(); - - for file in files.keys() { - if file.ends_with(".dsc") { - to_remove.insert(file.clone()); - - let contents = retry_request!( - collect_byte_stream( - self.client - .project(self.project.clone()) - .package(self.package.clone()) - .source_file(file) - .await?, - ) - .instrument(info_span!("find_files_to_remove:download", %file)) - .await - )?; - - let _span = info_span!("find_files_to_remove:parse", %file); - let dsc: Dsc = - rfc822_like::from_str(discard_pgp(std::str::from_utf8(&contents[..])?))?; - - to_remove.extend(dsc.files.into_iter().map(|f| f.filename)); - } else if file.ends_with(".changes") { - to_remove.insert(file.clone()); - } - } - - Ok(to_remove) - } - #[instrument(skip(self))] async fn get_latest_meta_md5(&self) -> Result { let dir = retry_request!( @@ -321,11 +270,7 @@ impl ObsDscUploader { let present_files: HashMap<_, _> = dir.entries.into_iter().map(|e| (e.name, e.md5)).collect(); - let mut files_to_commit = present_files.clone(); - - for to_remove in self.find_files_to_remove(&files_to_commit).await? { - files_to_commit.remove(&to_remove); - } + let mut files_to_commit = HashMap::new(); for file in &self.dsc.files { files_to_commit.insert(file.filename.clone(), file.hash.clone()); @@ -366,15 +311,14 @@ impl ObsDscUploader { .await })?; - let build_srcmd5 = if let Some(link) = current_dir.linkinfo.into_iter().next() { - link.xsrcmd5 - } else { - current_dir.srcmd5 - }; + ensure!( + current_dir.linkinfo.is_empty(), + "linkinfo unexpectedly present after upload" + ); Ok(UploadResult { rev, - build_srcmd5, + build_srcmd5: current_dir.srcmd5, unchanged, }) } @@ -758,17 +702,15 @@ d29ybGQgaGVsbG8K let mut dir = assert_ok!(package_1.list(None).await); assert_eq!(dir.rev.as_deref(), Some("3")); - assert_eq!(dir.entries.len(), 5); + assert_eq!(dir.entries.len(), 4); dir.entries.sort_by(|a, b| a.name.cmp(&b.name)); assert_eq!(dir.entries[0].name, "_meta"); - assert_eq!(dir.entries[1].name, already_present_file); - assert_eq!(dir.entries[1].md5, already_present_md5); - assert_eq!(dir.entries[2].name, dsc2_file.file_name().unwrap()); - assert_eq!(dir.entries[2].md5, dsc2_md5); - assert_eq!(dir.entries[3].name, test1_file); - assert_eq!(dir.entries[3].md5, test1_md5_a); - assert_eq!(dir.entries[4].name, test2_file); - assert_eq!(dir.entries[4].md5, test2_md5); + assert_eq!(dir.entries[1].name, dsc2_file.file_name().unwrap()); + assert_eq!(dir.entries[1].md5, dsc2_md5); + assert_eq!(dir.entries[2].name, test1_file); + assert_eq!(dir.entries[2].md5, test1_md5_a); + assert_eq!(dir.entries[3].name, test2_file); + assert_eq!(dir.entries[3].md5, test2_md5); assert_eq!(dir.srcmd5, result.build_srcmd5); @@ -794,17 +736,15 @@ d29ybGQgaGVsbG8K let mut dir = assert_ok!(package_1.list(None).await); assert_eq!(dir.rev.as_deref(), Some("4")); - assert_eq!(dir.entries.len(), 5); + assert_eq!(dir.entries.len(), 4); dir.entries.sort_by(|a, b| a.name.cmp(&b.name)); assert_eq!(dir.entries[0].name, "_meta"); - assert_eq!(dir.entries[1].name, already_present_file); - assert_eq!(dir.entries[1].md5, already_present_md5); - assert_eq!(dir.entries[2].name, dsc3_file.file_name().unwrap()); - assert_eq!(dir.entries[2].md5, dsc3_md5); - assert_eq!(dir.entries[3].name, test1_file); - assert_eq!(dir.entries[3].md5, test1_md5_b); - assert_eq!(dir.entries[4].name, test2_file); - assert_eq!(dir.entries[4].md5, test2_md5); + assert_eq!(dir.entries[1].name, dsc3_file.file_name().unwrap()); + assert_eq!(dir.entries[1].md5, dsc3_md5); + assert_eq!(dir.entries[2].name, test1_file); + assert_eq!(dir.entries[2].md5, test1_md5_b); + assert_eq!(dir.entries[3].name, test2_file); + assert_eq!(dir.entries[3].md5, test2_md5); assert_eq!(dir.srcmd5, result.build_srcmd5); @@ -826,15 +766,13 @@ d29ybGQgaGVsbG8K let mut dir = assert_ok!(package_1.list(None).await); assert_eq!(dir.rev.as_deref(), Some("5")); - assert_eq!(dir.entries.len(), 4); + assert_eq!(dir.entries.len(), 3); dir.entries.sort_by(|a, b| a.name.cmp(&b.name)); assert_eq!(dir.entries[0].name, "_meta"); - assert_eq!(dir.entries[1].name, already_present_file); - assert_eq!(dir.entries[1].md5, already_present_md5); - assert_eq!(dir.entries[2].name, dsc4_file.file_name().unwrap()); - assert_eq!(dir.entries[2].md5, dsc4_md5); - assert_eq!(dir.entries[3].name, test1_file); - assert_eq!(dir.entries[3].md5, test1_md5_b); + assert_eq!(dir.entries[1].name, dsc4_file.file_name().unwrap()); + assert_eq!(dir.entries[1].md5, dsc4_md5); + assert_eq!(dir.entries[2].name, test1_file); + assert_eq!(dir.entries[2].md5, test1_md5_b); // Re-upload with no changes and ensure the old commit is returned. @@ -873,6 +811,6 @@ d29ybGQgaGVsbG8K // XXX: the mock apis don't set the correct rev values on branch yet assert_matches!(dir.rev, Some(_)); - assert_eq!(dir.linkinfo[0].xsrcmd5, result.build_srcmd5); + assert!(dir.linkinfo.is_empty()); } }