-
Notifications
You must be signed in to change notification settings - Fork 109
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.
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
"module use" does not conform with manpage #60
Comments
Original comment by: rkowen |
I will implement this in the 3.3.x line. I'm reluctant to change the 3.2.x line with this, since it changes a long standing "feature." Original comment by: rkowen |
The more I think about it the more I like option #1. Just modify the manpage so it says: --append will add the directory to the end of the MODULEPATH if the directory is not already in the path. (Keep it simple) :) Original comment by: sirdude |
Option #2 is not really that difficult. Users can already work around the problem at the command-line by performing a 'module unuse' before each 'module use'. But it would be more convenient if 'modulecmd' did this automatically, so that users always get the MODULEPATH they expect. I have a modified version of modules-3.2.6 that implements the change described above. Please see the attached patch. Index: ModuleCmd_Use.c
===================================================================
--- ModuleCmd_Use.c (3.2.6)
+++ ModuleCmd_Use.c (3.2.6b)
@@ -188,12 +188,17 @@
}
/**
- ** Remove is done by another subroutine
+ ** Always remove paths so they are replaced in the expected order.
+ ** Remove is done by another subroutine.
**/
+ if( ModuleCmd_UnUse( interp, argc, argv) == TCL_ERROR) {
+ /* Command failed, so return error status */
+ return( TCL_ERROR);
+ } else if ( g_flags & M_REMOVE) {
+ /* Current mode flags specify that paths should only be removed */
+ return( TCL_OK);
+ }
- if( g_flags & M_REMOVE)
- return( ModuleCmd_UnUse( interp, argc, argv));
-
/**
** Append or prepend the new module path
**/ Original comment by: mw00ds |
In reply to Milton Woods (mw00ds@users.sf.net):
I know it's not difficult. But really, I think the manpage does not Kent
-- Original comment by: sirdude |
Hi Kent,
As far as I can tell, either option would produce the same results when searching for modulefiles. But the second option (removing the duplicate path) would prevent the MODULEPATH from growing, so it is probably more efficient in the long run.
I think it would be more useful to give the user what they have requested. Otherwise they may resort to a workaround such as setting MODULEPATH directly, which defeats the purpose of the 'module use' command. Regards, Original comment by: mw00ds |
In reply to Milton Woods (mw00ds@users.sf.net):
Hi Milton, I'm really not trying to argue with you, though it may sound like it. :) Giving the user what they requested though is the tricky bit. After I saw the ticket and wanted to share my option of things to give an Kent Original comment by: sirdude |
Hi Kent, Thanks for taking an interest in the issue. I agree that there are several possible solutions, but I do have a strong preference for "fixing" the code instead of the manpage.
The manpage seems fairly unambiguous to me:
Directories are either prepended or appended to MODULEPATH. There is no mention of conditional behaviour if a directory is already in MODULEPATH. Of course, there are some special cases that need to be handled but not necessarily explained in the manpage. For example, the handling of duplicate directories in MODULEPATH is undocumented, but whatever action is taken should not change the documented behaviour. Another undocumented feature is that 'module use' refuses to accept a directory that does not exist - but that behaviour seems reasonable and is quite predictable. I agree with you that, no matter what solution is chosen, the documentation should describe how the software actually works. But in this case, I am quite satisfied with the documented behaviour and I would like the software to work "as advertised". Regards, Original comment by: mw00ds |
I will implement option 1 aka the manpage update. Option 2 could be done later on if someone is still interested on this behavior change. It could be implemented as a configurable option, to let people decide the behavior they want and avoid breaking current behavior for people relying on it. Such change should also impact the |
According to the "module" manpage, the "module use" command should behave as follows:
However, what actually happens depends on the initial contents of MODULEPATH. For example:
So it appears that the search order in the MODULEPATH will not change if a specified directory is in the initial MODULEPATH. This applies to at least the following versions of modules - 3.2.6 & 3.2.7 (C-version) and 1.146 (TCL-version).
The actual behaviour of the "module use" could be considered a bug, because it does not conform with the manpage. This "bug" could be addressed in a few ways, such as:
I would prefer option 2 over option 1, because the current behaviour can cause confusing errors in scripts. For example, if a script inherits a MODULEPATH from the environment, the MODULEPATH may not be set as expected by an internal "module use" command. Option 3 avoids the problem, but it may not be future-proof.
Reported by: mw00ds
The text was updated successfully, but these errors were encountered: