Skip to content

Conversation

smoors
Copy link
Contributor

@smoors smoors commented Jun 21, 2025

required for installing in bwrap namespace, see also #4110
if install dir is bind mounted, it cannot be removed

@smoors smoors added the change label Jun 21, 2025
Samuel Moors added 2 commits June 21, 2025 14:58
@boegel
Copy link
Member

boegel commented Aug 13, 2025

@smoors As discussed during today's conf call, it probably makes sense to introduce another function (clean_dir?) that does a two-step approach: try to remove the specified directory, fall back to emptying it when that fails for some reason.

@smoors smoors changed the title empty install dir but don't remove it empty install dir removing it fails Aug 14, 2025
@smoors smoors changed the title empty install dir removing it fails empty install dir if removing it fails Aug 14, 2025
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smoors Let's also add a test for the clean_dir function?

@boegel boegel changed the title empty install dir if removing it fails try to empty install dir if removing it fails Aug 27, 2025
@boegel boegel added enhancement and removed change labels Aug 27, 2025
Comment on lines 1252 to 1258
if clean_instead_of_remove:
# clean the installation directory: first try to remove it; if that fails, empty it
clean_dir(dir_name)
self.log.info(f"Cleaned old directory {dir_name}")
else:
remove_dir(dir_name)
self.log.info(f"Removed old directory {dir_name}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this whole block be under an if clean:?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, only the last else block can go under if clean (although not strictly necessary). if clean_instead_of_remove, we should always clean it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 66ae73d

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@boegel boegel merged commit 16d3f4d into easybuilders:develop Sep 8, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants