Skip to content

Added functionality to skip devel module with naming scheme#2374

Merged
boegel merged 3 commits intoeasybuilders:developfrom
sara-nl:skip_devel_module
Feb 22, 2018
Merged

Added functionality to skip devel module with naming scheme#2374
boegel merged 3 commits intoeasybuilders:developfrom
sara-nl:skip_devel_module

Conversation

@jdonners
Copy link
Copy Markdown

So far, we have installed all modules with the default naming scheme. I would like to install a separate set of modules with a lowercase naming scheme (I know about the caveats of using such a scheme). This change allows to install an alternative set of modules that points to the same installation directory, without overwriting the devel module. I also attached the lowercase naming scheme that I used (I had to rename it from .py to .txt).
lowercase_easybuild_mns.txt

@jdonners
Copy link
Copy Markdown
Author

the failed test looks like a network hiccup..

@boegel
Copy link
Copy Markdown
Member

boegel commented Jan 12, 2018

@jdonners It most likely is, I've re-triggered the test.

Comment thread easybuild/framework/easyblock.py Outdated

if not ActiveMNS().mns.det_generate_devel_module():
self.log.info("Skipping devel module...")
return
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we try to avoid inline return statements across the codebase (doesn't help with readability).

To avoid changing indentation in this whole method, why not change the condition that guards the (only) call to make_devel_module instead to include and ActiveMNS().mns.det_generate_devel_module()?


def det_generate_devel_module(self):
"""
Determine if a devel module should be generated. Can be used to create a separate set of modules with a different naming scheme.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (> 120 chars), easy/sensible fix is to move 2nd sentence to new line

@jdonners
Copy link
Copy Markdown
Author

should I create another wrapper function for det_generate_devel_module in ActiveMNS()? I'm not sure what it's meant for..

@jdonners
Copy link
Copy Markdown
Author

and I could just rename the function to det_make_devel_module, so it matches the routine it is meant for.

@jdonners
Copy link
Copy Markdown
Author

Hi @boegel, is there something else you'd like to see changed about this pull request?

Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 20, 2018

@jdonners Tests in Travis fail on version check for bootstrap script, which is clearly not related to the changes you made.

Can you sync your branch with current develop? That should fix that issue...

@boegel boegel added this to the next release (3.5.2 or 3.6.0) milestone Feb 22, 2018
@boegel
Copy link
Copy Markdown
Member

boegel commented Feb 22, 2018

Looks good, thanks @jdonners!

@boegel boegel merged commit 0f03324 into easybuilders:develop Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants