Skip to content

always honor keeppreviousinstall setting #4876

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.

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

Closed
wants to merge 3 commits into from

Conversation

smoors
Copy link
Contributor

@smoors smoors commented May 11, 2025

needed for installing software in a bwrap namespace, where we bind mount the software installdir (name/version) of each software that will be installed by EB.

this in turn requires setting keeppreviousinstall to True, because removing a bind mounted directory is not possible. we currently set this in the parse hook (would be useful to have a build option for this).

EB currently hard sets keeppreviousinstall to False if buildininstalldir is True, which breaks the bwrap installation, for example for ecBuild.

this PR prints a big warning instead of changing keeppreviousinstall.

see also #4110

@smoors smoors added the change label May 11, 2025
@smoors smoors marked this pull request as draft May 11, 2025 12:50
Samuel Moors added 2 commits May 11, 2025 15:13
@smoors smoors marked this pull request as ready for review May 11, 2025 13:57
@boegel boegel added this to the 5.x milestone May 20, 2025
@boegel
Copy link
Member

boegel commented May 20, 2025

@smoors Hmm, it seems rather intrusive to change this behaviour, is this really the only way?

keepreviousinstall and buildininstalldir are fundamentally incompatible, so I'm not keen on just printing a warning here and pretending it'll all work out well

@smoors
Copy link
Contributor Author

smoors commented May 20, 2025

@smoors Hmm, it seems rather intrusive to change this behaviour, is this really the only way?

keepreviousinstall and buildininstalldir are fundamentally incompatible, so I'm not keen on just printing a warning here and pretending it'll all work out well

i agree that it's an intrusive change, but we really need a way to avoid cleaning up the installdir when building in the installdir. otherwise we have to modify each easyconfig that uses buildininstalldir.

the alternative would be to add another parameter to enforce this, e.g. keeppreviousbuild, would you prefer that?

@boegel
Copy link
Member

boegel commented May 20, 2025

@smoors Hmm, it seems rather intrusive to change this behaviour, is this really the only way?
keepreviousinstall and buildininstalldir are fundamentally incompatible, so I'm not keen on just printing a warning here and pretending it'll all work out well

i agree that it's an intrusive change, but we really need a way to avoid cleaning up the installdir when building in the installdir. otherwise we have to modify each easyconfig that uses buildininstalldir.

the alternative would be to add another parameter to enforce this, e.g. keeppreviousbuild, would you prefer that?

Maybe that makes more sense, yes. That way we're at least not changing existing behavior.

Though it still feels a bit off somehow...

@smoors
Copy link
Contributor Author

smoors commented May 20, 2025

another solution would be to only remove the contents of the installdir, but not the installdir itself. with bwrap, the installdir will appear empty anyway, so that will be very fast.

@smoors
Copy link
Contributor Author

smoors commented May 21, 2025

alternative implementation with new param forcekeeppreviousinstall in #4890

@smoors
Copy link
Contributor Author

smoors commented May 24, 2025

thinking more about this, i think the best solution is to add a build option to indicate that the installdir is mounted in a new namespace. that makes it clear why we are doing this, and this option can be used by the eb wrapper to install with bwrap. it will also allow fixing the Tarball easyblock, which needs self.cfg['install_type'] = 'merge', again to avoid removing the installdir.

@smoors
Copy link
Contributor Author

smoors commented May 25, 2025

closing in favor of #4894

@smoors smoors closed this May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants