Skip to content

follow Lmod rules for path updates #598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gsell wants to merge 16 commits into
base: main
Choose a base branch
from
+84 −18
Open

follow Lmod rules for path updates #598

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
15731e6
follow Lmod rules for path updates
gsell Oct 29, 2025
041b33a
code not belonging to this PR removed
gsell Oct 29, 2025
9f5864b
untabify
gsell Oct 29, 2025
cf424aa
config option renamed and add-path reviewed
gsell Nov 5, 2025
37ed41b
configure fixed
gsell Nov 5, 2025
76b8dea
comments added to make design more clear
gsell Nov 5, 2025
8b0e5d1
Emacs local variables fixed
gsell Nov 5, 2025
101ed8a
substitution for pathentryreorder added to Makefile
gsell Nov 10, 2025
6d8e491
Option --enable-path-entry-reoder fixed
gsell Nov 10, 2025
7bf3a19
Use @pathentryreorder@ to set default
gsell Nov 10, 2025
ddc1d4a
configuration option property fixed for path_entry_reorder
gsell Nov 12, 2025
5964ceb
revised code if path_entry_reoder is set
gsell Nov 12, 2025
e48dea7
untabify
gsell Nov 12, 2025
6b06afc
path_entry_reorder code reviewed
gsell Nov 20, 2025
a82c71b
add-path reviewed
gsell Dec 2, 2025
8af7c95
documentation of path_entry_reorder added (still draft)
gsell Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ sed -e 's|@prefix@|$(prefix)|g' \
-e 's|@sourcecache@|$(setsourcecache)|g' \
-e 's|@searchmatch@|$(searchmatch)|g' \
-e 's|@wa277@|$(setwa277)|g' \
-e 's|@pathentryreorder@|$(pathentryreorder)|g' \
-e 's|@icase@|$(icase)|g' \
-e 's|@nearlyforbiddendays@|$(nearlyforbiddendays)|g' \
-e 's|@tagabbrev@|$(tagabbrev)|g' \
Expand Down
6 changes: 5 additions & 1 deletion configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ multilibsupport libdir64 libdir32 versioning silentshdbgsupport \
setshellstartup quarantinesupport autohandling availindepth implicitdefault \
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 pathentryreorder advversspec ml \
windowssupport nearlyforbiddendays implicitrequirement tagabbrev \
tagcolorname mcookieversioncheck availoutput availterseoutput listoutput \
listterseoutput editor variantshortcut bashcompletiondir fishcompletiondir \
Expand Down Expand Up @@ -79,6 +79,7 @@ extendeddefault=y
advversspec=y
ml=y
wa277=n
pathentryreorder=n
loadedmodules=
quarantinevars=
binsearchpath=/usr/bin:/bin:/usr/local/bin
Expand Down Expand Up @@ -803,6 +804,9 @@ for arg in "$@"; do
--enable-mcookie-version-check*|--disable-mcookie-version-check)
# shellcheck disable=SC2034
mcookieversioncheck=$(get_feature_value "$arg") ;;
--enable-path-entry-reorder*|--disable-path-entry-reorder)
# shellcheck disable=SC2034
pathentryreorder=$(get_feature_value "$arg") ;;
--with-bin-search-path=*|--without-bin-search-path)
binsearchpath=$(get_package_value "$arg") ;;
--with-moduleshome=*|--without-moduleshome)
Expand Down
3 changes: 2 additions & 1 deletion init/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ comp_lint_opts := -a -i --all --icase
comp_modtosh_opts := --auto --no-auto --force -f --icase -i
comp_path_opts := -d --delim --duplicates
comp_rm_path_opts := -d --delim --index
comp_config_opts := --dump-state --reset abort_on_error advanced_version_spec auto_handling avail_indepth avail_output avail_terse_output cache_buffer_bytes cache_expiry_secs collection_pin_version collection_pin_tag collection_target color colors conflict_unload contact editor extended_default extra_siteconfig hide_auto_loaded home icase ignore_cache ignore_user_rc implicit_default implicit_requirement list_output list_terse_output locked_configs logged_events logger mcookie_check mcookie_version_check ml nearly_forbidden_days pager protected_envvars quarantine_support rcfile redirect_output require_via reset_target_state run_quarantine search_match set_shell_startup shells_with_ksh_fpath silent_shell_debug source_cache spider_indepth spider_output spider_terse_output sticky_purge tag_abbrev tag_color_name tcl_linter term_background term_width unique_name_loaded unload_match_order variant_shortcut verbosity wa_277
comp_config_opts := --dump-state --reset abort_on_error advanced_version_spec auto_handling avail_indepth avail_output avail_terse_output cache_buffer_bytes cache_expiry_secs collection_pin_version collection_pin_tag collection_target color colors conflict_unload contact editor extended_default extra_siteconfig hide_auto_loaded home icase ignore_cache ignore_user_rc implicit_default implicit_requirement list_output list_terse_output locked_configs logged_events logger mcookie_check mcookie_version_check ml nearly_forbidden_days pager protected_envvars quarantine_support rcfile redirect_output require_via reset_target_state run_quarantine search_match set_shell_startup shells_with_ksh_fpath silent_shell_debug source_cache spider_indepth spider_output spider_terse_output sticky_purge tag_abbrev tag_color_name tcl_linter term_background term_width unique_name_loaded unload_match_order variant_shortcut verbosity wa_277 path_entry_reorder

define translate-in-script
$(ECHO_GEN)
Expand Down Expand Up @@ -167,6 +167,7 @@ sed -e 's|@prefix@|$(prefix)|g' \
-e 's|@comp_path_opts@|$(comp_path_opts)|g' \
-e 's|@comp_rm_path_opts@|$(comp_rm_path_opts)|g' \
-e 's|@comp_config_opts@|$(comp_config_opts)|g' \
-e 's|@comp_path_entry_reorder@|$(comp_path_entry_reorder)|g' \
-e '$(setzshfpathre)' \
-e $$'s|@modulerc@|$(modulerc)|g' \
-e 's|@modulepath@|$(modulepath)|g' \
Expand Down
56 changes: 39 additions & 17 deletions tcl/envmngt.tcl.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1816,27 +1816,46 @@ proc add-path {cmd mode dflbhv args} {
set val [get-env $var]

foreach dir $path_list {
if {![info exists countarr($dir)] || $allow_dup} {
# ignore env var set empty if no empty entry found in reference
# counter array (sometimes var is cleared by setting it empty not
# unsetting it)
if {$val ne {} || [info exists countarr()]} {
if {[getConf path_entry_reorder] && [info exists countarr($dir)]\
&& ! $allow_dup} {
# if $dir is in $val and duplicates are not allowed:
# first remove all occurences of $dir in $val
# and add $dir at the beginning or end if $val
# is not the empty string
set mpath_list [split $val $separator]
set mpath_list [lsearch -inline -all -not -exact $mpath_list $dir]
set val [join $mpath_list $separator]
if {$val ne {}} {
set sep [expr {$val eq $separator ? {} : $separator}]
set val [expr {$bhv eq {prepend} ? "$dir$sep$val" :\
"$val$sep$dir"}]
} else {
set val $dir
}
}
if {[info exists countarr($dir)]} {
# do not increase counter if bare separator string is added or if
# reference count is ignored (--ignore-refcount set) unless if
# duplicate mode is enabled (--duplicates set)
if {!$val_set_is_delim && (!$ign_refcount || $allow_dup)} {
incr countarr($dir)
}
} else {
set countarr($dir) 1
} else {
if {![info exists countarr($dir)] || $allow_dup} {
# ignore env var set empty if no empty entry found in reference
# counter array (sometimes var is cleared by setting it empty not
# unsetting it)
if {$val ne {} || [info exists countarr()]} {
set sep [expr {$val eq $separator ? {} : $separator}]
set val [expr {$bhv eq {prepend} ? "$dir$sep$val" :\
"$val$sep$dir"}]
} else {
set val $dir
}
}
if {[info exists countarr($dir)]} {
# do not increase counter if bare separator string is added or if
# reference count is ignored (--ignore-refcount set) unless if
# duplicate mode is enabled (--duplicates set)
if {!$val_set_is_delim && (!$ign_refcount || $allow_dup)} {
incr countarr($dir)
}
} else {
set countarr($dir) 1
}
Copy link
Author

@gsell gsell Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@xdelaruelle xdelaruelle Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@gsell gsell Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@xdelaruelle xdelaruelle Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@xdelaruelle xdelaruelle Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@gsell gsell Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@xdelaruelle xdelaruelle Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down Expand Up @@ -2054,7 +2073,10 @@ proc getModulesEnvVarGlobList {{loaded_ctx 0}} {
return $envvar_glob_list
}

# ;;; Local Variables: ***
# ;;; mode:tcl ***
# ;;; End: ***
# ;;; Local Variables:
# ;;; Mode: tcl-mode
# ;;; tcl-indent-level: 3
# ;;; tcl-continued-indent-level: 3
# ;;; indent-tabs-mode: nil
# ;;; End:
# vim:set tabstop=3 shiftwidth=3 expandtab autoindent:
1 change: 1 addition & 0 deletions tcl/init.tcl.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ array set g_config_defs [list\
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 @pathentryreorder@ 0 0 b {0 1}}\
locked_configs {{} {@lockedconfigs@} 0 o}\
logged_events {MODULES_LOGGED_EVENTS {@loggedevents@} 1 l {auto_eval\
requested_eval requested_cmd} {} {} eltlist}\
Expand Down
9 changes: 6 additions & 3 deletions tcl/subcmd.tcl.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3148,7 +3148,10 @@ proc cmdModuleSpider {show_oneperline show_mtime show_filter search_filter\
$search_match $modpath_list {*}$args
}

# ;;; Local Variables: ***
# ;;; mode:tcl ***
# ;;; End: ***
# ;;; Local Variables:
# ;;; Mode: tcl-mode
# ;;; tcl-indent-level: 3
# ;;; tcl-continued-indent-level: 3
# ;;; indent-tabs-mode: nil
# ;;; End:
Copy link
Author

@gsell gsell Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@xdelaruelle xdelaruelle Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# vim:set tabstop=3 shiftwidth=3 expandtab autoindent: