ganga-devs / ganga

Ganga is an easy-to-use frontend for job definition and management
GNU General Public License v3.0
100 stars 159 forks source link

Parsing arguments with spaces #170

Closed cmarinbe closed 4 years ago

cmarinbe commented 8 years ago

Ganga does not handle properly arguments with blank spaces in between, as in the case of optional arguments. For instance, in a Gaudi Python application:

application.args = ["arg", "-d", "-t optarg"]
application.script = ["script.py"]

is interpreted as:

script.py "arg" -d -t " optarg"

leading to a blank extra space at the beginning of string optional arguments.

While application.args = ["arg", "-d", "-t", "optarg"] seems to do the job, it would be helpful to handle the previous usage properly or at least to have a warning message when using blank spaces. See this discussion in the mailing-list for an example of a use case. This issue can be found in one of the latest e-mails, E-mailed: 01/02/2016 17:13, the previous ones are an example of the consequences, which were difficult to connect to this issue.

rob-c commented 8 years ago

I think what we should be doing is checking for this and throwing an exception here which isn't being done. This can potentially interfere with users wishing to send strings with a space in, but I think this is a minor use-case.

I'll see to adding something about this as I work on the new LHCb app.

rmatev commented 8 years ago

I think the most correct behaviour is to accept arguments with spaces, but not split them afterwards. A warning in case of spaces will be useful to prevent unintentional use.

olupton commented 8 years ago

To echo Rosen, I would not write off the use-case of arguments with spaces in; this is not uncommon.

Perhaps: .args = "a b c" -> use .split() and get ["a", "b", "c"] .args = ["a","b","c"] -> don't do anything .args = ["a b", "c"] -> print a warning but don't mangle anything

Keeps everyone happy.

rob-c commented 8 years ago

Without me digging through the linked discussion what is the expected behaviour and what should we issue a warning for?

rob-c commented 8 years ago

@olupton Just beet me to posting, thanks for providing a summary

milliams commented 8 years ago

I think we should try to follow the standard Python way whereby arguments with spaces are allowed. We shouldn't be doing things like ' '.join(args) but should instead pas them to Popen almost raw. This means if you want to call a script like:

ls -hl "/home/matt/dir with spaces/test"

then

args = ["-hl", "/home/matt/dir with spaces/test"]

should work correctly as Python should do the correct shell escaping for the second arg.

Remember that quotes in bash etc. are not 'strings' but simply turn on or off auto-escaping so "two words" is identical to two\ words.

rob-c commented 8 years ago

yes but we should still probably warn users against a daft mistake to avoid the time it takes for them to debug this going wrong.

egede commented 8 years ago

Before we change anything we should also check how Dirac deals with this. I seem to remember that is does something pretty daft (we had lots of trouble in the past with file names containing commas).

rob-c commented 8 years ago

I think a harmless warning to the user when setting the parameter should be all that is required here. If they know better they can ignore it, if there is a problem elsewhere in the code then we should deal with that as a separate issue imo