From fca95ecc199610e88abc266f30d420b806b7c4b0 Mon Sep 17 00:00:00 2001 From: silverweed Date: Wed, 20 May 2026 10:47:06 +0200 Subject: [PATCH 1/2] [main] Add --abort-on-failure flag to rootcp and some sanity checks --- main/src/rootcp.cxx | 113 +++++++++++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 38 deletions(-) diff --git a/main/src/rootcp.cxx b/main/src/rootcp.cxx index dc9de89a486d3..e222c3ba28984 100644 --- a/main/src/rootcp.cxx +++ b/main/src/rootcp.cxx @@ -27,24 +27,25 @@ using namespace ROOT::CmdLine; static const char *const kShortHelp = "usage: rootcp [-h] [-c COMPRESS] [--recreate] [-r|--recursive] [--replace] " - "[-v|--verbose] SOURCE [SOURCE ...] DEST\n"; + "[-v|--verbose] [-a|--abort-on-failure] SOURCE [SOURCE ...] DEST\n"; static const char *const kLongHelp = R"( Copy objects from ROOT files into another positional arguments: - SOURCE Source file(s) - DEST Destination file + SOURCE Source file(s) + DEST Destination file options: - -h, --help show this help message and exit + -h, --help show this help message and exit -c, --compress COMPRESS - change the compression settings of the destination file (if not already - existing). - --recreate recreate the destination file. - -r, --recursive recurse inside directories - --replace replace object if already existing - -v be verbose - -vv be even more verbose + change the compression settings of the destination file (if not already + existing). + --recreate recreate the destination file. + -r, --recursive recurse inside directories + --replace replace object if already existing + -a, --abort-on-failure abort if any object fails to be copied (instead of skipping such objects) + -v be verbose + -vv be even more verbose Note: If an object has been written to a file multiple times, rootcp will copy only the latest version of that object. @@ -86,6 +87,7 @@ struct RootCpArgs { bool fRecreate = false; bool fReplace = false; bool fRecursive = false; + bool fAbortOnFailure = false; std::vector fSources; }; @@ -100,6 +102,7 @@ static RootCpArgs ParseArgs(const char **args, int nArgs) opts.AddFlag({"--recreate"}); opts.AddFlag({"--replace"}); opts.AddFlag({"-r", "--recursive"}); + opts.AddFlag({"-a", "--abort-on-failure"}); opts.AddFlag({"-h", "--help"}); opts.AddFlag({"-v"}); opts.AddFlag({"-vv"}); @@ -124,6 +127,7 @@ static RootCpArgs ParseArgs(const char **args, int nArgs) outArgs.fRecursive = opts.GetSwitch("recursive"); outArgs.fReplace = opts.GetSwitch("replace"); outArgs.fRecreate = opts.GetSwitch("recreate"); + outArgs.fAbortOnFailure = opts.GetSwitch("abort-on-failure"); if (opts.GetSwitch("vv")) SetLogVerbosity(3); @@ -143,7 +147,7 @@ static std::unique_ptr OpenFile(const char *fileName, const char *mode) gErrorIgnoreLevel = kError; auto file = std::unique_ptr(TFile::Open(fileName, mode)); if (!file || file->IsZombie()) { - Err() << "File " << fileName << "does not exist.\n"; + Err() << "File `" << fileName << "` does not exist.\n"; return nullptr; } gErrorIgnoreLevel = origLv; @@ -175,7 +179,9 @@ static std::pair DecomposePath(std::string_v // Copies `nodeIdx`-th node from `src`'s object tree to the file in `dest`. // `nodeIdx` is assumed to be in range. -static void CopyNode(const RootSource &src, const RootCpDestination &dest, NodeIdx_t nodeIdx, const RootCpArgs &args) +// Returns true if the node was successfully copied. +[[nodiscard]] static bool +CopyNode(const RootSource &src, const RootCpDestination &dest, NodeIdx_t nodeIdx, const RootCpArgs &args) { TFile *srcfile = src.fObjectTree.fFile.get(); // The file is guaranteed to be valid by ParseRootSource: if this crashes, it's a bug in there. @@ -208,7 +214,7 @@ static void CopyNode(const RootSource &src, const RootCpDestination &dest, NodeI if (src.fFileName == dest.fFname && srcFullPath == destFullPath) { Err() << src.fFileName << ":" << srcFullPath << ": source and destination cannot be the same\n"; - return; + return false; } Info(2) << "cp " << src.fFileName << ":" << srcFullPath << " -> " << dest.fFname << ":" << destFullPath << "\n"; @@ -229,33 +235,33 @@ static void CopyNode(const RootSource &src, const RootCpDestination &dest, NodeI if (destKey && !TClass::GetClass(destKey->GetClassName())->InheritsFrom("TDirectory") && !args.fReplace) { Err() << "an object of type '" << destKey->GetClassName() << "' already exists at " << dest.fFname << ':' << destFullPath << ". Use the --replace flag to overwrite existing objects.\n"; - return; + return false; } // retrieve the object's key const TDirectory *srcDir = srcfile->GetDirectory(std::string(srcDirPath).c_str(), true); if (!srcDir) { Err() << "failed to get source directory '" << srcDirPath << "'\n"; - return; + return false; } const TKey *srcKey = srcDir->GetKey(node.fName.c_str()); if (!srcKey) { Err() << "failed to read key of object '" << srcFullPath << "'\n"; - return; + return false; } // Verify that the class is known and supported. const std::string &className = node.fClassName; const TClass *cl = TClass::GetClass(className.c_str()); if (!cl) { - Err() << "unknown object type: " << className << "; object will be skipped.\n"; - return; + Err() << "unknown object type: '" << className << "'.\n"; + return false; } Info(3) << "read object \"" << srcFullPath << "\" of type " << node.fClassName << "\n"; if (!destDir) { Err() << "failed to create or get destination directory \"" << dest.fFname << ":" << destDirPath << "\"\n"; - return; + return false; } // Delete previous object if we're replacing it @@ -267,19 +273,24 @@ static void CopyNode(const RootSource &src, const RootCpDestination &dest, NodeI // if (cl->InheritsFrom("TObject")) { TObject *obj = node.fKey->ReadObj(); - if (!obj) { + if (!obj || obj->IsZombie()) { Err() << "failed to read object \"" << srcFullPath << "\".\n"; - return; + return false; } if (TTree *old = dynamic_cast(obj)) { // special case for TTree TDirectory::TContext ctx(gDirectory, destDir); obj = old->CloneTree(-1, "fast"); - if (dest.fIsNewObject) { - static_cast(obj)->SetName(std::string(destBaseName).c_str()); + if (!obj || obj->IsZombie()) { + Err() << "failed to clone tree '" << srcFullPath << "'.\n"; + return false; + } else { + if (dest.fIsNewObject) { + static_cast(obj)->SetName(std::string(destBaseName).c_str()); + } + obj->Write(); } - obj->Write(); old->Delete(); } else if (cl->InheritsFrom("TDirectory")) { // directory @@ -290,8 +301,11 @@ static void CopyNode(const RootSource &src, const RootCpDestination &dest, NodeI destDir->mkdir(node.fName.c_str(), srcKey->GetTitle(), true); RootCpDestination dest2 = dest; dest2.fPath = dest.fPath + (dest.fPath.empty() ? "" : "/") + node.fName; - for (auto childIdx = node.fFirstChild; childIdx < node.fFirstChild + node.fNChildren; ++childIdx) - CopyNode(src, dest2, childIdx, args); + for (auto childIdx = node.fFirstChild; childIdx < node.fFirstChild + node.fNChildren; ++childIdx) { + if (!CopyNode(src, dest2, childIdx, args) && args.fAbortOnFailure) { + return false; + } + } } } else { // regular TObject @@ -301,7 +315,10 @@ static void CopyNode(const RootSource &src, const RootCpDestination &dest, NodeI } else { Warn() << "object '" << node.fName << "' of type '" << node.fClassName << "' will not be copied, as its type is currently unsupported by rootcp.\n"; + return false; } + + return true; } int main(int argc, char **argv) @@ -398,16 +415,28 @@ int main(int argc, char **argv) if (args.fCompression) destFile->SetCompressionSettings(*args.fCompression); + const auto DoNodeCopy = [](auto &&src, auto &&dst, auto nodeIdx, auto &&args_) { + bool ok = CopyNode(src, dst, nodeIdx, args_); + if (ok) + return true; + + if (args_.fAbortOnFailure) { + Err() << "node failed to be copied. Aborting.\n"; + return false; + } else { + Warn() << "node will be skipped. Use --abort-on-failure if you want to abort on failure.\n"; + return true; + } + }; + const std::uint32_t flags = args.fRecursive * EGetMatchingPathsFlags::kRecursive; - bool errors = false; for (const auto &[srcFname, srcPattern] : sourcesFileAndPattern) { auto src = ROOT::CmdLine::GetMatchingPathsInFile(srcFname, srcPattern, flags); if (!src.fErrors.empty()) { for (const auto &err : src.fErrors) Err() << err << "\n"; - errors = true; - break; + goto errors; } // We should never register files to the global list for performance reasons. @@ -418,8 +447,7 @@ int main(int argc, char **argv) Err() << "multiple sources were specified but destination path \"" << destFname << ":" << destPath << "\" is not a directory.\n"; - errors = true; - break; + goto errors; } // Iterate all objects we need to copy @@ -429,24 +457,33 @@ int main(int argc, char **argv) dest.fIsNewObject = destIsNewObject; dest.fPath = destPath; for (auto nodeIdx : src.fObjectTree.fLeafList) { - CopyNode(src, dest, nodeIdx, args); + if (!DoNodeCopy(src, dest, nodeIdx, args) && args.fAbortOnFailure) { + goto errors; + } } for (auto nodeIdx : src.fObjectTree.fDirList) { if (nodeIdx == 0) { - // The root file node needs special treatment; for all other "top-level" directories, CopyNode handles them. + // The root file node needs special treatment; for all other "top-level" directories, CopyNode handles + // them. const auto &node = src.fObjectTree.fNodes[nodeIdx]; for (auto childIdx = node.fFirstChild; childIdx < node.fFirstChild + node.fNChildren; ++childIdx) - CopyNode(src, dest, childIdx, args); + if (!DoNodeCopy(src, dest, childIdx, args) && args.fAbortOnFailure) + goto errors; } else { - CopyNode(src, dest, nodeIdx, args); + if (!DoNodeCopy(src, dest, nodeIdx, args) && args.fAbortOnFailure) + goto errors; } } } - if (errors && !srcIsSameAsDstFile) { + // Success. + return 0; + +errors: + if (!srcIsSameAsDstFile) { // If the destination file was fresh, make sure we don't end up with a half-copied file in case of errors. gSystem->Unlink(std::string(destFname).c_str()); } - return errors; + return 1; } From d87406cd38b0e92a4e1aafeb548d4ce91691349c Mon Sep 17 00:00:00 2001 From: silverweed Date: Fri, 12 Jun 2026 09:06:02 +0200 Subject: [PATCH 2/2] [roottest] add --recreate flag to a RootCp test Otherwise it may fail to run if the file is already there --- roottest/main/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roottest/main/CMakeLists.txt b/roottest/main/CMakeLists.txt index e2272f4571d92..999b3f66cf0c4 100644 --- a/roottest/main/CMakeLists.txt +++ b/roottest/main/CMakeLists.txt @@ -378,7 +378,7 @@ ROOTTEST_ADD_TEST(RootcpReplaceEntireFileClean if(xrootd) ROOTTEST_ADD_TEST(RootcpReadRemote - COMMAND ${TOOLS_PREFIX}/rootcp${exeext} root://eospublic.cern.ch//eos/root-eos/h1/dstarmb.root copy_remote.root + COMMAND ${TOOLS_PREFIX}/rootcp${exeext} --recreate root://eospublic.cern.ch//eos/root-eos/h1/dstarmb.root copy_remote.root FIXTURES_SETUP main-RootcpReadRemote-fixture) ROOTTEST_ADD_TEST(RootcpReadRemoteCheckOutput