follow Lmod rules for path updates #598

gsell · 2025-10-29T12:22:37Z

Changes implemented in this PR:

add config option lmod_path_rules
if lmod_path_rules is set: append/prepend dir to path and remove all other occurrences of dir from path

xdelaruelle

Many thanks for your contribution. In addition to the code feedback:

We should find a better name for the option. It should not relate to an external software (that could change its name or behavior over the time) and it should concisely express what it does for users to easily get it
This option should apply to any kind of path element addition. So module use is also concerned. I have quickly looked at the code base and it seems that adding code to add-path procedure will cover any kind of path element addition.
The Add new configuration option document will guide you over all the things to adapt to introduce a new config option

xdelaruelle · 2025-11-02T09:00:46Z

tcl/envmngt.tcl.in

         } else {
-            set val $dir
+            set mpath_list "[lsearch -inline -all -not -exact\
+                $mpath_list $dir] $dir"


indent does not seem correct (4 spaces instead of 3)

I'm still fighting with the Emacs config for the Tcl-Mode. Setting tcl-continued-indent-level to 3 doesn't work as expected. Example:

set mpath_list [lsearch -inline -all -not -exact\ $mpath_list $dir]

Do you have an idea how I can fix this in Emacs?

I am not an Emacs user so I cannot help here unfortunately.

xdelaruelle · 2025-11-02T09:38:09Z

tcl/envmngt.tcl.in

+      set mpath_list [split $val $separator]
+      foreach dir $path_list {
+         if {$bhv eq {prepend}} {
+            set mpath_list "$dir [lsearch -inline -all -not -exact\


I recommend having the path removal command separated, as it is the same whether we append or we prepend.

xdelaruelle · 2025-11-02T09:40:37Z

tcl/envmngt.tcl.in

+            }
         }
      }
      if {[info exists countarr($dir)]} {


code to be added should be placed under this if branch instead of duplicating the whole foreach loop. this way --duplicates and --ignore-refcount are already handled.

Done in the revised code.

I think code addition would be better located within the if {[info exists countarr($dir)]} { code block as there would be no need to handle duplicates, ignore-refcount mode and also no need to have specific code to increase or not the reference counter.

There would be a small duplication of code to add the path entry into the variable value, but everything else will be already managed.

xdelaruelle · 2025-11-02T09:42:32Z

tcl/envmngt.tcl.in

+# Local Variables:
+# Mode: tcl-mode
+# tcl-indent-level: 3
+# End:


is there a reason why changing the whole syntax instead of just adding the indent directive using the existing syntax?

xdelaruelle · 2025-11-02T09:53:15Z

tcl/envmngt.tcl.in

+      set mpath_list [split $val $separator]
+      foreach dir $path_list {
+         if {$bhv eq {prepend}} {
+            set mpath_list "$dir [lsearch -inline -all -not -exact\


It would be interesting to have a code comment about the design decision: if the path element is found multiple times, all these entries are replaced by a single occurrence at the beginning or at the end

tcl/subcmd.tcl.in

xdelaruelle · 2025-11-09T09:36:38Z

configure

 extendeddefault moduleshome initconfin pager pageropts verbosity color \
 darkbgcolors lightbgcolors termbg lockedconfigs icase unloadmatchorder \
-searchmatch modulepath loadedmodules quarantinevars wa277 advversspec ml \
+searchmatch modulepath loadedmodules quarantinevars wa277 lmodpath advversspec ml \


need to adapt the option name here

xdelaruelle · 2025-11-09T09:37:34Z

tcl/init.tcl.in

      alias indesym sym tag hidden key} {} {} eltlist}\
   list_terse_output {MODULES_LIST_TERSE_OUTPUT {@listterseoutput@} 0 l\
      {header idx variant alias indesym sym tag hidden key} {} {} eltlist}\
+   path_entry_reorder {MODULES_PATH_ENTRY_REORDER 1 0 b {0 1}}\


use @pathentryreorder@ as default value for option

xdelaruelle · 2025-11-09T09:44:22Z

tcl/envmngt.tcl.in

+            }
         }
      }
      if {[info exists countarr($dir)]} {


I think code addition would be better located within the if {[info exists countarr($dir)]} { code block as there would be no need to handle duplicates, ignore-refcount mode and also no need to have specific code to increase or not the reference counter.

There would be a small duplication of code to add the path entry into the variable value, but everything else will be already managed.

xdelaruelle · 2025-11-09T09:48:04Z

configure

      pythonbin=$(get_package_value "$arg") ;;
   --with-module-path=*)
      echo_warning "Option \`--with-module-path' ignored, use \`--modulepath' instead" ;;
+   --enable-path-entry-reorder|--disable-path-entry-reorder)


