Skip to content

support adding specific paths to $PATH for generic Binary easyblock via 'prepend_to_path' custom easyconfig parameter #1426

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.

Sign up for GitHub

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

Merged
boegel merged 4 commits into from
May 22, 2018
Merged

support adding specific paths to $PATH for generic Binary easyblock via 'prepend_to_path' custom easyconfig parameter #1426

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e1de43
Add option to add specific path to PATH
Flamefire May 17, 2018
7e98049
Make prepend_to_path a list
Flamefire May 22, 2018
364254e
Use self.cfg.get to avoid KeyError
Flamefire May 22, 2018
e04ed0f
Use self.cfg.get for other keys as well
Flamefire May 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions easybuild/easyblocks/generic/binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ def extra_options(extra_vars=None):
'install_cmd': [None, "Install command to be used.", CUSTOM],
# staged installation can help with the hard (potentially faulty) check on available disk space
'staged_install': [False, "Perform staged installation via subdirectory of build directory", CUSTOM],
'prepend_to_path': [[''], "Prepend the given directories (relative to install-dir) to the environment "
"variable PATH in the module file.", CUSTOM],
})
return extra_vars

Expand Down Expand Up @@ -131,9 +133,10 @@ def sanity_check_rpath(self):
self.__class__.__name__)

def make_module_extra(self):
"""Add the install directory to the PATH."""
"""Add the specified directories to the PATH."""

txt = super(Binary, self).make_module_extra()
txt += self.module_generator.prepend_paths("PATH", [''])
if self.cfg['prepend_to_path'] is not None:
txt += self.module_generator.prepend_paths("PATH", self.cfg['prepend_to_path'])
Copy link
Member

@boegel boegel May 22, 2018

Choose a reason for hiding this comment

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

@Flamefire I think it's a good idea to be a bit more careful here w.r.t. accessing the custom prepend_to_path easyconfig parameter. Easyblocks that derive from Binary may not pick up the custom easyconfig parameters defined for Binary (because they don't properly define the extra_options static method, or because they don't care), which would result in self.cfg['prepend_to_path'] failing hard with KeyError...

We can be robust against this by using self.cfg.get('prepend_to_path'), but to fully retain backward compatibility we also need to make sure [''] is used as a default value, so we should introduce a constant to avoid hardcoding [''] in multiple places:

PREPEND_TO_PATH_DEFAULT = ['']
...
    def make_module_extra(self):
        ...
        prepend_to_path = self.cfg.get('prepend_to_path', PREPEND_TO_PATH_DEFAULT)
        if prepend_to_path:
            txt += self.module_generator.prepend_paths("PATH", prepend_to_path)

Copy link
Contributor Author

@Flamefire Flamefire May 22, 2018

Choose a reason for hiding this comment

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

Sure. I was just following existing usages. Example: if self.cfg['install_cmd'] is None:

Shall I change those usages too while I'm on it?

Copy link
Member

@boegel boegel May 22, 2018

Choose a reason for hiding this comment

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

@Flamefire Makes sense, but better do that in a separate commit?

Copy link
Member

@boegel boegel May 22, 2018

Choose a reason for hiding this comment

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

Those actually show that this may be overkill (if there would be a problem with easyblock deriving from Binary we would've known already), but it doesn't hurt in the long run to make this a bit more robust...

Copy link
Contributor Author

@Flamefire Flamefire May 22, 2018

Choose a reason for hiding this comment

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

Done.

self.log.debug("make_module_extra added this: %s" % txt)
return txt