Skip `module` in the symlinks #511

muffato · 2022-03-10T02:57:17Z

Hiya,

Following the comments that were raised in #510 , I've found away of making the symlinks point at the module.tcl directly, rather than their enclosing directory, without having to hardcode the module directory 💪🏼

As a reminder, the purpose is to make the output of module avail shorter, e.g. from

bwa-mem2/2.2.1--hd03093a_2/module.tcl  cooler/0.8.6--py_0/module.tcl           golang/1.18-rc/module.tcl             
bwa/0.7.17--h7132678_9/module.tcl      cooler/0.8.11--pyh3252c3a_0/module.tcl  samtools/1.15--h3843a85_0/module.tcl

to

bwa-mem2/2.2.1--hd03093a_2  bwa/0.7.17--h7132678_9  cooler/0.8.6--py_0  cooler/0.8.11--pyh3252c3a_0  golang/1.18-rc  samtools/1.15--h3843a85_0

I've never done any Lua, so only changed the Tcl templates. The change is not as trivial as I was expecting because:

file normalise doesn't normalise the filename, only the directories
file readlink raises an error if called on a direct file

So, I had to use file type to identify the symlinks, and then call file readlink.

If people find this change useful, and agree with the implementation method, the Lua code would still have to be written.

I'm aware that @marcodelapierre you said you were busy. no worries !

Matthieu

…ymlink

muffato · 2022-03-18T02:13:29Z

In the end, I'm simply skipping this functionality for Lmod because:

It breaks default_version=false
We said in Minor refactoring of check_symlink #510 (comment) that modules had to be portable, and couldn't include hard-coded paths (except the container itself). The module (and its symlink) must be able to resolve bin/ (for the wrapper scripts) under the real path. In Lmod, LUA scripts are evaluated in a sandbox that has access to a limited number of functions, cf https://github.com/TACC/Lmod/blob/master/src/sandbox.lua#L66-L260. Notably, the list excludes io.popen, which is typically used to resolve symlinks.

In summary, this PR removes module.tcl from the TCL symlinks, and leaves the LUA symlinks as they were in @vsoch 's original implementation

This is done by symlinking `<software>/<version>` itself to `<namespace>/<software>/<version>/module.tcl`. For the directory of the wrapper scripts to be correctly found, the symlink has to be resolved, but TCL's `file normalize` won't normalise the filename. So, we need to use `file readlink` instead, but only on real symlinks because it raises an error.

which is when default_version==True (default_version==False can't be made to work with symlinks).

muffato · 2022-03-20T01:00:29Z

Not sure how you want to deal with that, @vsoch

This is the branch I'm using locally for all my tests. I've merged the latest main and #517 because they both impact / conflict with the implementation of symlinks.

For review here, there are two main commits: f469cb7 and c671723; and 2ea12ac

The simplified symlink tree is now available for Lmod too, if default_version==True.

vsoch · 2022-03-20T01:41:26Z

Deal with what @muffato ?

muffato · 2022-03-20T01:54:33Z

With the review. The two merge commits make the diff harder to review

Signed-off-by: vsoch <[email protected]>

vsoch · 2022-03-20T03:01:27Z

oh I see! So I think I just need to rebase that older PR with main (as you have done) so they don't show up as new.

vsoch · 2022-03-20T03:05:46Z

okay @muffato I think I was able to update both PRs so the number of files changed is now 9 and not 16! Can you take a quick look and make sure I resolved the merge conflicts correctly?

muffato · 2022-03-20T03:37:56Z

All good 👍🏼

muffato · 2022-03-26T12:36:39Z

Added two commits:

no need to abort the installation if the user doesn't want the symlink to be overwritten
added a command-line option (--force) to force overwriting the symlink

shpc/main/modules/__init__.py

…ing symlinks The name `--force` is generic, so that other things could be forced through it, not just overwriting symlinks. Also added an info message if a symlink is overwritten, which can be hidden with the `--quiet` flag.

Co-authored-by: Vanessasaurus <[email protected]>

`--symlink-tree` now also overrides the config file

vsoch · 2022-03-27T04:00:10Z

shpc/main/modules/__init__.py

@@ -343,7 +357,24 @@ def check(self, module_name):
        config = self._load_container(module_name.rsplit(":", 1)[0])
        return self.container.check(module_name, config)

-    def install(self, name, tag=None, symlink=False, **kwargs):
+    def write_version_file(self, version_dir):


Is this section old from before? I guess we need to merge the version changes before this one!

Yes, because I had done a merge of the tcl_default_version :/
I can probably remove this merge commit to make this PR clearer ?

Yes - let's update this PR with the changes in current main and then I can give another review!

vsoch · 2022-03-27T04:02:49Z

shpc/client/__init__.py

@@ -103,7 +103,21 @@ def get_parser():
    install.add_argument(
        "--symlink-tree",
        dest="symlink",
-        help="install to symlink tree too.",
+        help="install to symlink tree too (overrides settings.yml).",
+        default=None,


Oh this is neat I’ve never seen it before! So if —symlink-tree is set, the None is True and this overrides the default False of no symlink tree (should that be None too)? And then if no symlink tree is True the None is ignored? What happens if both flags are provided?

Perhaps we should use store_const here to make the three cases more clear? https://stackoverflow.com/a/34750557

this overrides the default False of no symlink tree (should that be None too)

It works as it is: None if no option given, and True / False for --symlink-tree / --no-symlink-tree

What happens if both flags are provided?

If both are given, the last one wins

Perhaps we should use store_const here to make the three cases more clear? https://stackoverflow.com/a/34750557

A matter of choice :) I find store_true and store_false very explicit :)