the * character after --enable-path-entry-reorder is missing (to handle --enable-path-entry-reorder=no syntax).

best is to move this code to handle an --enable option next to other option of this kind.

gsell · 2025-11-12T15:07:33Z

tcl/envmngt.tcl.in

+            }
+         } else {
+            set countarr($dir) 1
+         }


I'm still a bit confused/unsure about the reference counter countarr($dir). If path_entry_reorder is true, are there cases where countarr($dir) is not equal to 1? If it is always 1, the current solution might be the best in terms of readability.

It could be greater than one (for instance if two loaded modules add the same path entry)

Yes, but if path_entry_reorder is true, all occurrences of $dir are removed and exactly one is added again. Looks like I still don't understand the code. :( Maybe an example would help.

countarr is the reference counter of each entry (how many loaded modules set this entry). It helps when unloading a module to determine if the path entry should be kept (if it is used by other loaded modules) or not.

reference counter should behave the same whether path_entry_reorder is set or not. These 2 concepts are not linked.

Do not hesitate to tell me if it is still not clear.

Thanks for the clarification! The allow_dup confused me.

What is the correct behaviour if path_entry_reorder is set and allow_dup is true? If $dir already exists in path move them to the beginning/end and append/prepend $dir? Example:

[gsell@merlin-l-001 modules]$ module append-path --duplicates PATHVAR /dir [gsell@merlin-l-001 modules]$ module append-path --duplicates PATHVAR /foo [gsell@merlin-l-001 modules]$ module append-path --duplicates PATHVAR /bar [gsell@merlin-l-001 modules]$ module append-path --duplicates PATHVAR /foo [gsell@merlin-l-001 modules]$ module append-path --duplicates PATHVAR /anotherdir [gsell@merlin-l-001 modules]$ module config path_entry_reorder 1 [gsell@merlin-l-001 modules]$ module append-path --duplicates PATHVAR /foo

In this case PATHVAR should be

/dir:/bar:/anotherdir:/foo:/foo:/foo

or?

I think it would be better for allow_dup to have precedence over path_entry_reorder.

Which means if allow_dup is set the value of path_entry_reorder should not change its behavior: if $dir already exists, path is not moved and a new occurrence of it is appended/prepended to $dir.

xdelaruelle · 2025-11-21T15:47:15Z

tcl/envmngt.tcl.in

+         set val [join $mpath_list $separator]
+      }
+      if {[getConf path_entry_reorder] || ![info exists countarr($dir)]\
+         || $allow_dup} {


if dir is NOT in path || duplicates are allowed, there is no need to check the state of path_entry_reorder

Without checking the state of path_entry_reorder. It can happen, that $dir is removed but not added again. If path_entry_reorder is true and $dir is added twice, $dir will be removed in the second call but not added again, because countarr($dir) exists and $allow_dup is false.

it will be interesting to add a specific test for the case you describe, for future refactoring not to miss that point

tcl/envmngt.tcl.in

xdelaruelle · 2025-12-05T06:26:29Z

@gsell Many thanks for the new commits.

To go further, please follow the Add new configuration option guide to add documentation, completion and tests.

It will especially help to fix the existing "config" tests to see if the change does not introduce regression.

Specific test to demonstrate the new feature should also be added. Especially to highlight corner cases.
All your commits should also be adapted to include a developer certificate of origin (DCO)

pull changes from upstream

xdelaruelle · 2025-12-31T07:45:48Z

@gsell Thanks for the new additions. The most critical part that is missing is the DCO signature on each commit: https://modules.readthedocs.io/en/latest/CONTRIBUTING.html#developer-certificate-of-origin-dco. You can correctly rebase and sign-off all the commits with the following command:

git rebase -i main --signoff

Once that will be done, I will handle the rest for you. I will add some tests so it will provide you an example for future contributions :-)

Signed-off-by: Achim Gsell <[email protected]>

gsell · 2026-01-05T17:03:28Z

@xdelaruelle the commits are signed now. But I still have a merge commit which is not signed:

commit 03b655a233da6a39bd3ff966ef2caa2de7085168 (origin/main, origin/HEAD, main)
Merge: 6860bdea 7c91bea8
Author: Achim Gsell <[email protected]>
Date:   Fri Dec 19 09:55:08 2025 +0100

    Merge pull request #1 from envmodules/main
    
    pull changes from upstream

and I don't know how to sign it.

Another issue might be:

