skip lines that start with 'module-version' when determining whether a module exists in ModulesTool.exist#3379
Conversation
…a module exists in ModulesTool.exist (fixes easybuilders#3376)
|
@casparvl Can you verify this fix? @Flamefire Thoughts? |
…odulesTool.exist + add more logging
…dule show' output
|
I can confirm that the current version of |
Flamefire
left a comment
Flamefire
left a comment
There was a problem hiding this comment.
Looks good so far but I'd recheck the self.log calls especially the distinction between debug and info. IMO for info at most 1 line per module should be printed which contains the module name and the result. Detailed information as for how it was determined or what is going to happen should be debug
To be concrete:
Checking whether %s existsshould bedebugor none at all if you promote theResult for existence check of %s module: %stoinfo. IMO it doesn't provide any further informationChecking whether %s exists based on output of 'module show'is not required at all especially not asinfobecause it is a local method only called locally with an already existing log message- For consistency with the
not visiblebranchModule %s not found in list of available modules, checking via 'module show'should bedebugand hence thefound in list of available modulesshould be too.
casparvl
left a comment
casparvl
left a comment
There was a problem hiding this comment.
With this PR we still take the more cautious approach to checking if modules exist by requiring a rather explicit regexp pattern, as was proposed by the original implementation from @Flamefire in EB 4.2.1. At the same time, this PR accounts for the specific behaviour of (old) Tcl when performing a module show, which is different than that of LMOD and was thus not anticipated by @Flamefire in his original implementation.
Seems to me like the best of both worlds.
Thanks @boegel for fixing this so quickly. Going in!
|
|
||
| # skip whitespace lines | ||
| if OUTPUT_MATCHES['whitespace'].search(line): | ||
| self.log.debug("Treating line '%s' as whitespace, so skipping it", line) |
There was a problem hiding this comment.
isn't this overly verbose, even for debugging?
There was a problem hiding this comment.
It helped a lot with figuring out the problem reported in #3376, so no.
| self.log.debug("Result for existence check of wrapped module %s: %s", wrapped_mod, mod_exists) | ||
|
|
||
| self.log.debug("Result for existence check of %s module: %s", mod_name, mod_exists) | ||
| self.log.info("Result for existence check of %s module: %s", mod_name, mod_exists) |
There was a problem hiding this comment.
why bump all these debugs to info?
There was a problem hiding this comment.
Because the non-debug log wasn't verbose enough to make it clear what was going on.
4 participants
fixes #3376