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

Make module-extensions true by default #4501

Draft
wants to merge 3 commits into
base: 5.0.x
Choose a base branch
from

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Apr 4, 2024

fixes #4396

@Micket Micket added change EasyBuild-5.0 EasyBuild 5.0 labels Apr 4, 2024
@Micket Micket added this to the 5.0 milestone Apr 4, 2024
@Micket
Copy link
Contributor Author

Micket commented Apr 5, 2024

I'm very confused, out of the type of tests i expected to see failing, the one that actually broke is test_exts_list, the one the test is supposed to already be using this feature to see the list of extensions?? And It fails because it is missing homepage field.

ERROR: test_exts_list (test.framework.easyconfig.EasyConfigTest)
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/test/framework/easyconfig.py", line 523, in test_exts_list
    modfile = os.path.join(eb.make_module_step(), 'PI', '3.14' + eb.module_generator.MODULE_FILE_EXTENSION)
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/easybuild/framework/easyblock.py", line 3778, in make_module_step
    txt += self.make_module_description()
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/easybuild/framework/easyblock.py", line 1376, in make_module_description
    return self.module_generator.get_description()
  File "/tmp/runner/d4b8e7c4de51b3c7733badf96aac3b5c5e83313c/lib/python3.6/site-packages/easybuild/tools/module_generator.py", line 1302, in get_description
    'homepage': self.app.cfg['homepage'],

I can only assume this test was never actually running properly then? Confusing.

@Micket
Copy link
Contributor Author

Micket commented Apr 6, 2024

oh, it's just the test that is super misleading, issue has nothing to do with homepage. It was just due to the partial template resolving code that get_description attempts for some reason.

I think it started from needlessly using template to put in the whatis section, and then that template added name, version, homepage and a bunch of other common things since they happened to be there. But this code shouldn't be doing this, it should leave templates alone. It certainly isn't necesssary to add the whatis section, and this change to just write the lines in order directly is shorter, simpler.

Old method needlessly involved template resolutions,
duplicating the template resolving code, but only partially
and would break if anything but the 4 most common templates were used.
@Micket
Copy link
Contributor Author

Micket commented Apr 8, 2024

I'm struggling with the test suite, because of the already existing bug that breaks --module-extensions=True.

  1. The test test_exts_list is implying that this
    ("ext-%(namelower)s", "%(version_major)s.0"),
    should be valid.
  2. The get_description (and some other parts of module_generator) randomly decided to resolve 3 template strings (name, version, installdir)
    'name': self.app.name,
    'version': self.app.version,
    'whatis_lines': '\n'.join(whatis_lines),
    'installdir': self.app.installdir,
    and nothing else.
  3. If one actually runs the suggested easyconfig, using version_major, it does not get expanded by make_extension_string
    exts_str = self.app.make_extension_string(name_version_sep='/', ext_sep=',')
    so, the unresolved version_major is there and EasyBuild will crash. Note: This is already present in easybuild, regardless of this PR.
  4. Given that %(name) and %(version) is supposed to be resolved to refer to the extension itself, it's a bit confusing to also use it as the name and version for an extension in my opinion. I understand the difference of if it's used for 2 first tuple fields of an extension, I still think it's confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant