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

[Flamefire]

Also allows to not add a path at all or set it to a specific one.
See e.g. #801 for problems caused by current behavior.

Basically: This EasyBlock is for binary packages or binary installers. So assuming the User/EasyConfig meant to add installdir to PATH is questionable at least as we do know nothing about the package to be installed.

I thought about making it a simple True/False switch but I think this is more flexible: You can either be fine with installdir being added (kept as default for backwards compatibility), change it to a custom binary path or disable it altogether.

[houndci-bot]

line too long (130 > 120 characters)

[boegel]

Please clarify that the specified path is relative to the installation prefix? Also, putting each part on a separate line is overkill?

'prepend_to_path': ['', "Prepend given directory (relative to installation prefix) to $PATH (use None to skip)",
                    CUSTOM],

[boegel]

Also, maybe we should make the default value [''] (and adjust accordingly below), to allow for adding multiple location to $PATH?

And then you don't need to use None as a magic value, an empty list will do too to skip adding anything to $PATH...

[Flamefire]

The list idea is good, I'm just not sure if it is helpful. Can there be any situation where you would want to add multiple paths to PATH? Usually it is "bin" and this already lets you add one additional path. Should be enough but I'll change that if you like it better. And I don't consider None to be a magic value. IMO it is very descriptive.

About the separate lines: The Code-review bot was complaining about line length. If I add the relative part then even this would not fit. Where and how shall I split it? I could not find convention.

[boegel]

I admit that having to add multiple locations to $PATH is rare, but it does happen (e.g. both bin & scripts). You are right that not being able to provide multiple locations via prepend_to_path is not a big issue though, you can always get additional locations included via modextrapaths = {'PATH': [...]} in the easyconfig file.
I was only referring to None as magical value since an empty string '' can't be used to skip adding a location to $PATH, and with a list this is no longer necessary (since an empty list [] also does that).
With that in mind, let's indeed keep the default value as is ('').

W.r.t. line length:

'prepend_to_path': ['', "Prepend given directory (relative to installation prefix) to $PATH"
                    " (use None to skip)", CUSTOM],

(not using a , after the first line results in the two strings to be concatenated for the help message)

[boegel]

@Flamefire Please use if ... is not None, see https://docs.python.org/2/reference/expressions.html#not-in

[boegel]

@Flamefire Any idea if you'll be able to make the (trivial) requested changes soon? It would be nice to squeeze this into EasyBuild v3.6.1, which I'd like to get released this week...

[houndci-bot]

continuation line under-indented for visual indent

[Flamefire]

I was just doing them :)

[boegel]

@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)

[Flamefire]

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?

[boegel]

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

[boegel]

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...

[Flamefire]

Done.

[boegel]

Reviewed & tested this with a couple of easyconfigs using Binary as well as with easyblocks that derive from Binary, good to go, thanks a lot for tackling this @Flamefire!