Skip to content

Revert "Make clean-tutorial.sh executable from any directory"#757

Open
MakisH wants to merge 1 commit intodevelopfrom
revert-716-fix-clean-scripts
Open

Revert "Make clean-tutorial.sh executable from any directory"#757
MakisH wants to merge 1 commit intodevelopfrom
revert-716-fix-clean-scripts

Conversation

@MakisH
Copy link
Copy Markdown
Member

@MakisH MakisH commented Mar 26, 2026

Reverts #716

Since #716, running ./clean-tutorial.sh seems to be triggering the OpenFOAM cleanCase, which deletes *.xml, deleting the precice-config.xml.

Not sure if there is an easy fix, but I am opening this as a reminder for the next release.

@MakisH MakisH self-assigned this Mar 26, 2026
@PranjalManhgaye
Copy link
Copy Markdown
Contributor

PranjalManhgaye commented Mar 28, 2026

Hii @MakisH i went into why cleaning could go wrong after #716, and i think i see a concrete shell issue on top of whatever OpenFOAM cleanCase was doing.

What I think went wrong
a lot of clean-tutorial.sh files were basically a single line pointing at ../tools/clean-tutorial-base.sh. i didn’t realize at first that the shell treats that as a path relative to where you’re standing (cwd), not relative to the tutorial folder. So if you run something like ./some-tutorial/clean-tutorial.sh from the repo root, it’s easy to end up resolving the wrong path or cleaning the wrong tree.
also, i have learned the hard way that cd "$(dirname "$0")" doesn’t save you when $0 is ./clean-tutorial.sh, because dirname is just . and cd . follows cwd, not the script’s real folder.

I’m not 100% sure that’s the only story behind precice-config.xml disappearing — ig OpenFOAM’s cleanCase could still be part of it — but this cwd/path stuff can definitely put you in a bad state before any case cleaner runs.

What i did instead

  • i added a small wrapper in each tutorial’s clean-tutorial.sh that resolves the tutorial directory using the usual ${PWD}/$0 trick, sets PRECICE_TUTORIAL_DIR, and then sources clean-tutorial-base.sh (so we don’t exec and accidentally make $0 look like tools/...).
  • clean-tutorial-base.sh now prefers PRECICE_TUTORIAL_DIR when the wrapper sets it, and still has a guard so we don’t run tools/clean-tutorial-base.sh directly (that would set the “tutorial dir” to tools/ and walk the wrong tree).
  • We applied the same ${PWD}/$0 idea to clean-all.sh for consistency.

What I checked locally

  • I ran ./elastic-tube-1d/clean-tutorial.sh from the repo root and from inside the tutorial — precice-config.xml was still there afterward.
  • I checked partitioned-heat-conduction-direct the same way after switching it to the shared pattern.
  • I haven’t re-run a full OpenFOAM-heavy tutorial here without sourcing OpenFOAM, so I’ll rely on you / CI for that if needed.

i am happy to open a PR with this (either alongside or after #757) — whatever fits your release plan best. If my read of the failure mode is off, i am totally open to adjusting after a repro you prefer. (whenever contributions again starts).

@AdityaGupta716
Copy link
Copy Markdown

hi @MakisH @PranjalManhgaye I have gone through the issue thought i might be able to help ,i saw the cleaning code paths and found that ESI OpenFOAM's cleanAuxiliary() does rm -rf ./*.xml this would delete precice-config.xml if cleanCase ever runs at the tutorial root level. The Foundation version doesn't have this line i think it's specific to the ESI/OpenCFD version.

Normally cleanCase runs inside case subdirectories where there are no .xml files, so it's safe. Something after #716 must be causing it to run at the tutorial root instead, though I wasn't able to reproduce it.

For the fix, I thought of two things :

  1. Replace symlinks with small wrapper scripts : removes any shell/platform-dependent symlink resolution from the equation:
#!/usr/bin/env sh
set -e -u
cd "$(dirname "$0")"
. ../tools/clean-tutorial-base.sh
  1. Add a guard in clean_openfoam in cleaning-tools.sh : so even if directory resolution goes wrong in the future, precice-config.xml can never be accidentally deleted:
 clean_openfoam() {
     (
         set -e -u
         cd "$1"
+        if [ -f "precice-config.xml" ]; then
+            echo "Error: clean_openfoam called at tutorial root level, skipping" >&2
+            exit 1
+        fi
         echo "- Cleaning up OpenFOAM case in $(pwd)"
         if [ -n "${WM_PROJECT:-}" ] || error "No OpenFOAM environment is active."; then

what do you guys think ?

@PranjalManhgaye
Copy link
Copy Markdown
Contributor

thanks @AdityaGupta716, yeah ig your point about esi/opencfd cleanCase deleting *.xml is valid, and that guard in clean_openfoam is a really good safety net. i had been thinking about the same issue from the path-resolution side, and ig both views fit together.
for long term, i think we should combine both: your guard + wrapper-based clean-tutorial.sh path handling, so scripts always run from the correct tutorial dir even from repo root. if this sounds good, i can open a pr with both together.

@AdityaGupta716
Copy link
Copy Markdown

@PranjalManhgaye Yea, lets wait for now and let @MakisH decide ,if he think its good, u can go ahead and open a pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants