Support more flexible command line options for ModuleManager

[hiker]

When supporting the fully inlined driver extraction for gocean, compilation fails because now the module manager needs to find parallel_utils_mod. dl_esm_inf provides two implementations, one with MPI, one without. Previously, the standard build options used in our examples would only compile the non-MPI version of the code, and compilation (examples/gocean/eg5/extract) worked.

Now with inlining, the ModuleManager needs to find the source code fore parallel_utils_mod, and it 'picks' the MPI version, so compilation fails.

My suggestion would be to add an option to the module manager to add files to be ignored. The user script could then instruct the module manager to ignore the MPI version of the file.

Note that the current build process of dl_esm_inf builds two libraries, by adding the corresponding source code (MPI or non-MPI) to the list of source files

[hiker]

As discussed elsewhere, the module manager should accept the following specifications:

• a directory which is searched recursively (the current implementation)
• a directory which is searched, but not recursively
  • this is already possible in the API, just no command line option is available
• a filename (potentially a list of filenames) to be added
• an option to exclude a file
• an option to exclude a directory
• Besides API, we need command line options
• And since we are at it, look at combining -d and -I.

Besides adding support to the module manager API, we also need command line options that allow a user to specify the various options. Suggestion:

• -I non-recursive adds a directory (i.e. all files in that directory), or a single file (depending on parameter) - compatible with compiler flags
• -Ir adds directories recursively
• -I- excludes a file or directory

Additionally, as asked in #1991, we should look at combining the current -d and -I options (one for kernel path, one to find modules, and due to the original semantic of -d which is recursive, atm only -d is passed on to the module manager). Once -d is deprecated (assuming that we agree on the above variations of command line options), we can internally map reach -d option to -Ir to avoid breaking the API (again).

@arporter , @sergisiso , are you ok with the suggested command line options? And are you aware of any problem with combining kernel search path and include path?

[arporter]

That sounds fine to me @hiker although there may be unforeseen problems when combining the two. We'll just have to see how it goes. For the "excluding file or directory" I would favour arguments similar to those accepted by e.g. grep so --exclude-dir or --exclude-file. We may want regex support in there?

[sergisiso]

Some other things to consider:

• build systems sometimes generate empty "-I" when some environment variables are unset. We should make sure we don't trip on this.
  e.g -I -I /path_to_libs

• Also, compilers do not require a space between the -I and the directory.
  e.g -I/path_to_libs
  Is this possible with argparse? Would this cause issues with -Ir with relative path starting with 'r'?

• We may want regex support in there?

Bash already does the parameter expansion (I think?), so we just need to accept multiple paths as parameters after '-I'

[hiker]

I like --exclude-dir/-file.

Some other things to consider:

• build systems sometimes generate empty "-I" when some environment variables are unset. We should make sure we don't trip on this.
  e.g -I -I /path_to_libs

I quickly tried:

$./test.py -I bla -Irinclude_path -r file
Namespace(I=['bla', 'rinclude_path'], reverse=True, filename='file')
$ ./test.py -I  -Irinclude_path -r file
usage: test.py [-h] [-I I] [--reverse] filename
test.py: error: argument -I: expected one argument

Surprisingly, that works as expected :)

• Also, compilers do not require a space between the -I and the directory.
  e.g -I/path_to_libs
  Is this possible with argparse? Would this cause issues with -Ir with relative path starting with 'r'?
  We won't have the -Ir problem with Andy's suggestion, and a space is not required. E.g. this scripts accept -I and -r:

./test.py  -Irinclude_path -r file
Namespace(I=['rinclude_path'], reverse=True, filename='file')

So, -Ir is not being confused with -I -r.

• We may want regex support in there?

Bash already does the parameter expansion (I think?), so we just need to accept multiple paths as parameters after '-I'
regex or wildcard?
I can't get that to work with bash ? Well, bash expansion works, but then we won't get them as one parameter as far as I can see - any ideas?

./test.py  -I *py -r file
argv is ['./test.py', '-I', 'dataflow.py', 'omp_opt.py', 'setup.py', 'test.py', '-r', 'file']
usage: test.py [-h] [-I I] [--reverse] filename
test.py: error: unrecognized arguments: setup.py test.py file

$ ./test.py  -I "*py" -r file
argv is ['./test.py', '-I', '*py', '-r', 'file']
Namespace(I=['*py'], reverse=True, filename='file')

$ ./test.py  -I \*py -r file
argv is ['./test.py', '-I', '*py', '-r', 'file']
Namespace(I=['*py'], reverse=True, filename='file')

$ ./test.py  -I "\*py" -r file
argv is ['./test.py', '-I', '\\*py', '-r', 'file']
Namespace(I=['\\*py'], reverse=True, filename='file')

I can get it to work with an echo statement:

$ ./test.py --exclude="$(echo *.py)" ./README.md 
argv is ['./test.py', '--exclude=dataflow.py omp_opt.py setup.py test.py', './README.md']
Namespace(I=None, exclude=['dataflow.py omp_opt.py setup.py test.py'], reverse=False, filename='./README.md')

But I would suggest that if we support this, we will do this in python.

Otherwise I think that was all I needed - thanks!

[sergisiso]

I do not understand some of your responses, for the empty -I you say that it works, but the command above says test.py: error: argument -I: expected one argument. I think is just a matter of adding the argparse nargs='?' option?

For the missing whitespace my concern is not confusing -Ir with -I -r. It is if a relative import of a folder just called 'r' can be specified as -Ir, do we need a different way to specify recursive searches, as the previous comment said -Ir PATH was the proposed flag?

For the expansion, it seems it works without the strings/echo, you just need the argparse nargs='*' option as the parameters are properly expanded in 'argv'.

[hiker]

I do not understand some of your responses, for the empty -I you say that it works, but the command above says test.py: error: argument -I: expected one argument. I think is just a matter of adding the argparse nargs='?' option?

Hmm - just a single -I with no parameters is an error, isn't it? Whatever happens next is unintended (I believe compilers might then take the next string ... which is likely -SOMETHING flag as include path, which doesn't exist. )

For the missing whitespace my concern is not confusing -Ir with -I -r. It is if a relative import of a folder just called 'r' can be specified as -Ir, do we need a different way to specify recursive searches, as the previous comment said -Ir PATH was the proposed flag?

Indeed, I thought I had tested it. Maybe I just forgot to copy this. But you are right:

./test.py  -Ir bla
argv is ['./test.py', '-Ir', 'bla']
Namespace(I=['r'], exclude=None, reverse=False, filename='bla')

But as you expected, if I add a -Ir definition as argument, it takes precedence, and we can't specify a relative path that's called r.

Any suggestions? We could just use -r (seems to be not used atm), or a long-term only (--include-recursive)?
I tried +I, but that doesn't work (clashes with -I). Or we could use -rI?

For the expansion, it seems it works without the strings/echo, you just need the argparse nargs='*' option as the parameters are properly expanded in 'argv'.

I tried that, but it "uses up" all other arguments till the next - that starts a new parameter. Since PSyclone expects a filename as last parameter, that means we can't have a -I as last parameter (before the last filename).

Re multiple paths as one parameter, we could either support a different format (space separate, but that wouldn't work with bash expansion - -I "a b c"; colon-separated -I a:b:c). or we just glob ourselves, which would work out of the box, but needs quotes -I "*.py". As far as I can see, we could actually support all three options (as long as the user has no files with a space or colon ;) )

[arporter]

I don't particularly like -Ir anyway so could we just use something else?

[hiker]

I'll start with -r and --recursive-include as long-form (which fits with -r as shortcut). Changing the name of the option is trivial to do in the review.