Skip to content

skip dependencies marked as external modules when packaging#1550

Merged
boegel merged 1 commit intoeasybuilders:developfrom
boegel:pkg_skip_external_deps
Jan 16, 2016
Merged

skip dependencies marked as external modules when packaging#1550
boegel merged 1 commit intoeasybuilders:developfrom
boegel:pkg_skip_external_deps

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Jan 16, 2016

fix for issue reported in easybuilders/easybuild#188

@rjeschmi: please review?

@boegel boegel added this to the v2.6.0 milestone Jan 16, 2016
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Jan 16, 2016

original report by @DirkdeDraak:

When an EasyConfig file has an EXTERNAL MODULE as a dependency, it fails to package.
The reason is that the ec object has no toolchain set (None) and thus cannot retrieve the toolchain name.
The error occurs in easybuild/tools/module_naming_scheme/utilities.py in the det_full_ec_version function on:

    # determine main install version based on toolchain
    if ec['toolchain']['name'] == DUMMY_TOOLCHAIN_NAME:
        ecver = ec['version']

Here's a possible fix:

--- easybuild/tools/module_naming_scheme/utilities.py.orig      2016-01-13 16:23:57.962731609 +0100
+++ easybuild/tools/module_naming_scheme/utilities.py   2016-01-14 09:27:28.245027212 +0100
@@ -53,6 +53,9 @@

     ecver = None

+    # EXTERNAL MODULES don't have toolchain set
+    if ec.get('external_module') == True and ec['toolchain'] == None:
+        return None
     # determine main install version based on toolchain
     if ec['toolchain']['name'] == DUMMY_TOOLCHAIN_NAME:
         ecver = ec['version']

@boegel boegel mentioned this pull request Jan 16, 2016
Closed
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Jan 16, 2016

@DirkdeDraak: thanks for reporting this

I don't think your proposed suggestion is the best one long-term, however. With your approach, you'll end up with argument to fpm like --depends "some/external/module-None".
I'm not sure that even works, since the dependency can never be resolved?

My proposed change in this PR just skips dependencies that are marked as external modules, which seems more appropriate. Note that this does not affect the generated module which is included in the package, the module load statements for the external modules will still be there.

rjeschmi
rjeschmi reviewed Jan 16, 2016
View reviewed changes
Comment thread easybuild/tools/package/utilities.py
Copy link
Copy Markdown
Contributor

@rjeschmi rjeschmi Jan 16, 2016

Choose a reason for hiding this comment

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

is get('external_module', default=False) also valid? I find this a bit unintuitive, but understand it now.

Copy link
Copy Markdown
Member Author

@boegel boegel Jan 16, 2016

Choose a reason for hiding this comment

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

No, see:

>> help({}.get)
Help on built-in function get:

get(...)
    D.get(k[,d]) -> D[k] if k in D, else d.  d defaults to None.

>>> {}.get('external_module', False)
False
>>> {}.get('external_module', default=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: get() takes no keyword arguments

I'm using this instead of dep['external_module'] because the external_module key is not defined for the toolchain (yet). And it's safer, anyway.

Copy link
Copy Markdown
Contributor

@rjeschmi rjeschmi Jan 16, 2016

Choose a reason for hiding this comment

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

Yeah that's too bad. Anyway, it looks good. Thanks for fixing this.

I'll think about test cases at some point.

On Sat, Jan 16, 2016, 9:24 AM Kenneth Hoste [email protected]
wrote:

In easybuild/tools/package/utilities.py
#1550 (comment)
:

@@ -106,9 +106,12 @@ def package_with_fpm(easyblock):
pprint.pformat([easyblock.toolchain.as_dict()] + easyblock.cfg.dependencies()))
depstring = ''
for dep in deps:

  •    _log.debug("The dep added looks like %s ", dep)

  •    dep_pkgname = package_naming_scheme.name(dep)

  •    depstring += " --depends %s" % quote_str(dep_pkgname)

  •    if dep.get('external_module', False):

No, see:

help({}.get)
Help on built-in function get:

get(...)
D.get(k[,d]) -> D[k] if k in D, else d. d defaults to None.

{}.get('external_module', False)
False
{}.get('external_module', default=False)
Traceback (most recent call last):
File "", line 1, in
TypeError: get() takes no keyword arguments

I'm using this instead of dep['external_module'] because the
external_module key is not defined for the toolchain (yet). And it's
safer, anyway.

—
Reply to this email directly or view it on GitHub
https://github.com/hpcugent/easybuild-framework/pull/1550/files#r49932352
.

@rjeschmi
Copy link
Copy Markdown
Contributor

rjeschmi commented Jan 16, 2016

Looks good

@hpcugentbot
Copy link
Copy Markdown

hpcugentbot commented Jan 16, 2016

EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2509/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Jan 16, 2016

Jenkins: test this please

@hpcugentbot
Copy link
Copy Markdown

hpcugentbot commented Jan 16, 2016

EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/2513/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Jan 16, 2016

Thanks for the review @rjeschmi!

boegel added a commit that referenced this pull request Jan 16, 2016
@boegel
Merge pull request #1550 from boegel/pkg_skip_external_deps
f8436ba 
skip dependencies marked as external modules when packaging
@boegel boegel merged commit f8436ba into easybuilders:develop Jan 16, 2016
@boegel boegel deleted the pkg_skip_external_deps branch January 16, 2016 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v2.6.0

Development

Successfully merging this pull request may close these issues.

3 participants

@boegel @rjeschmi @hpcugentbot