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

respect --suffix-modules-path value for user-specific module path extensions #2250

Merged
merged 7 commits into from
Jun 29, 2017

Conversation

ocaisa
Copy link
Member

@ocaisa ocaisa commented Jun 28, 2017

No description provided.

@ocaisa
Copy link
Member Author

ocaisa commented Jun 28, 2017

@boegel @geimer Fixes the issue mentioned at the end of #1472

@boegel boegel changed the title User-specific module path was not respecting... User-specific module path was not respecting mod_path_suffix Jun 28, 2017
@boegel boegel added this to the 3.3.1 milestone Jun 28, 2017
@boegel
Copy link
Member

boegel commented Jun 28, 2017

@ocaisa Please fix the test accordingly:

======================================================================
FAIL: Test for make_module_extend_modpath
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/248050102/lib/python2.6/site-packages/easybuild_framework-3.3.1.dev0-py2.6.egg/test/framework/easyblock.py", line 221, in test_make_module_extend_modpath
    self.assertTrue(regex.search(txt), "Pattern '%s' found in: %s" % (regex.pattern, txt))
AssertionError: Pattern 'if isDir\(pathJoin\(os.getenv\("HOME"\), "my/own/modules/Compiler/pi/3.14"\)\) then' found in: prepend_path("MODULEPATH", "/tmp/eb-kON31a/eb-Tjg9zA/eb-3rMknL/tmpUJTmLr/modules/all/Compiler/pi/3.14/compiler")
prepend_path("MODULEPATH", "/tmp/eb-kON31a/eb-Tjg9zA/eb-3rMknL/tmpUJTmLr/modules/all/Compiler/pi/3.14/tools")
if isDir(pathJoin(os.getenv("HOME"), "my/own/modules/all/Compiler/pi/3.14")) then
    prepend_path("MODULEPATH", pathJoin(os.getenv("HOME"), "my/own/modules/all/Compiler/pi/3.14"))
end

Also, I'd like to get feedback from @geimer on this, how did this go unnoticed? Are you using this together with $EASYBUILD_SUFFIX_MODULES_PATH='' @geimer?

@ocaisa
Copy link
Member Author

ocaisa commented Jun 28, 2017

I can answer for @geimer, yes he uses EASYBUILD_SUFFIX_MODULES_PATH=''

@geimer
Copy link
Contributor

geimer commented Jun 29, 2017

@boegel Yes, I've always used EASYBUILD_SUFFIX_MODULES_PATH='' as the all suffix never made any sense to me. IMHO, the empty string should actually be the default.

@boegel
Copy link
Member

boegel commented Jun 29, 2017

@geimer Thanks for the confirmation, that explains why this went unnoticed.

Using '' as default for --suffix-modules-path wouldn't work because EasyBuild by default also creates subdirectories for each moduleclass...
Let's consider it a historical mistake, any default will only make sense for some sites.

@ocaisa
Copy link
Member Author

ocaisa commented Jun 29, 2017

Ok, looks like the tests pass now.

@boegel
Copy link
Member

boegel commented Jun 29, 2017

The changes look good, but it raises a question...

Say EasyBuild is configured with --suffix-modules-path=all for the centrally installed modules, but the user configures EasyBuild differently, for example with --suffix-modules-path='' like @geimer is doing.

Should that be a concern? If so, than we need a separate --suffix-modules-path-user?

@geimer
Copy link
Contributor

geimer commented Jun 29, 2017

@boegel Better leave this can of worms closed... What if the user prefers to use a flat naming scheme but the system-wide install uses HMNS? You see my point?

@ocaisa
Copy link
Member Author

ocaisa commented Jun 29, 2017

The user does have control over what suffix they use (since it is their own instance of --suffix-modules-path that will create the path) but the system has to chose some default (@geimer chooses '', the EB default is all but the choice is system specific) since it cannot handle the infinite options open to the end user. As long as the sys-admin is the one preparing the module that sets up their EB environment this is not an issue...and even if they are not it is still fixable by the end user by choosing an appropriate --suffix-modules-path

@ocaisa
Copy link
Member Author

ocaisa commented Jun 29, 2017

Tests passing, good to go

@boegel
Copy link
Member

boegel commented Jun 29, 2017

@geimer Well, mixing different types of naming schemes is a whole different matter, since you just can't make that work at all.

@ocaisa I'm mainly worried about changing the default behaviour... In EasyBuild v3.3.0, you would never get a suffix for the module path, now you get 'all' by default.

I think others besides @geimer are using this too (do you happen to know who, @geimer)?

But, since it's a bug fix we can probably get away with clearly mentioning this in the release notes?

@ocaisa
Copy link
Member Author

ocaisa commented Jun 29, 2017

I see where you are coming from, but the previous behaviour forced you to explicitly move away from EasyBuild default behaviour for the user (they must use --suffix-modules-path="" or it doesn't work properly). If the system default is --suffix-modules-path="" (which I know is true for both @geimer and @akesandgren), then this will not change the behaviour for them at all.

@boegel
Copy link
Member

boegel commented Jun 29, 2017

@ocaisa Good point. So, let's merge!

Thanks for the fix @ocaisa, and @geimer for the feedback!

@boegel boegel changed the title User-specific module path was not respecting mod_path_suffix respect --suffix-modules-path value for user-specific module path extensions Jun 29, 2017
@boegel boegel merged commit daa0228 into easybuilders:develop Jun 29, 2017
@ocaisa ocaisa deleted the patch-4 branch September 26, 2018 11:22
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.

3 participants