Skip to content

fix(executor): move auto-stamp out of run_hook into explicit API #12

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.

Sign up for GitHub

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

Merged
Sunrisepeak merged 1 commit into from
May 2, 2026
Merged

fix(executor): move auto-stamp out of run_hook into explicit API #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 42 additions & 35 deletions src/xpkg-executor.cppm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,44 +459,51 @@ public:
lua::pop(L_, 1);
}

// Auto-stamp wrapper packages (linux-headers etc.) whose install hook
// returns success but leaves install_dir empty because the real
// payload lives elsewhere (subos sysroot via config(), or a
// sub-dependency). Without something in install_dir, xlings's catalog
// probe (`is_directory && !is_empty`) flags the package as not
// installed on every dependent install, re-running this hook + the
// expensive config hook every time. Drop a tiny `.xim-installed`
// stamp so install_dir is non-empty post-install. Authors no longer
// need to remember to write this themselves.
//
// Policy: only writes when (a) Install hook succeeded, (b) install_dir
// exists or can be created, (c) install_dir is empty. If the hook
// already laid down content, we don't touch it. Failed hooks or
// genuinely-missing install_dir return paths fall through unchanged.
if (hook == HookType::Install && result.success && !ctx.install_dir.empty()) {
std::error_code ec;
fs::create_directories(ctx.install_dir, ec);
ec.clear();
bool dir_exists = fs::exists(ctx.install_dir, ec)
&& fs::is_directory(ctx.install_dir, ec);
ec.clear();
if (dir_exists && fs::is_empty(ctx.install_dir, ec)) {
auto stamp_path = ctx.install_dir / ".xim-installed";
std::ofstream out(stamp_path);
if (out.is_open()) {
// Key=value INI for human-readable + grep-friendly access.
// schema=1 reserves room to evolve the format later.
out << "schema = 1\n"
<< "name = " << ctx.pkg_name << "\n"
<< "version = " << ctx.version << "\n"
<< "platform = " << ctx.platform << "\n";
}
}
}

return result;
}

// Auto-stamp wrapper packages (linux-headers etc.) whose install hook
// returns success but leaves install_dir empty because the real
// payload lives elsewhere (subos sysroot via config(), or a
// sub-dependency). Without something in install_dir, xlings's catalog
// probe (`is_directory && !is_empty`) flags the package as not
// installed on every dependent install, re-running this hook + the
// expensive config hook every time. Drop a tiny `.xim-installed`
// stamp so install_dir is non-empty post-install. Authors no longer
// need to remember to write this themselves.
//
// CRITICAL: Call this AFTER all install paths complete (run_hook +
// any consumer-side stage_extracted_payload / default_install
// fallback). If invoked from inside run_hook, the stamp is written
// before the consumer's "is install_dir empty?" check, which would
// suppress the extracted-payload fallback used by packages whose
// install hook silently fails (e.g. tarballs without a top-level
// dir, where the hook's `os.mv(extracted_dir, install_dir)` no-ops
// because extracted_dir doesn't exist).
//
// Policy: only writes when (a) install_dir exists or can be created,
// (b) install_dir is empty. If anything else has populated it, we
// don't touch it.
void apply_install_stamp_if_empty(const ExecutionContext& ctx) {
if (ctx.install_dir.empty()) return;
std::error_code ec;
fs::create_directories(ctx.install_dir, ec);
ec.clear();
bool dir_exists = fs::exists(ctx.install_dir, ec)
&& fs::is_directory(ctx.install_dir, ec);
ec.clear();
if (!dir_exists || !fs::is_empty(ctx.install_dir, ec)) return;
auto stamp_path = ctx.install_dir / ".xim-installed";
std::ofstream out(stamp_path);
if (!out.is_open()) return;
// Key=value INI for human-readable + grep-friendly access.
// schema=1 reserves room to evolve the format later.
out << "schema = 1\n"
<< "name = " << ctx.pkg_name << "\n"
<< "version = " << ctx.version << "\n"
<< "platform = " << ctx.platform << "\n";
}

HookResult check_installed(const ExecutionContext& ctx) {
return run_hook(HookType::Installed, ctx);
}
Expand Down
77 changes: 41 additions & 36 deletions tests/test_executor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,7 +656,9 @@ std::string read_file(const fs::path& p) {
}
} // namespace

