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

Allow use of custom delimiter for paths in module generator #4687

Open
wants to merge 15 commits into
base: 5.0.x
Choose a base branch
from

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Oct 21, 2024

This is the straight forward option that we definitely should expose.
It would allow any easyblock to use this at least, though in practice, we should also enable some way for easyconfigs to chose this when specifying modextrapaths.

I left that out for this initial PR, since we need to introduce some new syntax. Maybe good to mark that for 5.0.x.

Various ideas have been thrown out:

modextrapaths = {
    'PATH': 'some_path',    # Normal : delimited paths continue to be either a string
    'PATH': ['some_path', 'other_path'],    # or a list of strings
    'FOO': ('foo', ';'), # Using a tuple indicates second argument is delimiter ; in this case
    'FOO': (['foo', 'bar'], ';'), # same with multiple paths, the string is replaced by a list.
    'BAR': {'paths': 'foo', 'delimiter': ';'}, # more explicit option
    'BAR': {'paths': ['foo', 'bar'], 'delimiter': ';'}, # and with multiple strings
}

That's about all the options I can think of. I would be against doing

modextrapaths = {
    'FOO': [('foo', ';'), ('bar', ';')], # not good
}

since it could allow for different separators to be used for the same variable, which doesn't make any sense.

Needs tests.

@jfgrimm
Copy link
Member

jfgrimm commented Oct 21, 2024

personally, I'd be in favour of the more explicit:

    'BAR': {'paths': 'foo', 'delim': ';'}, # more explicit option
    'BAR': {'paths': ['foo', 'bar'], 'delim': ';'}, # and with multiple strings

@Micket
Copy link
Contributor Author

Micket commented Oct 21, 2024

Having looked at the code, we have allowed tuples to be used instead of lists here, so might be best to opt for the dict for that reason alone.

I added support for that now be rewriting the necessary parts in modextra.
I refactored the code a bit since i wanted to deduplicate the prepend and append code.

Completely untested so far, I ran out of time.

@jfgrimm
Copy link
Member

jfgrimm commented Oct 25, 2024

FAIL: test_make_module_step (test.framework.easyblock.EasyBlockTest)
Test the make_module_step
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/runner/1f60854bac9a4cfba15aca0717bfcfe964c33e1f/lib/python3.6/site-packages/test/framework/easyblock.py", line 1346, in test_make_module_step
    self.assertTrue(regex.search(txt), "Pattern %s found in %s" % (regex.pattern, txt))
AssertionError: None is not true : Pattern ^prepend-path\s+(-d ".")?TCLLIBPATH\s+\$root/paths$ found in #%Module
proc ModulesHelp { } {
    puts stderr {

Description
===========
This \{is a\}\} \[fancy\]\] \[\[description\]\]. \{\{\[\[TEST\}\]


More information
================
 - Homepage: http://example.com/
    }
}

module-whatis {Description: This \{is a\}\} \[fancy\]\] \[\[description\]\]. \{\{\[\[TEST\}\]}
module-whatis {Homepage: http://example.com}/
module-whatis {URL: http://example.com}/

set root /tmp/eb-2q3i9mdp/eb-evnuh1i2/eb-epx33r9k/tmpu4hfzyac/software/pi/3.14

conflict pi

module load GCC/6.4.0-2.28

module load test/.1.2.3

prepend-path	CMAKE_PREFIX_PATH		$root
prepend-path	PATH		$root/bin
setenv	EBROOTPI		"$root"
setenv	EBVERSIONPI		"3.14"
setenv	EBDEVELPI		"$root/easybuild/pi-3.14-easybuild-devel"

setenv	PI		"3.1415"
setenv	FOO		"bar"
pushenv	TEST_PUSHENV		"123"
prepend-path	PATH		$root/xbin
prepend-path	PATH		$root/pibin
prepend-path	CPATH		$root/pi/include
prepend-path -d " "	TCLLIBPATH		$root/pi 
append-path	APPEND_PATH		$root/append_path
# Built with EasyBuild version 5.0.0.dev0

@Micket
Copy link
Contributor Author

Micket commented Oct 25, 2024

had a 2 day meeting that got in the way of finishing this. I hate regexes. The generated file should be OK i think. Lets see if i got it right now

Copy link
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

Looks good, I only have some comments to simplify the code.

easybuild/tools/module_generator.py Outdated Show resolved Hide resolved
easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Nice-to-have
Development

Successfully merging this pull request may close these issues.

4 participants