Navigation Menu

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

finish custom module naming scheme support #879

Merged

Conversation

boegel
Copy link
Member

@boegel boegel commented Mar 6, 2014

this fixes #687, and is a big step towards fixing #862

@boegel boegel added this to the v1.12 milestone Mar 6, 2014
@stdweird
Copy link
Contributor

stdweird commented Mar 9, 2014

@boegel merge fails?

@@ -238,6 +238,39 @@ def retrieve_blocks_in_spec(spec, only_blocks, silent=False):
return [spec]


def det_full_module_name(ec, eb_ns=False, build_options=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

tools.py is not the garbage bin for all utility functions. at least easybuild.tools.module_naming_scheme is a better namespace since it's related.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is framework/easyconfig/tools.py, not filetools.py...

moving it to module_naming_scheme is not an option, as this det_full_module_name requires process_easyconfig...

@stdweird
Copy link
Contributor

stdweird commented Mar 9, 2014

it's clear to me that the old dep tuple needs more extension, well, this is not the way to go. maybe a wrapper around tuple can give you backawards compatability to the regular tuple and allow to add more data via custom attributes?

@boegel
Copy link
Member Author

boegel commented Mar 31, 2014

@stdweird: are you saying I should look into implementing a Dependency class that represents dependencies, and then support mod_name in that instead?

@stdweird
Copy link
Contributor

@boegel yes, some wrapper around a 2 element tuple should do it.

@@ -775,7 +776,7 @@ def make_module_dep(self):

# Load dependencies
builddeps = self.cfg.builddependencies()
for dep in self.toolchain.dependencies:
for (dep, mod_name) in self.toolchain.dependencies:
if not dep in builddeps:
dep_mod_name = det_full_module_name(dep)
Copy link
Member Author

Choose a reason for hiding this comment

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

stupid code is stupid: we're getting the module name via mod_name, and we then re-determine the module name with another call to det_full_module_name... >_<

…ndency, reorganise modules in easyconfig package to make that possible
@boegel
Copy link
Member Author

boegel commented Apr 1, 2014

@stdweird: remark fixed as discussed, which required some moving around of stuff in easybuild.framework.easyconfig again I'm afraid... I added a tweak.py module to avoid makes the individual modules too big (< 1K characters).

Please rereview?

"""
# convert tuple to string otherwise python might complain about the formatting
self.log.debug("Parsing %s as a dependency" % str(dep))

attr = ['name', 'version', 'versionsuffix', 'toolchain']
dependency = {
'dummy': False,
'name': '',
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment what the difference is between name and mod_name

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

@stdweird
Copy link
Contributor

stdweird commented Apr 1, 2014

@boegel looks much better. should have followed your advise and stayed away from tweak.py ;)

@boegel
Copy link
Member Author

boegel commented Apr 1, 2014

Merging in this beast, thanks for the review @stdweird!

boegel added a commit that referenced this pull request Apr 1, 2014
…e_support

finish custom module naming scheme support
@boegel boegel merged commit f858dfa into easybuilders:develop Apr 1, 2014
@boegel boegel deleted the finish_custom_module_naming_scheme_support branch April 1, 2014 13:26
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.

None yet

2 participants