Skip to content

Commit 995cd0c

Browse files
committed
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.
1 parent de9c55c commit 995cd0c

File tree

5 files changed

+57
-102
lines changed

5 files changed

+57
-102
lines changed

Cargo.lock

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/handler.rs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1343,11 +1343,7 @@ mod tests {
13431343
.await
13441344
);
13451345

1346-
if build_info.is_branched {
1347-
dir.linkinfo.into_iter().next().unwrap().xsrcmd5
1348-
} else {
1349-
dir.srcmd5
1350-
}
1346+
dir.srcmd5
13511347
}
13521348
);
13531349

src/monitor.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,9 @@ mod tests {
345345
TEST_PACKAGE_2.to_owned(),
346346
MockBranchOptions {
347347
srcmd5: branch_srcmd5.clone(),
348-
xsrcmd5: branch_xsrcmd5.clone(),
348+
link_resolution: MockLinkResolution::Available {
349+
xsrcmd5: branch_xsrcmd5.clone(),
350+
},
349351
..Default::default()
350352
},
351353
);

src/prune.rs

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,25 @@ use tracing::info;
55

66
use crate::retry_request;
77

8+
async fn is_originally_branched(
9+
client: &obs::Client,
10+
project: &str,
11+
package: &str,
12+
) -> Result<bool> {
13+
// Replacing the files in a package without setting keeplink will clear the
14+
// linkinfo, so in order to determine if this was once branched, fetch the
15+
// very first revision and check for linkinfo there.
16+
retry_request!(
17+
client
18+
.project(project.to_owned())
19+
.package(package.to_owned())
20+
.list(Some("1"))
21+
.await
22+
.wrap_err("Failed to list package @ revision 1")
23+
)
24+
.map(|first_rev| !first_rev.linkinfo.is_empty())
25+
}
26+
827
#[tracing::instrument(skip(client))]
928
pub async fn prune_branch(
1029
client: &obs::Client,
@@ -14,6 +33,11 @@ pub async fn prune_branch(
1433
) -> Result<()> {
1534
// Do a sanity check to make sure this project & package are actually
1635
// linked (i.e. we're not going to be nuking the main repository).
36+
ensure!(
37+
is_originally_branched(client, project, package).await?,
38+
"Rejecting attempt to prune a non-branched package"
39+
);
40+
1741
let dir = retry_request!(
1842
client
1943
.project(project.to_owned())
@@ -23,11 +47,6 @@ pub async fn prune_branch(
2347
.wrap_err("Failed to list package")
2448
)?;
2549

26-
ensure!(
27-
!dir.linkinfo.is_empty(),
28-
"Rejecting attempt to prune a non-branched package"
29-
);
30-
3150
if let Some(expected_rev) = expected_rev {
3251
if dir.rev.as_deref() != Some(expected_rev) {
3352
info!(

src/upload.rs

Lines changed: 27 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,8 @@
1-
use std::collections::{HashMap, HashSet};
1+
use std::collections::HashMap;
22

3-
use bytes::Bytes;
43
use camino::{Utf8Path, Utf8PathBuf};
54
use color_eyre::eyre::{Context, Result, ensure, eyre};
65
use derivative::*;
7-
use futures_util::{Stream, TryStreamExt};
86
use gitlab_runner::outputln;
97
use md5::{Digest, Md5};
108
use open_build_service_api as obs;
@@ -20,19 +18,6 @@ type Md5String = String;
2018

2119
const META_NAME: &str = "_meta";
2220

23-
async fn collect_byte_stream<E: std::error::Error + Send + Sync + 'static>(
24-
stream: impl Stream<Item = Result<Bytes, E>>,
25-
) -> Result<Vec<u8>> {
26-
let mut data = vec![];
27-
stream
28-
.try_for_each(|chunk| {
29-
data.extend_from_slice(&chunk);
30-
futures_util::future::ready(Ok(()))
31-
})
32-
.await?;
33-
Ok(data)
34-
}
35-
3621
pub fn compute_md5(data: &[u8]) -> String {
3722
base16ct::lower::encode_string(&Md5::digest(data))
3823
}
@@ -168,42 +153,6 @@ impl ObsDscUploader {
168153
}
169154
}
170155

171-
#[instrument(skip(self))]
172-
async fn find_files_to_remove(
173-
&self,
174-
files: &HashMap<String, Md5String>,
175-
) -> Result<HashSet<Md5String>> {
176-
let mut to_remove = HashSet::new();
177-
178-
for file in files.keys() {
179-
if file.ends_with(".dsc") {
180-
to_remove.insert(file.clone());
181-
182-
let contents = retry_request!(
183-
collect_byte_stream(
184-
self.client
185-
.project(self.project.clone())
186-
.package(self.package.clone())
187-
.source_file(file)
188-
.await?,
189-
)
190-
.instrument(info_span!("find_files_to_remove:download", %file))
191-
.await
192-
)?;
193-
194-
let _span = info_span!("find_files_to_remove:parse", %file);
195-
let dsc: Dsc =
196-
rfc822_like::from_str(discard_pgp(std::str::from_utf8(&contents[..])?))?;
197-
198-
to_remove.extend(dsc.files.into_iter().map(|f| f.filename));
199-
} else if file.ends_with(".changes") {
200-
to_remove.insert(file.clone());
201-
}
202-
}
203-
204-
Ok(to_remove)
205-
}
206-
207156
#[instrument(skip(self))]
208157
async fn get_latest_meta_md5(&self) -> Result<Md5String> {
209158
let dir = retry_request!(
@@ -321,11 +270,7 @@ impl ObsDscUploader {
321270
let present_files: HashMap<_, _> =
322271
dir.entries.into_iter().map(|e| (e.name, e.md5)).collect();
323272

324-
let mut files_to_commit = present_files.clone();
325-
326-
for to_remove in self.find_files_to_remove(&files_to_commit).await? {
327-
files_to_commit.remove(&to_remove);
328-
}
273+
let mut files_to_commit = HashMap::new();
329274

330275
for file in &self.dsc.files {
331276
files_to_commit.insert(file.filename.clone(), file.hash.clone());
@@ -366,15 +311,14 @@ impl ObsDscUploader {
366311
.await
367312
})?;
368313

369-
let build_srcmd5 = if let Some(link) = current_dir.linkinfo.into_iter().next() {
370-
link.xsrcmd5
371-
} else {
372-
current_dir.srcmd5
373-
};
314+
ensure!(
315+
current_dir.linkinfo.is_empty(),
316+
"linkinfo unexpectedly present after upload"
317+
);
374318

375319
Ok(UploadResult {
376320
rev,
377-
build_srcmd5,
321+
build_srcmd5: current_dir.srcmd5,
378322
unchanged,
379323
})
380324
}
@@ -758,17 +702,15 @@ d29ybGQgaGVsbG8K
758702

759703
let mut dir = assert_ok!(package_1.list(None).await);
760704
assert_eq!(dir.rev.as_deref(), Some("3"));
761-
assert_eq!(dir.entries.len(), 5);
705+
assert_eq!(dir.entries.len(), 4);
762706
dir.entries.sort_by(|a, b| a.name.cmp(&b.name));
763707
assert_eq!(dir.entries[0].name, "_meta");
764-
assert_eq!(dir.entries[1].name, already_present_file);
765-
assert_eq!(dir.entries[1].md5, already_present_md5);
766-
assert_eq!(dir.entries[2].name, dsc2_file.file_name().unwrap());
767-
assert_eq!(dir.entries[2].md5, dsc2_md5);
768-
assert_eq!(dir.entries[3].name, test1_file);
769-
assert_eq!(dir.entries[3].md5, test1_md5_a);
770-
assert_eq!(dir.entries[4].name, test2_file);
771-
assert_eq!(dir.entries[4].md5, test2_md5);
708+
assert_eq!(dir.entries[1].name, dsc2_file.file_name().unwrap());
709+
assert_eq!(dir.entries[1].md5, dsc2_md5);
710+
assert_eq!(dir.entries[2].name, test1_file);
711+
assert_eq!(dir.entries[2].md5, test1_md5_a);
712+
assert_eq!(dir.entries[3].name, test2_file);
713+
assert_eq!(dir.entries[3].md5, test2_md5);
772714

773715
assert_eq!(dir.srcmd5, result.build_srcmd5);
774716

@@ -794,17 +736,15 @@ d29ybGQgaGVsbG8K
794736

795737
let mut dir = assert_ok!(package_1.list(None).await);
796738
assert_eq!(dir.rev.as_deref(), Some("4"));
797-
assert_eq!(dir.entries.len(), 5);
739+
assert_eq!(dir.entries.len(), 4);
798740
dir.entries.sort_by(|a, b| a.name.cmp(&b.name));
799741
assert_eq!(dir.entries[0].name, "_meta");
800-
assert_eq!(dir.entries[1].name, already_present_file);
801-
assert_eq!(dir.entries[1].md5, already_present_md5);
802-
assert_eq!(dir.entries[2].name, dsc3_file.file_name().unwrap());
803-
assert_eq!(dir.entries[2].md5, dsc3_md5);
804-
assert_eq!(dir.entries[3].name, test1_file);
805-
assert_eq!(dir.entries[3].md5, test1_md5_b);
806-
assert_eq!(dir.entries[4].name, test2_file);
807-
assert_eq!(dir.entries[4].md5, test2_md5);
742+
assert_eq!(dir.entries[1].name, dsc3_file.file_name().unwrap());
743+
assert_eq!(dir.entries[1].md5, dsc3_md5);
744+
assert_eq!(dir.entries[2].name, test1_file);
745+
assert_eq!(dir.entries[2].md5, test1_md5_b);
746+
assert_eq!(dir.entries[3].name, test2_file);
747+
assert_eq!(dir.entries[3].md5, test2_md5);
808748

809749
assert_eq!(dir.srcmd5, result.build_srcmd5);
810750

@@ -826,15 +766,13 @@ d29ybGQgaGVsbG8K
826766

827767
let mut dir = assert_ok!(package_1.list(None).await);
828768
assert_eq!(dir.rev.as_deref(), Some("5"));
829-
assert_eq!(dir.entries.len(), 4);
769+
assert_eq!(dir.entries.len(), 3);
830770
dir.entries.sort_by(|a, b| a.name.cmp(&b.name));
831771
assert_eq!(dir.entries[0].name, "_meta");
832-
assert_eq!(dir.entries[1].name, already_present_file);
833-
assert_eq!(dir.entries[1].md5, already_present_md5);
834-
assert_eq!(dir.entries[2].name, dsc4_file.file_name().unwrap());
835-
assert_eq!(dir.entries[2].md5, dsc4_md5);
836-
assert_eq!(dir.entries[3].name, test1_file);
837-
assert_eq!(dir.entries[3].md5, test1_md5_b);
772+
assert_eq!(dir.entries[1].name, dsc4_file.file_name().unwrap());
773+
assert_eq!(dir.entries[1].md5, dsc4_md5);
774+
assert_eq!(dir.entries[2].name, test1_file);
775+
assert_eq!(dir.entries[2].md5, test1_md5_b);
838776

839777
// Re-upload with no changes and ensure the old commit is returned.
840778

@@ -873,6 +811,6 @@ d29ybGQgaGVsbG8K
873811
// XXX: the mock apis don't set the correct rev values on branch yet
874812
assert_matches!(dir.rev, Some(_));
875813

876-
assert_eq!(dir.linkinfo[0].xsrcmd5, result.build_srcmd5);
814+
assert!(dir.linkinfo.is_empty());
877815
}
878816
}

0 commit comments

Comments
 (0)