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

gsell · 2025-11-05T16:42:31Z

tcl/subcmd.tcl.in

+# ;;; tcl-indent-level: 3
+# ;;; tcl-continued-indent-level: 3
+# ;;; indent-tabs-mode: nil
+# ;;; End:


Emacs is not happy with the trailing ***, so I removed them. tcl-continued-indent-level still doesn't work for me.

It seems that tcl-continued-indent-level do not do we need here but it is ok as this situation does not occur too often.

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

xdelaruelle · 2025-11-21T15:49:51Z

tcl/envmngt.tcl.in

         } else {
            set val $dir
         }
      }


the condition introduced above to set val may be placed here under a } elseif {[getConf path_entry_reorder]} {

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)

gsell added 3 commits October 29, 2025 12:51

follow Lmod rules for path updates

15731e6

code not belonging to this PR removed

041b33a

untabify

9f5864b

xdelaruelle reviewed Nov 2, 2025

View reviewed changes

gsell added 4 commits November 5, 2025 16:05

config option renamed and add-path reviewed

cf424aa

configure fixed

37ed41b

comments added to make design more clear

76b8dea

Emacs local variables fixed

8b0e5d1

gsell commented Nov 5, 2025

View reviewed changes

xdelaruelle reviewed Nov 9, 2025

View reviewed changes

gsell added 6 commits November 10, 2025 13:28

substitution for pathentryreorder added to Makefile

101ed8a

Option --enable-path-entry-reoder fixed

6d8e491

Use @pathentryreorder@ to set default

7bf3a19

configuration option property fixed for path_entry_reorder

ddc1d4a

revised code if path_entry_reoder is set

5964ceb

untabify

e48dea7

gsell commented Nov 12, 2025

View reviewed changes

path_entry_reorder code reviewed

6b06afc

xdelaruelle reviewed Nov 21, 2025

View reviewed changes

add-path reviewed

a82c71b

documentation of path_entry_reorder added (still draft)

8af7c95

follow Lmod rules for path updates #598

Are you sure you want to change the base?

follow Lmod rules for path updates #598

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!

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!

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!

xdelaruelle commented Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants