add support for --hide-all-deps
Change proposed in this feature request: #2543
This is my first contribution to opensource software(don't know if I did everything right)
I have just updated the code from PR #2544 and added a test case
@thiagoabdo Thank you very much for your contribution!
We're honored that you picked EasyBuild as the first open source project to contribute to, thank you very much!
One concern that was raised in #2544 is that --hide-all-deps will reinstall dependencies that are already installed non-hidden, any thoughts on this?
Other than that, I don't see any reason to hold this back, it's complete w.r.t. implementation since it also includes a dedicated test...
cc @casparvl, @ocaisa
Right now, I have not touched the part of handling dependencies, so it is going to be handled as if the user just asked to hide all dependencies. I think that it should be changed, but the problem is how?
1 - Should we add a modifier to this option that allows unhiden dependencies? 2 - Or should we change how eb handles a dependencie that is already installed?
I believe that we should go with 2, and when we do the check for if a module is already installed we just check for hidden and hidden and take actions accordingly
We could when installing a easybuild as non hidden just install the hidden version as well, but that seems a little hacky.
I could change the 'skip_available' in easybuild/framework/easyconfig/tools.py to check if the non hidden version exists if it is installing a hidden version or always do the check based in a flag.
But if I think that if I just skip it there, they module files will have the hidden values but they were not installed. Maybe I should add another check before writing the modules.
We do not have a method 'is_instaled'/'is_available', right?
We do not have a method 'is_instaled'/'is_available', right?
You mean for modules? Of course we do, see the exist method in ModulesTool.
Maybe @casparvl has some thoughts on how to proceed with this...
Hm, I'm not sure on how to implement it, but I do have an opinion on what the default behaviour should be...
There are two scenarios when A is a dependency of B
- Module
Ais already present and you're trying to install with--hide-deps=Aor--hide-all-deps - Hidden module
.Ais already present, and you're trying to install without--hide-deps=A/--hide-all-deps.
In the first case, I would say
-
--hide-deps=Aand--hide-all-depsshould simply be ignored (but make sure the logs clearly show this, and explain why!) - The modulefile of
Bshould simply referenceA(not.A) The only scenario where this default behaviour might be unwanted is when you want to install with hidden dependencies and later on remove the non-hidden module for some reason. To me, that is the exception.
Though I'm no expert in the ModulesTool, the exist function sounds like the way to go if you want to implement this default behaviour.
In the second case, I think
- the module SHOULD be duplicated or linked as non-hidden. I don't know which should be preferred, duplication or linking... I guess I'd prefer linking since otherwise you could potentially at some point have two different versions between the hidden and non-hidden modulefiles, but only one software installation behind it.
- the software should NOT be reinstalled (you'd also not reinstall in any other scenario where the modulefile is already present)
- the non-hidden modulefile should remain untouched
I'm a bit in doubt whether all this should be part of this PR. On the one hand, I'd argue that hide-deps has always had the problematic behaviour that dependencies would be reinstalled if a non-hidden module was already present and that this is therefore a separate issue. On the other hand: a hide-all-deps would make the issue way more prominent, and way more important to fix...
Maybe the best solution is to fix the reinstalling-dependency issue first? And only when that is merged, update this PR? Then it can still be two separate PRs, which is typically simpler to handle / test. @boegel do you agree? @thiagoabdo Would you be willing and able to make a PR that creates the desired for the existing hide-deps functionality first?