Ok, I’m cool with that :)

will probably finish review tomorrow - I want to look everything over once more with a clear head, and I’m rather tired and still a bit on edge from the day. So stay tuned for tomorrow! I think the default version PR looks great (and can update the PR here) so probably I’ll merge that one first.

goodnight from the inferno! lol

marcodelapierre · 2022-03-28T01:59:43Z

Just a note to remember keeping an eye on #502, which is related to this one.
@muffato and @vsoch, you have the bigger picture I think, from what I can tell the missing bit, with proposed implementation in that other PR, is the short naming for symlinks also for Lmod. So this is quite close to a wrap I reckon!

vsoch · 2022-03-29T06:04:57Z

@muffato just to follow up here - I think we want to see some kind of tweak /rebase so these version changes don't show up here anymore (that are now deprecated in favor of the other PRs that we worked on!) I'm not sure the best way to go about it if it's not trivial to fix, but let me know what you'd suggest.

muffato · 2022-04-01T23:14:15Z

Hiya. I've got on my local checkout a version of this branch, with the latest main merged in, the conflicts solved, and all tested on the HPC. I'd suggest pushing my branch, and changing the target branch of this PR to main. The diff would then essentially show the content of #502 + the present changes for removing module. Then we approve + merge this PR here, and can forget about #502 (actually, it depends how you would do the merge. With an actual merge commit, GitHub would automatically close #502 because it would see the commits following each-other. But I think you tend to a squash + rebase ? In that case, I'm not sure GitHub could find out)

vsoch · 2022-04-01T23:33:37Z

oh gotcha! That does make sense. So I'm good to merge into the symlink branch (which is against main) and we can pick up from there.

@marcodelapierre

