fix conflict specification included in module files by boegel · Pull Request #1017 · easybuilders/easybuild-framework

boegel · 2014-08-22T12:13:20Z

fix conflict specification included in module files (see #994), base it on short module name obtained from active module naming scheme

this forced me to resort to another solution for verifying toolchain definitions vs the loaded toolchain module, since this basically means we can no longer obtain the (isolated) software name from a given module file (keeping in mind that a custom module naming scheme may be embedded a software name in some weird way)

the inner function tc_elem_present now using a regex to check whether a module name could be for a software package with the specified name, which is the best I can come up with

this approach should work most of the time, but it possible to come with a (weird) module naming scheme that breaks the assumptions made in tc_elem_present...

… module name obtain from active module naming scheme

…in definition vs loaded toolchain module differently

boegel · 2014-08-22T12:13:51Z

@wpoely86: please review?
cc: @ikirker, @pforai

pforai · 2014-08-23T19:26:53Z

Looks very good to me. I like the work here. Good improvement getting rid of TC sanity checking via conflict stanza.

@boegel Could you just give an example of the limitations imposed by custom MNS and tc_elem_present() as you did on IRC? Should be left here for reference maybe.

stdweird · 2014-08-26T10:26:36Z

make it a class method? seems useful

ok, see is_dep_in_toolchain_module

boegel · 2014-08-26T22:08:37Z

@stdweird: remarks fixed, please revisit

stdweird · 2014-08-29T06:12:49Z

see my previous remark that got lost: the regexp needs proper termination with (probably) (?:/|$).

it didn't get lost: I replied to it ;-)

This is deliberetaly open-ended, see also the examples mentioned. With your approach, the lib/math/BLACS-stable/1.1 wouldn't match anymore...

hmm, this worse then i thought...
with this code, looking for a with only /aa would return a match, which is clearly wrong (it now assume this is a with suffix a?). so this can't be open ended.

but actually, only the naming scheme can decide here what is possible or not. in principle blacs and BLACS are not the same software. so general case insensitive name search is clearly too open.

each found module should be inspected to see it matches the searched name. (or eg the show of info commands should contain the name of the software you are looking for)

and to make matters worse, since we don't enforce a reverse for the naming scheme, there isn't a straightforward relation between modulename and software name to begin with.
also, nowhere do we assume that the software name is in the module name. the naming scheme has the mapping (eg imagine module name is md5sum of software name).

we don't enforce being able to reverse the naming scheme yet, i.e. go from module name to software name/version and toolchain name/version; not only software name, we will need all four in the end (and even versionsuffix too)

I was also thinking to simply ask the module naming scheme what the software name is for a given module name. But then we need access to ActiveMNS here in toolchain.py, which brings back the cyclic imports issue...

If you agree this is the best approach (adding something like software_name_from_module(mod) to the MNS API), then I can make this work.

One way to avoid making the regex open-ended is to use (?:\W) (i.e. close it with a non-alphanumeric character). That would make things slightly better, but only slightly.

reversability is not mandatory in this case. worst case you inspect all modules to find the ones with matching name 😄
but it would really help if this is delegated to the naming scheme.
then people who do stupid naming scheme can solve the isse (eg when using case insensitive names, the author of the naming scheme can decide how to handle clashes).

what cyclic imports issue was that again? the naming scheme instance can be passed as argument to this function from wherever it is called (or something like that).

the issue is not wheter it is openended or not (because of suffixes). look at the a vs aa example. in this case aa is a candidate (a with suffix a), but need inspection of the module itself. so the naming scheme can return candidates, they still need inspection. (but it's the naming scheme that should return the candidates)

stdweird · 2014-08-29T06:20:19Z

one small bug, 2 non-blocking remarks.

Conflicts: easybuild/framework/easyblock.py easybuild/tools/module_naming_scheme/hierarchical_mns.py easybuild/tools/modules.py test/framework/modules.py

…erify toolchain definition based on module names

stdweird · 2014-09-05T19:29:16Z

any reason why this check is not part of _det_module_name_with?

the check should only be done using a short module name (e.g. OpenMPI/1.6.4), never with a full module name (e.g. Compiler/GCC/4.7.2/OpenMPI/1.6.4), while _det_module_name_with is used for both short/full module names

esp since you are actually doing a sanity check of mns, using self.mns.is_module_for seems only logical

I can use self.mns.is_module_for directly here, but that wouldn't make a difference, ActiveMNS still needs to wrap around self.mns.is_module_for with an own method is_module_for, since that one is called from toolchain.py

boegel added 4 commits August 22, 2014 11:05

fix conflict specification included in module files, should use short…

b56f7c0

… module name obtain from active module naming scheme

add unit test for toolchain definition verification

a567ada

fix test_descr unit test w.r.t. spacing in 'conflict' line

839bed2

get rid of module_software_name method in ModulesTool, verify toolcha…

4a4fee0

…in definition vs loaded toolchain module differently

This was referenced Aug 22, 2014

Generated modules can only conflict on app.name #994

Closed

issues with support for hierarchical module naming schemes in EasyBuild v1.14.0 #1011

Open

stdweird reviewed Aug 26, 2014
View reviewed changes

fix remarks

46833b6

stdweird reviewed Aug 29, 2014
View reviewed changes

boegel added 7 commits August 29, 2014 09:32

fix remark w.r.t. regex for 'module load'

273fbaa

fix small remark w.r.t. log error in HierarchicalMNS

24ac4eb

Merge branch 'develop' into fix_conflicts

0c6158b

Conflicts: easybuild/framework/easyblock.py easybuild/tools/module_naming_scheme/hierarchical_mns.py easybuild/tools/modules.py test/framework/modules.py

fix merge conflict resolution failure

a12675e

Merge branch 'develop' into fix_conflicts

a818369

add is_module_for method to module naming scheme API, and use it to v…

7726971

…erify toolchain definition based on module names

add unit test for is_module_for method, fix test module naming schemes

6b6cb7d

stdweird reviewed Sep 5, 2014
View reviewed changes

Conversation

boegel commented Aug 22, 2014

Uh oh!

boegel commented Aug 22, 2014

Uh oh!

pforai commented Aug 23, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Aug 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stdweird commented Aug 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants