Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tcl module fixes #739

Merged
merged 29 commits into from
Oct 31, 2013
Merged

Tcl module fixes #739

merged 29 commits into from
Oct 31, 2013

Conversation

boegel
Copy link
Member

@boegel boegel commented Oct 26, 2013

Various fixes w.r.t. modules.py and its unit tests.

This should fix #733 and #734.

Currently still a WIP, since some tests still fail in certain circumstances, see https://jenkins1.ugent.be/view/EasyBuild%20(boegel)/

- run 'module use' on all module paths after changing $MODULEPATH
- also set $LD_LIBRARY_PATH_modshare when (re)setting $LD_LIBRARY_PATH
- also tweak stdout for purge/use module commands
- deprecate passing of list of module paths to run_module; not used, not needed, only complicates things w.r.t. 'module use' after touching $MODULEPATH
@boegel
Copy link
Member Author

boegel commented Oct 29, 2013

Should be fully ready now, with all unit tests successful with all module tools we test (see https://jenkins1.ugent.be/view/EasyBuild%20(boegel)/, should be all blue).

@boegel
Copy link
Member Author

boegel commented Oct 30, 2013

@stdweird: Some more issues popped up with the DEISA modulecmd.tcl, fixed now, all my tests are blue (see https://jenkins1.ugent.be/view/EasyBuild%20(boegel)/).
Ready for review...

#
'available': re.compile(r"""
^(?!-*\s) # disallow lines starting with (empty) list of '-'s followed by a space
\s* # ignore whitespace at start of the line
Copy link
Contributor

Choose a reason for hiding this comment

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

change comment, this line has nothing to do with start of line

Copy link
Member Author

Choose a reason for hiding this comment

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

well, yes, it does, since the previous line is a lookahead...

Copy link
Contributor

Choose a reason for hiding this comment

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

no, per the previous statement, the line can't start with a whitespace character

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmk, this is not what I intended here...

It's OK, because none of the modules tools we currently support include whitespace before module names with --terse, but it's more strict than I would like it to be.
I'll remove this part of the regex all together, since it's senseless.

mods = super(EnvironmentModulesTcl, self).available(mod_name=mod_name)
# strip off slash at beginning, if it's there
# under certain circumstances, modulecmd.tcl (DEISA variant) spits out available modules like this
clean_mods = []
Copy link
Contributor

Choose a reason for hiding this comment

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

you mean clean_mods = [mod.lstrip(os.path.sep) for mod in mods]

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I should have realized that, fixed

@stdweird
Copy link
Contributor

@boegel reviewed


def which(cmd):
"""Return (first) path in $PATH for specified command, or None if command is not found."""
paths = os.environ.get('PATH', '').split(os.pathsep)
Copy link
Contributor

Choose a reason for hiding this comment

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

i would throw an error of some sort in case PATH is not available. won't find anything anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

The error message will show that no paths were considered ([]), isn't that sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

Copy link
Member Author

Choose a reason for hiding this comment

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

Enhanced the error message, it now clearly mentions $PATH

@stdweird
Copy link
Contributor

@boegel feel free to merge this in, we can chekc the shell issue later

boegel added a commit that referenced this pull request Oct 31, 2013
@boegel boegel merged commit bb647d5 into easybuilders:develop Oct 31, 2013
@boegel boegel deleted the tcl_module_fixes branch November 5, 2013 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants