Preserve sticky module requirements during purge - #693
Conversation
a005bf0 to
2f4a98e
Compare
xdelaruelle
left a comment
There was a problem hiding this comment.
Testsuite should be fixed as CI jobs are failing.
Documentation should be adapted to mention this behavior change, especially in module.rst, changes.rst, the sticky/module-tags design docs and the sticky-modules cookbook.
Also good to check if this has an impact on other stuff mentioning sticky modules like stash-environment, initial-environment, conflict_unload, evaluation-errors. Check these design doc and adapt them if required.
18f8719 to
4d70791
Compare
3885540 to
d3c36a3
Compare
d3c36a3 to
f330e14
Compare
Purge skips sticky and super-sticky modules but still attempts to unload their non-sticky requirements. This reports an unrelated dependency error without explaining why the requirement remains loaded. Preserve requirements needed by retained sticky modules and apply the sticky_purge reporting mode to their skipped unload. Fixes envmodules#582 Assisted-by: Codex:gpt-6-astra Signed-off-by: Adrien Cotte <adrien.cotte@asplus.fr>
On a forced purge, a sticky module required by a retained super-sticky module got a "forced unload" warning immediately followed by the error reporting its unload is skipped as a super-sticky requirement. The module stayed loaded, but the sticky check reported the forced unload before the requirement check retained the module. Check whether a module is required by a retained sticky module as part of the sticky check itself, prior reporting the forced unload of a sticky module. Rely on the existing sticky module test to determine if a sticky dependent is retained rather than duplicating this rule. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The requirement check on purge only looked at dependent modules loaded after the module to unload. A requirement loaded after its super-sticky dependent, for instance reloaded after a forced unload, was purged and left this dependent with a missing dependency, the very situation this protection aims to avoid. Look at dependent modules whatever their load order and drop the direct dependent pre-check that relied on load order too. Also cover the abort of the purge when the error raised for a retained requirement occurs with abort_on_error configured for this sub-command. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
Move the NEWS entry to the end of the 5.7.0 list, as entries follow the order changes landed. Report in the cookbook example the output of the env/base load, which auto-loads its requirements, and the auto-loaded tag these requirements then carry in the module list. Also restore the blank line removed from NEWS.rst, unrelated to this change. Assisted-by: Claude:claude-fable-5-1 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
f330e14 to
a61af73
Compare
xdelaruelle
left a comment
There was a problem hiding this comment.
While reviewing I found two gaps in the new protection, so I rebased the branch on current main and pushed three follow-up commits on top of yours:
- Keep sticky requirement of super-sticky module on forced purge. A requirement tagged sticky and needed by a retained super-sticky module got two contradictory messages on purge -f: "Unload of sticky module forced" immediately followed by "Unload of super-sticky module requirement skipped". The sticky check ran first and reported the forced unload before the requirement check retained the module. The requirement check now lives inside failOrSkipUnloadIfSticky, ahead of the forced-unload warning. It also reuses isModuleSticky for the dependent stickiness rule and drops the extra call in cmdModuleUnload.
- Protect sticky module requirement whatever its load order on purge. The dependent lookup used the positional-order dependency hash, so a requirement loaded after its super-sticky dependent (for instance after module unload -f req then module load req) was purged and left the dependent with a missing prereq, which is the very situation #582 describes. The lookup now uses the non-positional-order hash. Tests cover both load orders, and the interplay with abort_on_error when the skipped requirement raises an error. A bullet in the sticky-modules design doc records this.
- doc: fix NEWS placement and cookbook example. The NEWS entry moves to the end of the 5.7.0 list, as entries follow the order changes landed. The cookbook example now shows the output of module load env/base, whose requirements are auto-loaded, and the tags they carry in module list.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Purge skips sticky and super-sticky modules but still attempts to unload their non-sticky requirements. This reports an unrelated dependency error without explaining why the requirement remains loaded.
Preserve requirements needed by retained sticky modules and apply the sticky_purge reporting mode to their skipped unload.
Fixes #582
Assisted-by: Codex:gpt-6-astra