Skip to content

Commit 86ddef5

Browse files
committed
fix: onelf asset paths, GHCR push staging, and container output perms
- Extract onelf icon/desktop beside the binary (packages/<parent>/) instead of the root outdir, and keep renamed icons in that same dir. - push_files stages all files into one temp dir before invoking oras. Previously oras cd'd into the first file's parent and referenced every file by basename, so shared root-outdir files (BUILD.log, CHECKSUM) were unreachable when pushing from a package subdir and the push failed with 'stat .../BUILD.log: no such file'. Same files, names, and destination as before — only multi-directory file sets are now handled. - Container builds run as root, so output subdirs the recipe creates (e.g. packages/<pkg>/) aren't writable by the host user and block host-side steps (icon/desktop extraction, signing). Append 'chmod -R a+rwX /sbuild' to the container script, preserving its exit status. chmod (not chown) is used so it works under both rootful docker and rootless docker/podman, where container root maps to the host user via a subuid range.
1 parent e26721a commit 86ddef5

3 files changed

Lines changed: 88 additions & 32 deletions

File tree

sbuild/src/builder.rs

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -608,16 +608,29 @@ impl Builder {
608608
}
609609

610610
let exec_file = x_exec.run.as_ref().map(|run| {
611-
let script = format!(
612-
"#!/usr/bin/env {}\n{}\n{}",
613-
x_exec.shell,
614-
match self.log_level {
615-
2 => "set -x",
616-
3 => "set -xv",
617-
_ => "",
618-
},
619-
run
620-
);
611+
let setx = match self.log_level {
612+
2 => "set -x",
613+
3 => "set -xv",
614+
_ => "",
615+
};
616+
// In a container the script runs as root, so subdirs the
617+
// recipe creates under the mounted /sbuild (e.g.
618+
// packages/<pkg>/) aren't writable by the host user, which
619+
// blocks later host-side steps (icon/desktop extraction,
620+
// signing). Relax permissions so the host user can write
621+
// regardless of ownership. chmod (not chown) is used
622+
// deliberately: under rootless docker/podman the container
623+
// root maps to the host user via a subuid range, so
624+
// chown-ing to a host uid would map it back out of reach;
625+
// chmod works the same under both rootful and rootless
626+
// runtimes. Preserve the script's real exit status.
627+
let trailer = if build_config.x_exec.container.is_some() {
628+
"\n__sbuild_rc=$?\nchmod -R a+rwX /sbuild 2>/dev/null || true\nexit $__sbuild_rc"
629+
} else {
630+
""
631+
};
632+
let script =
633+
format!("#!/usr/bin/env {}\n{}\n{}{}", x_exec.shell, setx, run, trailer);
621634
let tmp = temp_file(pkg_id, &script);
622635
tmp.to_string_lossy().to_string()
623636
});
@@ -848,13 +861,19 @@ impl Builder {
848861
self.pkg_type = PackageType::Onelf;
849862
}
850863

864+
// Write extracted assets next to the binary so sub-package
865+
// provides (packages/<parent>/<cmd>) land in their own dir
866+
// rather than the root outdir.
867+
let dest_dir = provide_path.parent().unwrap_or_else(|| Path::new(""));
868+
851869
match OnelfPackage::open(&provide_path) {
852870
Ok(mut pkg) => {
853871
if self.icon.get(&provide).is_none() {
854-
let dest = format!("{}.DirIcon", cmd);
872+
let dest = dest_dir.join(format!("{}.DirIcon", cmd));
855873
match pkg.extract_icon(cmd, &dest) {
856874
Ok(Some(())) => {
857-
self.logger.info(&format!("Extracted icon to {}", dest));
875+
self.logger
876+
.info(&format!("Extracted icon to {}", dest.display()));
858877
self.rename_icon(dest, context, &provide, cmd);
859878
}
860879
Ok(None) => {}
@@ -866,10 +885,11 @@ impl Builder {
866885
}
867886
}
868887
if self.desktop.get(&provide).is_none() {
869-
let dest = format!("{}.desktop", cmd);
888+
let dest = dest_dir.join(format!("{}.desktop", cmd));
870889
match pkg.extract_desktop(cmd, &dest) {
871890
Ok(Some(())) => {
872-
self.logger.info(&format!("Extracted desktop to {}", dest));
891+
self.logger
892+
.info(&format!("Extracted desktop to {}", dest.display()));
873893
self.desktop.insert(provide.clone(), true);
874894
}
875895
Ok(None) => {}
@@ -926,12 +946,15 @@ impl Builder {
926946
} else {
927947
None
928948
} {
929-
let final_path = format!("{}.{}", cmd, extension);
949+
// Keep the renamed icon in the same directory as the source so
950+
// sub-package icons don't get hoisted to the root outdir.
951+
let dir = file_path.parent().unwrap_or_else(|| Path::new(""));
952+
let final_path = dir.join(format!("{}.{}", cmd, extension));
930953
fs::rename(&file_path, &final_path).unwrap();
931954
self.logger.info(&format!(
932955
"Renamed {} to {}",
933956
file_path.display(),
934-
final_path
957+
final_path.display()
935958
));
936959
self.icon.insert(provide.to_string(), true);
937960
} else {

sbuild/src/ghcr.rs

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -107,32 +107,38 @@ impl GhcrClient {
107107
// Add target
108108
cmd.arg(&target);
109109

110-
// Collect files and determine working directory
111-
// We need to cd to the directory so oras stores just filenames, not full paths
112-
let mut work_dir: Option<&Path> = None;
110+
// oras stores each pushed file under its basename relative to the
111+
// working directory. Our file set can span multiple directories (the
112+
// package subdir plus shared files in the root outdir like BUILD.log
113+
// and CHECKSUM), so cd-ing to a single source directory can't see all
114+
// of them. Stage every file into one temp dir and push from there.
115+
let staging = tempfile::tempdir()?;
113116
let mut filenames: Vec<String> = Vec::new();
117+
let mut seen: std::collections::HashSet<String> = std::collections::HashSet::new();
114118

115119
for file in files {
116120
let path = file.as_ref();
117-
if path.exists() {
118-
if work_dir.is_none() {
119-
work_dir = path.parent();
120-
}
121-
if let Some(filename) = path.file_name() {
122-
filenames.push(filename.to_string_lossy().to_string());
123-
}
121+
if !path.exists() {
122+
continue;
124123
}
124+
let Some(filename) = path.file_name() else {
125+
continue;
126+
};
127+
let name = filename.to_string_lossy().to_string();
128+
// First occurrence of a basename wins; skip later duplicates so a
129+
// shared file never clobbers a package-specific one.
130+
if !seen.insert(name.clone()) {
131+
continue;
132+
}
133+
std::fs::copy(path, staging.path().join(filename))?;
134+
filenames.push(name);
125135
}
126136

127-
// Add filenames (relative to work_dir)
137+
// Add filenames (relative to the staging dir)
128138
for filename in &filenames {
129139
cmd.arg(filename);
130140
}
131-
132-
// Set working directory if we have one
133-
if let Some(dir) = work_dir {
134-
cmd.current_dir(dir);
135-
}
141+
cmd.current_dir(staging.path());
136142

137143
let output = cmd.output()?;
138144

sbuild/tests/onelf_extract.rs

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
//! ```
88
99
use std::env;
10+
use std::path::Path;
1011

1112
use sbuild::onelf::OnelfPackage;
1213

@@ -38,3 +39,29 @@ fn extracts_icon_and_desktop() {
3839
);
3940
eprintln!("desktop:\n{desktop}");
4041
}
42+
43+
/// Mirrors the builder's dest-dir computation: for a sub-package binary at
44+
/// `packages/<parent>/<cmd>`, extracted assets must land in that same dir,
45+
/// not the root.
46+
#[test]
47+
#[ignore]
48+
fn extracts_next_to_subpackage_binary() {
49+
let path = env::var("ONELF_TEST_FILE").expect("set ONELF_TEST_FILE");
50+
let cmd = env::var("ONELF_TEST_CMD").unwrap_or_else(|_| "amdgpu_top".to_string());
51+
let provide_path = Path::new(&path);
52+
let dest_dir = provide_path.parent().unwrap_or_else(|| Path::new(""));
53+
54+
let mut pkg = OnelfPackage::open(provide_path).expect("open onelf");
55+
56+
let desktop_dest = dest_dir.join(format!("{cmd}.desktop"));
57+
let got = pkg
58+
.extract_desktop(&cmd, &desktop_dest)
59+
.expect("extract desktop");
60+
assert!(got.is_some(), "expected a desktop file");
61+
assert!(
62+
desktop_dest.exists(),
63+
"desktop should be written beside the binary at {}",
64+
desktop_dest.display()
65+
);
66+
eprintln!("desktop written to: {}", desktop_dest.display());
67+
}

0 commit comments

Comments
 (0)