Repository navigation
Conversation
Parameter::set removed every space from string values, so a results folder named "my results" became "myresults". Trim only leading and trailing whitespace. Create the results and remeshing folders and the last-restart link with std::filesystem instead of shell commands, which would split such paths and ignore their failures. Test a restart in a folder whose name contains spaces and a results folder that cannot be created.
genBC ran through system() with an unquoted command, and its exit status was ignored, so when genBC was missing or failed the solver read back the inputs it had just written as genBC's outputs. Launch genBC without a shell and stop with an error when it cannot start or exits with a nonzero status. Test genBC at a path that contains spaces and a missing genBC executable.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #703 +/- ##
==========================================
+ Coverage 73.45% 73.72% +0.27%
==========================================
Files 281 281
Lines 40768 40778 +10
Branches 6770 6778 +8
==========================================
+ Hits 29945 30063 +118
+ Misses 10567 10458 -109
- Partials 256 257 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
GitHub Actions
Inactive
bearsontheloose
left a comment
bearsontheloose
left a comment
There was a problem hiding this comment.
This looks good. I have three minor comments/questions.
| if (chnl_mod.appPath != "") { | ||
| auto mkdir_arg = std::string("mkdir -p ") + chnl_mod.appPath; | ||
| std::system(mkdir_arg.c_str()); | ||
| std::filesystem::create_directories(chnl_mod.appPath); |
There was a problem hiding this comment.
This changes error handling: std::system() ignored a failed mkdir, whereas std::filesystem::create_directories() throws std::filesystem::filesystem_error. The exception is caught by the top-level std::exception handler, so the run now fails immediately with a directory-creation error instead of continuing until history-file initialization fails. Is that intentional? I'm assuming it's an improvement.
| int status; | ||
| while (waitpid(process, &status, 0) == -1) { | ||
| if (errno != EINTR) throw std::system_error(errno, std::generic_category(), "Could not wait for " + executable); | ||
| } |
There was a problem hiding this comment.
Would it be helpful to report on the signal that terminated the process?
if (WIFSIGNALED(status)) { throw std::runtime_error( "Process terminated by signal " + std::to_string(WTERMSIG(status)) + ": " + executable ); }
| shutil.copytree(Path(__file__).parent / "cases/fluid/pipe_RCR_genBC", case, | ||
| ignore=shutil.ignore_patterns("*-procs", "genBC")) | ||
| with pytest.raises(RuntimeError, match="Could not start genBC"): | ||
| run_by_name(str(case), "solver.xml", 2, n_proc) |
There was a problem hiding this comment.
This checks that a missing genBC executable fails, but not that an executable which starts and exits with a nonzero status fails. That second case is part of the branch’s intended fix. Would you like to add a test?
GitHub Actions
Inactive
Current situation
Parameter::setremoves every space from string values, somy resultsbecomesmyresults.genBC_Integ_Xruns genBC throughsystem()and ignores its exit status, so a missing or failing genBC goes unnoticed.This PR trims only leading and trailing whitespace, launches genBC without a shell and stops when it cannot start or fails, and replaces the
mkdir -pandln -fshell commands for the results folder and restart links withstd::filesystem, so paths with spaces work.Fixes #693.
Release Notes
Documentation
No documentation changes are needed.
Testing
Code of Conduct & Contributing Guidelines