commit fb65540800b0cd46a2fcb267502e2cbf7372d758 (HEAD -> enhancement/Update_order_of_entries_in_path_environment_variable, origin/enhancement/Update_order_of_entries_in_path_environment_variable)
gpg: Signature made Mon Jan  5 17:45:46 2026 CET
gpg:                using EDDSA key 37BB6807BB9F78F76CB16F1F0B5913AD51BFF7DA
gpg: Good signature from "Achim Gsell <[email protected]>" [unknown]
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 37BB 6807 BB9F 78F7 6CB1  6F1F 0B59 13AD 51BF F7DA
Author: Achim Gsell <[email protected]>
Date:   Thu Dec 18 14:44:27 2025 +0100

    setting default value for path_entry_reorder fixed
    
    Signed-off-by: Achim Gsell <[email protected]>

We are using GPG mainly internally and I (we) signed our commits. Would it be possible that you sign my key?

gsell · 2026-01-05T17:07:42Z

Help with the tests would be very appreciated! It took me a while to understand the design/concept behind them. For me it is still not clear where to add the new tests. I didn't found an example where similar tests are performed after changing something with module config.

gsell · 2026-01-05T17:11:15Z

While trying to understand how to run the tests, I run into an issue with resolving group id's: In some cases group id's cannot be resolved to a name. In particular this is the case if you are in an AFS process authentication group (PAG).

I can open a new issue for this problem.

xdelaruelle · 2026-01-06T08:26:38Z

@xdelaruelle the commits are signed now. But I still have a merge commit which is not signed:
...
and I don't know how to sign it.

No need to sign this one, it will disappear when I will rebase everything. The important is to sign the commit with your content, which is done now (the DCO CI test is green).

Another issue might be:

commit fb65540800b0cd46a2fcb267502e2cbf7372d758 (HEAD -> enhancement/Update_order_of_entries_in_path_environment_variable, origin/enhancement/Update_order_of_entries_in_path_environment_variable)
gpg: Signature made Mon Jan  5 17:45:46 2026 CET
gpg:                using EDDSA key 37BB6807BB9F78F76CB16F1F0B5913AD51BFF7DA
gpg: Good signature from "Achim Gsell <[email protected]>" [unknown]
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 37BB 6807 BB9F 78F7 6CB1  6F1F 0B59 13AD 51BF F7DA
Author: Achim Gsell <[email protected]>
Date:   Thu Dec 18 14:44:27 2025 +0100

    setting default value for path_entry_reorder fixed
    
    Signed-off-by: Achim Gsell <[email protected]>

We are using GPG mainly internally and I (we) signed our commits. Would it be possible that you sign my key?

I have no GPG key. Crypto signature is not required here. Just the "Signed-off-by: " line is needed.

xdelaruelle · 2026-01-06T08:28:20Z

While trying to understand how to run the tests, I run into an issue with resolving group id's: In some cases group id's cannot be resolved to a name. In particular this is the case if you are in an AFS process authentication group (PAG).

I can open a new issue for this problem.

Yes feel free to open an issue for that. What will be important is to describe it for me to be able to reproduce the issue on my side.

xdelaruelle · 2026-01-07T07:20:11Z

Help with the tests would be very appreciated! It took me a while to understand the design/concept behind them. For me it is still not clear where to add the new tests. I didn't found an example where similar tests are performed after changing something with module config.