* adding testing of a symlink tree install * unset symlink_home * missing module load * adding symlink_tree setting to indicate to always set also adding support for custom config on the fly with -c and for removing symlinks on uninstall * missing command argument * symlink_home -> symlink_tree and --symlink-tree instead of --symlink * forgot to add dest * ensure that we cleanup symlink directory of .version and empty * ensure we dont create symlink version unless tcl and default version is true * do not require symlink_base to be defined we can have a default of $root_dir/symlinks instead * bugfix: without this, create_symlink would only be called based on the CLI argument, not the .yml setting (#509) * verison bump * Minor refactoring of `check_symlink` (#510) * No need to create the symlink base directory here since it will be created by `create_symlink` * Make this part of the code symmetric with self.create_symlink() * Fixed a truncated sentence * running black * Skip `module` in the symlinks (#511) * Implemented "default_version" for TCL * Use write_version_file for the symlink tree too * Skip `module.tcl` in the symlinks This is done by symlinking `<software>/<version>` itself to `<namespace>/<software>/<version>/module.tcl`. For the directory of the wrapper scripts to be correctly found, the symlink has to be resolved, but TCL's `file normalize` won't normalise the filename. So, we need to use `file readlink` instead, but only on real symlinks because it raises an error. * Symlink to module.lua when possible which is when default_version==True (default_version==False can't be made to work with symlinks). * Added a `--force` option to `shpc install` to force overwriting existing symlinks The name `--force` is generic, so that other things could be forced through it, not just overwriting symlinks. Also added an info message if a symlink is overwritten, which can be hidden with the `--quiet` flag. * Made `force` optional * Forgot the variable for substitution * The "delete" command was superseded by "uninstall" in #6 * Added `--no-symlink-tree` to override the config file `--symlink-tree` now also overrides the config file * Make it explicit we are expecting yes or no * add force and symlink arg back in * do not pin black * Completion of the symlink feature (#539) * Removed leftover from an earlier implementation of `default_version` * bugfix: True and False are deprecated (but still valid) * Lmod symlinks can now skip "module" too thanks to @marcodelapierre * Added the missing newline character at the end of the file * bugfix: the return needs to happen after the creation of the symlink * `symlink_base` should allow environment variable expansion, like the other directories * Cannot consider using symlinks without the base defined * The two classes are meant to address exactly this * bugfix: the symlink needs to be cleaned regardless of where the modules are held * rmdir_to_base does the upwards clean-up correctly * No need to complain if symlinks are not enabled in the first place * bugfix: the caller of write_version_file needs to build the directory path All other calls were already doing it, this was the only exception. The change is required because not all .version files are in $module_base * bugfix: .version needs to be updated in the symlink tree too * bugfix: when uninstalling the last version of a tool there is no directory any more * Make the settings object upgrade legacy values so that the code only needs to know about the current ones * Expanded the function to deal with files and symlinks * Adding we can just use a filesystem loader and then from_string instead * Message update cf #502 (comment) * saving start of work on views this adds an shpc view command and refactors the symlink nomenclature to instead b about views! I need to write documentation and make a few additional commands to load a view from file and write tests for views too. * adding changes to docs and tests for views! * spelling mistake and missing tests helpers file * force needs to be specific for uninstall * remove deprecated symlink-tree * adding support to install from a file and list installed modules * legacy values is broken - remove for now * fixing bug with module version files - cannot do any funny business with creating module class on the fly! * module .version in symlink only should exist for lmod * fixing bug with uninstall and adding shpc view list * adding support to generate a view module a user can now do shpc view add <var> <val> to add customizations to a view! In practice this means that installed modules need to make an attempt to load the view and given that it is a symbolic link, it should find the file (at least we hope). This means in practice one view should be loaded at once, otherwise you will have two conflicting .view_module. * ensure we only use try-load for modules >= 4.8. I am also updating the testing to run for both an older and newer version (with and without support) * fixing bug with test I had changed the client variable to be module_sys instead of module to match the rest of the library, and forgot to update it in the tests helper script. * adding support for shpc view remove <name> <var> <value> and I am also adding better testing to look at both the config and file content of view_module in both cases Signed-off-by: vsoch <[email protected]> Co-authored-by: vsoch <[email protected]> Co-authored-by: Matthieu Muffato <[email protected]> Co-authored-by: Matthieu Muffato <[email protected]>

muffato added 3 commits March 16, 2022 10:58

Implemented "default_version" for TCL

496a305

Merge branch 'main' into latest_symlink

2661e03

Merge remote-tracking branch 'fork/tcl_default_version' into latest_s…

6c0802e

…ymlink

muffato force-pushed the add/symlink-install branch from 9d2223e to 60e2222 Compare March 18, 2022 02:08

muffato changed the title ~~[incomplete] Skip module.tcl in the symlinks~~ Skip module.tcl in the TCL symlinks Mar 18, 2022

muffato mentioned this pull request Mar 18, 2022

Adding testing of a symlink tree install #502

Closed

muffato added 3 commits March 19, 2022 23:41

Use write_version_file for the symlink tree too

2ea12ac

Symlink to module.lua when possible

c671723

which is when default_version==True (default_version==False can't be made to work with symlinks).

muffato force-pushed the add/symlink-install branch from 60e2222 to c671723 Compare March 20, 2022 00:51

muffato changed the title ~~Skip module.tcl in the TCL symlinks~~ Skip module in the symlinks Mar 20, 2022

vsoch added a commit that referenced this pull request Mar 20, 2022

opening pr to show diff of #511

1406561

Signed-off-by: vsoch <[email protected]>

Merge branch 'add/symlink-install' into add/symlink-install

f7bf019

Merge branch 'add/symlink-install' into add/symlink-install

896673b

vsoch reviewed Mar 26, 2022

View reviewed changes

shpc/main/modules/__init__.py Outdated Show resolved Hide resolved

shpc/main/modules/__init__.py Outdated Show resolved Hide resolved

shpc/main/modules/__init__.py Outdated Show resolved Hide resolved

muffato and others added 6 commits March 27, 2022 04:50

Made force optional

df82913

Co-authored-by: Vanessasaurus <[email protected]>

Forgot the variable for substitution

069010b

The "delete" command was superseded by "uninstall" in singularityhub#6

daf78d7

Added --no-symlink-tree to override the config file

5496b00

`--symlink-tree` now also overrides the config file

Make it explicit we are expecting yes or no

7265c95

muffato force-pushed the add/symlink-install branch from 4fe601c to 7265c95 Compare March 27, 2022 03:54

vsoch reviewed Mar 27, 2022

View reviewed changes

vsoch merged commit 18a76ee into singularityhub:add/symlink-install Apr 1, 2022

muffato deleted the add/symlink-install branch April 2, 2022 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip `module` in the symlinks #511

Skip `module` in the symlinks #511

muffato commented Mar 10, 2022

muffato commented Mar 18, 2022

muffato commented Mar 20, 2022

vsoch commented Mar 20, 2022

muffato commented Mar 20, 2022

vsoch commented Mar 20, 2022

vsoch commented Mar 20, 2022

muffato commented Mar 20, 2022

muffato commented Mar 26, 2022

vsoch Mar 27, 2022

muffato Mar 27, 2022

vsoch Mar 27, 2022

vsoch Mar 27, 2022 •

edited

Loading

vsoch Mar 27, 2022

muffato Mar 27, 2022

vsoch Mar 27, 2022

marcodelapierre commented Mar 28, 2022 •

edited

Loading

vsoch commented Mar 29, 2022

muffato commented Apr 1, 2022

vsoch commented Apr 1, 2022

Skip module in the symlinks #511

Skip module in the symlinks #511

Conversation

muffato commented Mar 10, 2022

muffato commented Mar 18, 2022

muffato commented Mar 20, 2022

vsoch commented Mar 20, 2022

muffato commented Mar 20, 2022

vsoch commented Mar 20, 2022

vsoch commented Mar 20, 2022

muffato commented Mar 20, 2022

muffato commented Mar 26, 2022

vsoch Mar 27, 2022

Choose a reason for hiding this comment

muffato Mar 27, 2022

Choose a reason for hiding this comment

vsoch Mar 27, 2022

Choose a reason for hiding this comment

vsoch Mar 27, 2022 • edited Loading

Choose a reason for hiding this comment

vsoch Mar 27, 2022

Choose a reason for hiding this comment

muffato Mar 27, 2022

Choose a reason for hiding this comment

vsoch Mar 27, 2022

Choose a reason for hiding this comment

marcodelapierre commented Mar 28, 2022 • edited Loading

vsoch commented Mar 29, 2022

muffato commented Apr 1, 2022

vsoch commented Apr 1, 2022

Skip `module` in the symlinks #511

Skip `module` in the symlinks #511

vsoch Mar 27, 2022 •

edited

Loading

marcodelapierre commented Mar 28, 2022 •

edited

Loading