Allow registering multiple hooks of the same type - alternate approach

[casparvl]

Note: there is an alternative approach in another PR. However, I consider the approach in this (#696) PR cleaner, because it is more explicit.

This PR stores hooks in Hook.lua internally as tables. It also adds the additional possibility of appending hooks to this table: a third (boolean) argument can be passed to hook.register that defines if the function should be appended (true) to the exist function, or whether it should overwrite it (false). The default value if this argument is not specified is false, to maintain backwards compatibility.

It can be tested with this simple SitePackage.lua:

require("strict")
local hook = require("Hook")

local function load_hook_a(t)
    local frameStk  = require("FrameStk"):singleton()
    local mt        = frameStk:mt()
    local simpleName = string.match(t.modFullName, "(.-)/")
    LmodMessage("Load hook A called on " .. simpleName)
end

local function load_hook_b(t)
    local frameStk  = require("FrameStk"):singleton()
    local mt        = frameStk:mt()
    local simpleName = string.match(t.modFullName, "(.-)/")
    LmodMessage("Load hook B called on " .. simpleName)
end

hook.register("load", load_hook_a)
hook.register("load", load_hook_b) -- overwrites the previous hook, as the append argument is missing (and defaults to false)
hook.register("load", load_hook_b, true) -- appends to the previous hook
hook.register("load", load_hook_a, true) -- appends to the previous hook

I consider the fact that you have to be explicit (by passing the append boolean argument) a plus compared to the other PR, where the appending behaviour is implicit based on the datatype of the func argument. I'm sure that implicit behaviour will surprise some people, whereas this explicit behaviour shouldn't surprise anyone. It also makes for slightly cleaner code internally, since we don't have to handle an extra potential datatype.

Anyway, those are my 2 cents, curious to hear what you think.

[rtmclay]

I have created a branch at github called PR696 which started as your patch. I have made some key changes.

1. Changed the use of true to mark an append. Instead I have make the optional third argument to be one of the following: "append", "prepend" and "replace". No argument means "replace".
2. The last function in each hook array must return a value. Not all hooks return a value but some do and some return multiple results.

Please test the PR696 branch to see if it works for you.

Also the LMOD_RC is only for properties and will not work for hooks.

[rtmclay]

The PR696 branch also has updated documentation. Please review files: docs/source/025_new.rst and docs/source/170_hooks.rst

[casparvl]

Thanks for polishing this and implementing the tests! I think they cover all scenarios well.

Please test the PR696 branch to see if it works for you.

I've tested it (actually using the SitePackage.lua's you wrote for the tests :)), works like a charm.

Also the LMOD_RC is only for properties and will not work for hooks.

Yep, message well received in the Lmod meeting :) I actually also ran into this when I created this PR: indeed I got into a scenario where the lmodrc file wasn't loaded 'early' enough, and the hooks would only be called once I was loading modules with the GPU property. So not putting this in lmodrc isn't only right in theory, it will break in certain cases. The reason we didn't hit this in EESSI is because we use an Lmod cache: initializing that happens early if you call the module command, and triggers a load of the lmodrc files - but that's clearly not a guarantee. Anyway, I've made a PR to change this in EESSI. Always good to learn :)

docs/source/025_new.rst

Looks good to me.

docs/source/170_hooks.rst

You could consider to add the expected output for even more clarity:

local function load_hook_a(t)
    local frameStk  = require("FrameStk"):singleton()
    local mt        = frameStk:mt()
    local simpleName = string.match(t.modFullName, "(.-)/")
    LmodMessage("Load hook A called on " .. simpleName)
end

local function load_hook_b(t)
    local frameStk  = require("FrameStk"):singleton()
    local mt        = frameStk:mt()
    local simpleName = string.match(t.modFullName, "(.-)/")
    LmodMessage("Load hook B called on " .. simpleName)
end

hook.register("load", load_hook_a)
hook.register("load", load_hook_b) -- overwrites the previous hook,
-- expected output for 'module load my_software/version':
-- Load hook B called on my_software

hook.register("load", load_hook_a)
hook.register("load", load_hook_b, "replace") -- overwrites the previous hook function,
-- expected output for 'module load my_software/version':
-- Load hook B called on my_software

-- > the following will run load_hook_a then load_hook_b.
hook.register("load", load_hook_a)           -- initializes the load hook function
hook.register("load", load_hook_b, "append") -- appends to the previous hook function.
-- expected output for 'module load my_software/version':
-- Load hook A called on my_software
-- Load hook B called on my_software

-- > the following will run load_hook_b then load_hook_a
hook.register("load", load_hook_a)            -- initializes the load hook function
hook.register("load", load_hook_b, "prepend") -- prepends to the previous hook function
-- expected output for 'module load my_software/version':
-- Load hook B called on my_software
-- Load hook A called on my_software

Also, I'd replace

Note that if the optional third argument (the action argument) is missing, causes the 2nd call or later call to hook.register to replace the function for a given hook name.

with the more straighforward

Note that if the optional third argument (the action argument) is not provided, the default behaviour is "replace".

The way you handle the return values of the hooks also makes sense to me

Finally,

There are some hooks (such as groupName, SiteName, etc) that require return values. The last registered hook function will be used to return the value.

makes total sense to me and I think this is the most intuitive behavior. I guess for those types of hooks, it doesn't really make sense to us append or prepend. You could consider documenting that since they rely on a single return type, using append and prepend does not serve any purpose and is therefor discouraged.

Also, in the current Hook functions section, you might want to document which hooks return something, so that the reader knows to which hooks your comment above applies exactly: sometimes this is explicit from the description (e.g. parse_updateFn(...): This hook returns the time on the timestamp file.), but sometimes, it is more implicit (SiteName: This hook is used to specify Site Name. It is used to generate family prefix: site_FAMILY_COMPILER site_FAMILY_MPI … => it might not be clear that this means it returns that prefix)

[rtmclay]

I have updated the documentation based on your comments. In addition I have remarked on which hooks return something. Some of these hooks require a strong understanding of how Lmod uses these hook functions.

[casparvl]

Looks good to me! What's the process: we close this PR and you make a new PR from your feature branch?

[rtmclay]

I think the question is when I close this pull request does the PR696 branch disappear. The answer is no. You can use and test this branch. Sometime in the near future the PR696 branch will be merged into the main branch.

Does this answer your question or have I misunderstood your question.

[rtmclay]

The PR696 branch has been merged on to the main branch and is now Lmod 8.7.36. Thanks for the issue and the PR's