TEST(ExecutorTest, AutoStamp_WritesStampWhenInstallDirEmpty) {
TEST(ExecutorTest, ApplyInstallStamp_WritesStampWhenInstallDirEmpty) {
// Wrapper packages (linux-headers, fromsource:* aliases) leave
// install_dir empty after install hook; stamp marks them as installed.
const fs::path temp = make_temp_dir("libxpkg-stamp-empty-");
const fs::path install_dir = temp / "install";
fs::create_directories(install_dir);
Expand All @@ -676,6 +678,9 @@ TEST(ExecutorTest, AutoStamp_WritesStampWhenInstallDirEmpty) {

auto r = exec->run_hook(HookType::Install, ctx);
ASSERT_TRUE(r.success) << r.error;
// Consumer (e.g. xlings installer) calls stamp explicitly after all
// install paths (hook + extracted-payload fallback + script default).
exec->apply_install_stamp_if_empty(ctx);

auto stamp = install_dir / ".xim-installed";
EXPECT_TRUE(fs::exists(stamp)) << "auto-stamp must write .xim-installed when install_dir empty";
Expand All @@ -689,9 +694,9 @@ TEST(ExecutorTest, AutoStamp_WritesStampWhenInstallDirEmpty) {
fs::remove_all(temp);
}

TEST(ExecutorTest, AutoStamp_SkipsWhenInstallDirNonEmpty) {
// If the install hook (or anyone else) put content in install_dir
// before run_hook returns, we must NOT add the stamp file.
TEST(ExecutorTest, ApplyInstallStamp_SkipsWhenInstallDirNonEmpty) {
// If anything has populated install_dir (install hook content,
// staged extracted payload, default script install), don't add stamp.
const fs::path temp = make_temp_dir("libxpkg-stamp-nonempty-");
const fs::path install_dir = temp / "install";
fs::create_directories(install_dir);
Expand All @@ -714,76 +719,76 @@ TEST(ExecutorTest, AutoStamp_SkipsWhenInstallDirNonEmpty) {

auto r = exec->run_hook(HookType::Install, ctx);
ASSERT_TRUE(r.success) << r.error;
exec->apply_install_stamp_if_empty(ctx);

EXPECT_TRUE(fs::exists(install_dir / "payload.txt"));
EXPECT_FALSE(fs::exists(install_dir / ".xim-installed"))
<< "auto-stamp must not write when install hook already populated install_dir";
<< "auto-stamp must not write when install_dir already has content";

fs::remove_all(temp);
}

TEST(ExecutorTest, AutoStamp_SkipsWhenInstallHookFailed) {
// Failed install hooks must not leave a stamp behind. If the hook
// returned false / errored, we don't yet know whether anything is
// actually installed; defaulting to "stamp present" would mask the
// failure on the next dependent install.
const fs::path temp = make_temp_dir("libxpkg-stamp-failed-");
TEST(ExecutorTest, RunHook_DoesNotImplicitlyStamp) {
// Regression: auto-stamp used to live inside run_hook, which wrote
// .xim-installed before xlings's stage_extracted_payload_ fallback
// could check "is install_dir empty?". This poisoned the fallback
// for packages whose install hook silently no-ops (e.g. patchelf,
// whose tarball has no top-level dir, so `os.mv(extracted_dir,
// install_dir)` is a no-op). Stamp must now be explicit.
const fs::path temp = make_temp_dir("libxpkg-stamp-noimplicit-");
const fs::path install_dir = temp / "install";
fs::create_directories(install_dir);

auto pkg = temp / "failing.lua";
auto pkg = temp / "silent.lua";
write_text(pkg, std::string(
"package = { spec = \"1\", name = \"failing\", "
"package = { spec = \"1\", name = \"silent\", "
"xpm = { linux = { [\"1.0.0\"] = { url = \"x\", sha256 = \"0\" } } } }\n"
"function install() return false end\n"));
"function install() return true end\n"));

auto exec = create_executor(pkg);
ASSERT_TRUE(exec.has_value()) << exec.error();

ExecutionContext ctx = make_context(install_dir, "linux");
ctx.pkg_name = "failing";
ctx.pkg_name = "silent";
ctx.version = "1.0.0";

auto r = exec->run_hook(HookType::Install, ctx);
EXPECT_FALSE(r.success);
ASSERT_TRUE(r.success) << r.error;
EXPECT_FALSE(fs::exists(install_dir / ".xim-installed"))
<< "auto-stamp must not write when install hook returned false";
<< "run_hook must NOT write the stamp; consumers call apply_install_stamp_if_empty explicitly";

fs::remove_all(temp);
}

TEST(ExecutorTest, AutoStamp_NotWrittenForNonInstallHooks) {
// The auto-stamp is specific to HookType::Install. Other hooks
// (Config, Uninstall, Installed, Update) running on an empty
// install_dir must NOT trigger a stamp write.
const fs::path temp = make_temp_dir("libxpkg-stamp-otherhook-");
TEST(ExecutorTest, ApplyInstallStamp_IsIdempotent) {
// Calling apply_install_stamp_if_empty twice is safe — the second
// call sees a non-empty dir (the first call's stamp) and no-ops.
const fs::path temp = make_temp_dir("libxpkg-stamp-idempotent-");
const fs::path install_dir = temp / "install";
fs::create_directories(install_dir);

auto pkg = temp / "noisy.lua";
auto pkg = temp / "any.lua";
write_text(pkg, std::string(
"package = { spec = \"1\", name = \"noisy\", "
"package = { spec = \"1\", name = \"any\", "
"xpm = { linux = { [\"1.0.0\"] = { url = \"x\", sha256 = \"0\" } } } }\n"
"function install() return true end\n"
"function config() return true end\n"
"function uninstall() return true end\n"));
"function install() return true end\n"));

auto exec = create_executor(pkg);
ASSERT_TRUE(exec.has_value()) << exec.error();

ExecutionContext ctx = make_context(install_dir, "linux");
ctx.pkg_name = "noisy";
ctx.pkg_name = "any";
ctx.version = "1.0.0";

// Config hook on empty install_dir → no stamp written.
auto r = exec->run_hook(HookType::Config, ctx);
ASSERT_TRUE(r.success) << r.error;
EXPECT_FALSE(fs::exists(install_dir / ".xim-installed"));
exec->apply_install_stamp_if_empty(ctx);
auto stamp = install_dir / ".xim-installed";
ASSERT_TRUE(fs::exists(stamp));
auto first_content = read_file(stamp);

// Uninstall hook on empty install_dir → no stamp written.
r = exec->run_hook(HookType::Uninstall, ctx);
ASSERT_TRUE(r.success) << r.error;
EXPECT_FALSE(fs::exists(install_dir / ".xim-installed"));
exec->apply_install_stamp_if_empty(ctx);
auto second_content = read_file(stamp);
EXPECT_EQ(first_content, second_content)
<< "second call must not rewrite stamp";

fs::remove_all(temp);
}
Loading