This works like a charm :-)

lgtm

This certainly a more elegant solution! lgtm2

Thanks for the feedback everyone, going in to be included in EasyBuild v3.1.1.

I did not check properly... This fix is good for the compiler-only toolchain case where the toolchain used equals the MODULEPATH extender (in fact my hook for ec in expand_toolchain_load(ec=self.cfg) was another way to fix this because our module naming scheme uses that to check for compiler-only toolchains, but your fix is much better than this).

However the original problem fixed in #2091 is still there.
I can reproduce in the same way as in #2091 with HPL-2.1-intel-2016b.eb. It now includes again these two superfluous loads:

load("GCCcore/5.4.0")
 
load("binutils/2.26")

How does that work: HPL using intel-2016b expands the dependencies from intel-2016b.eb:

dependencies = [
    ('GCCcore', gccver),
    ('binutils', binutilsver, '-GCCcore-%s' % gccver),
    ('icc', compver, gccsuff),
    ('ifort', compver, gccsuff),
    ('impi', '5.1.3.181', '', ('iccifort', '%s%s' % (compver, gccsuff))),
    ('imkl', '11.3.3.210', '', ('iimpi', version)),
]

The modules icc, ifort, and impi are eliminated since they expand $MODULEPATH.
However, GCCcore and binutils, which are dependencies of icc and ifort are no longer eliminated.

@bartoldeman Hmm, ok, I indeed overlooked that... And since I also changed to test case, the tests didn't complain either.

Can you open a new PR to current develop, basically re-adding the changes you had in #2091?

Do also include test case from #2091 in the existing test_make_module_dep_hmns test, so we retain both test cases (i.e. the one from #2091 and the one I changed it to in this PR).

Do make sure that the original test from #2091 highlights the reintroduced bug; if not, we need a better test case.

I am a little worried about reintroducing #2091 because that also means adding #2135 again.
For the intel/2016b this issue can be avoided by simply stripping down intel-2016b.eb:

  dependencies = [
-   ('GCCcore', gccver),
-   ('binutils', binutilsver, '-GCCcore-%s' % gccver),
     ('icc', compver, gccsuff),
    ('ifort', compver, gccsuff),
    ('impi', '5.1.3.181', '', ('iccifort', '%s%s' % (compver, gccsuff))),
    ('imkl', '11.3.3.210', '', ('iimpi', version)),
]

since icc and ifort already depend on those. Now in my case I have a different issue since in our setup it's iccifort that expands $MODULEPATH, instead of icc and ifort.

However if I rewrite deps in intel-2016b.eb as follows:

dependencies = [
    ('iccifort', compver, gccsuff),
    ('impi', '5.1.3.181', '', ('iccifort', '%s%s' % (compver, gccsuff))),
    ('imkl', '11.3.3.210', '', ('iimpi', version)),
]

then I trigger an exception in _verify_toolchain(self) in easybuild/tools/toolchain/toolchain.py.

I think I will try to experiment with increasing depth in the

self.toolchain_dep_mods = dependencies_for(mod_name, self.modules_tool, depth=0)

call in toolchain.py since that way the icc and ifort dependencies are found (because they are deps of iccifort).

I am a little worried about reintroducing #2091 because that also means adding #2135 again.

No, it doesn't, #2135 was a workaround for modules that include load GCCcore while they shouldn't, while #2091 avoids inserting the load GCCcore statements in the first place.

So, as long as your modules have been regenerating with the fix included in #2091, you don't need to avoid reloading modules anymore like we do in #2135 (other than maybe to optimise things, but you'll gain very little from it).

fix was (re)implemented by @bartoldeman in #2152