filter out load statements that extend the $MODULEPATH to make the module being installed available#1016
Conversation
…dule being installed available
…rated with HierarchicalMNS
|
@wpoely86: please review? |
There was a problem hiding this comment.
Minor remark: puts these alphabetically?
|
A first quick read and it seems fine to me. |
… simplify (and correct) code by checking extensions full path
|
This fix makes sense to me. |
There was a problem hiding this comment.
Something that caught my eye: the first \s in this regex uses +. Shouldn't that be *? (like with module use below?)
There was a problem hiding this comment.
Yeah, it should, but this works since currently "module load" statements are always guarded, and hence indented.
Will fix, nice catch!
|
After a second read, still looks good. |
|
fixes pushed for remarks, I'll keep this on hold until @stdweird takes a look at this (per his request) |
There was a problem hiding this comment.
why all the realpath calls? any particular issue encountered?
There was a problem hiding this comment.
I ran into issues with symlinks with the unit tests (on OSX, /tmp is a symlink to /private/tmp for historical reasons). We're both comparing paths and taking path prefixes, using realpath basically gets rid of the symlinks.
There was a problem hiding this comment.
Actually, using realpath shouldn't be necessary (anymore) since we're using os.path.samefile below.
There was a problem hiding this comment.
remove the intermediate varaible and comment, it's confusing
|
@stdweird: remarks fixed again |
There was a problem hiding this comment.
Why do this? You remove them yourself in the function below? If you want to know if a module extends, a simple module in dict should do the trick?
There was a problem hiding this comment.
this is a more general method, returning a dict that doesn't contain some of the modules in the original list may be easily considered a bug?
|
minor remarks to improve readability, good to go otherwise. (it feels a bit brute force imho, but can't quite propose something better). no reason to delay this anyfurther, i doubt it has a real performnace impact. |
filter out load statements that extend the $MODULEPATH to make the module being installed available
This is a fix for #996.
I tried very hard to make this a generic fix, i.e. one that should work with any hierarchical module naming scheme, not just the 2-level one using
Core,Compiler/<comp_name>/<comp_version>andMPI/<comp_name>/<comp_version>/<mpi_name>/<mpi_version>.Basically, what is being done now in
make_module_depis to figure out which dependency module is responsible for extending$MODULEPATHwith the location where the module being installed will end up. That module is being excluded from the list of dependencies for which load statements are included in the generated module file.This is done recursively: the module that makes the excluded dependency module being available is then also excluded, etc. Until the top of the hierarchy is reached, or none of the dependencies extends
$MODULEPATHwith the specified path.This results in
GCC/4.8.2andOpenMPI/1.6.4being excluded in a module built with agompiorgoolftoolchain, and should also work in other hierarchical module naming schemes.@rtmclay: Does this make sense to you?
(CC: @ocaisa, @olavks, @pforai, @kcgthb, @geimer)