Tests relative to the modulefile evaluation are located in the testsuite/modules.50-cmds directory. The tests for prepend-path and append-path` are located in the 05x and 06x files.

It is expected to adapt these existing tests in case the testsuite is run with the new option mode enabled. Affected tests should be adapted or they may be duplicated into dedicated 05x and 06x files. If the second option is preferred, add setenv_var MODULES_PATH_ENTRY_REORDER 0 at the beginning of existing affected files and setenv_var MODULES_PATH_ENTRY_REORDER 1 at the beginning of the new files.

To quickly determine the affected tests:

make distclean
./configure --enable-path-entry-reorder
script/mt 50/{05,06}

Several new tests should be added to check the corner cases relative to the code logic put in place.

Regarding the config sub-command, it is tested in 70/220. The Add new configuration option guide shows how to update this file to cover the new option.

Do not hesitate if this is not clear or if you need additional information.

Signed-off-by: Achim Gsell <[email protected]>

gsell · 2026-01-07T16:19:20Z

The affected test are fixed now. Are there more tests I should add? Maybe you have some edge cases in mind.

The test for the config option have been already added somewhen in the past.

What else must be done?

xdelaruelle · 2026-01-07T19:45:31Z

The affected test are fixed now. Are there more tests I should add? Maybe you have some edge cases in mind.

The test for the config option have been already added somewhen in the past.

What else must be done?

It may be interesting to add a test to demonstrate the corner case mentionned previously:

Without checking the state of path_entry_reorder. It can happen, that $dir is removed but not added again. If path_entry_reorder is true and $dir is added twice, $dir will be removed in the second call but not added again, because countarr($dir) exists and $allow_dup is false.

I also suggest to update .github/workflows/linux_tests.yaml to add on tcl85-2 test case the --enable-path-entry-reorder option into CONFIGURE_OPTS. This way one CI environment will run the testsuite with this new option enabled by default.

Doc need to be fixed to:

change :instopt:--with-path-entry-reorder into :instopt:--enable-path-entry-reorder.
indicate that the option are added on version 5.7 (instead of 5.x)

xdelaruelle reviewed Nov 2, 2025

View reviewed changes

gsell commented Nov 5, 2025

View reviewed changes

tcl/subcmd.tcl.in Show resolved Hide resolved

xdelaruelle reviewed Nov 9, 2025

View reviewed changes

gsell commented Nov 12, 2025

View reviewed changes

xdelaruelle reviewed Nov 21, 2025

View reviewed changes

tcl/envmngt.tcl.in Show resolved Hide resolved

Merge pull request #1 from envmodules/main

03b655a

pull changes from upstream

gsell added 21 commits January 5, 2026 17:44

follow Lmod rules for path updates

81797d6

Signed-off-by: Achim Gsell <[email protected]>

code not belonging to this PR removed

df8c748

Signed-off-by: Achim Gsell <[email protected]>

untabify

1c2a7b4

Signed-off-by: Achim Gsell <[email protected]>

config option renamed and add-path reviewed

3015e13

Signed-off-by: Achim Gsell <[email protected]>

configure fixed

1cd103b

Signed-off-by: Achim Gsell <[email protected]>

comments added to make design more clear

45a90b1

Signed-off-by: Achim Gsell <[email protected]>

Emacs local variables fixed

997f58b

Signed-off-by: Achim Gsell <[email protected]>

substitution for pathentryreorder added to Makefile

967b8c0

Signed-off-by: Achim Gsell <[email protected]>

Option --enable-path-entry-reoder fixed

8866671

Signed-off-by: Achim Gsell <[email protected]>

Use @pathentryreorder@ to set default

8e0119d

Signed-off-by: Achim Gsell <[email protected]>

configuration option property fixed for path_entry_reorder

d0cb4e1

Signed-off-by: Achim Gsell <[email protected]>

revised code if path_entry_reoder is set

c040243

Signed-off-by: Achim Gsell <[email protected]>

untabify

d0c9208

Signed-off-by: Achim Gsell <[email protected]>

path_entry_reorder code reviewed

4e05776

Signed-off-by: Achim Gsell <[email protected]>

add-path reviewed

79309f2

Signed-off-by: Achim Gsell <[email protected]>

documentation of path_entry_reorder added (still draft)

b9b7009

Signed-off-by: Achim Gsell <[email protected]>

documentation of path_entry_reorder updated

9408131

Signed-off-by: Achim Gsell <[email protected]>

documentation updated

7e163e7

Signed-off-by: Achim Gsell <[email protected]>

documentation updated

2e65696

Signed-off-by: Achim Gsell <[email protected]>

testsuite updated

87e23ee

Signed-off-by: Achim Gsell <[email protected]>

setting default value for path_entry_reorder fixed

fb65540

Signed-off-by: Achim Gsell <[email protected]>

gsell force-pushed the enhancement/Update_order_of_entries_in_path_environment_variable branch from 8aea288 to fb65540 Compare January 5, 2026 16:56

append/prepend tests added

4ff70bf

Signed-off-by: Achim Gsell <[email protected]>

follow Lmod rules for path updates #598

Are you sure you want to change the base?

follow Lmod rules for path updates #598

Uh oh!

Conversation

gsell commented Oct 29, 2025

Uh oh!

xdelaruelle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xdelaruelle commented Dec 5, 2025

Uh oh!

xdelaruelle commented Dec 31, 2025

Uh oh!

gsell commented Jan 5, 2026

Uh oh!

gsell commented Jan 5, 2026

Uh oh!

gsell commented Jan 5, 2026

Uh oh!

xdelaruelle commented Jan 6, 2026

Uh oh!

xdelaruelle commented Jan 6, 2026

Uh oh!

xdelaruelle commented Jan 7, 2026

Uh oh!

gsell commented Jan 7, 2026

Uh oh!

xdelaruelle commented Jan 7, 2026

Uh oh!

Reviewers

Assignees