Opened 3 years ago

Closed 12 months ago

#3170 closed defect (fixed)

GRASS_BATCH_JOB does not tolerate path with spaces

Reported by: marisn Owned by: wenzeslaus
Priority: trivial Milestone: 7.8.0
Component: Startup Version: svn-trunk
Keywords: grass.py init spaces exec GRASS_BATCH_JOB Popen shell=True Cc:
CPU: Unspecified Platform: Unspecified

Description

It is not possible to execute any GRASS_BATCH_JOB script form a path containing spaces.

Executing </home/maris/my way/gol_batch.sh>....
/bin/sh: /home/maris/my: No such file or directory

Quoting or shell escaping also doesn't work due to get_batch_job_from_env_variable checking file presence with os.access()

Workaround is to change line 1351 of grass.py to read:

proc = Popen('"' + batch_job + '"', shell=True)

As quoting of path is a delicate issue and there have been fights in other parts of GRASS with Popen, I'll better leave for more experienced GRASS Pythonists to decide if such workaround can be considered safe or more serious changes are needed.

Change History (10)

comment:1 Changed 2 years ago by martinl

Milestone: 7.2.17.2.2

comment:2 Changed 2 years ago by neteler

Milestone: 7.2.27.2.3

Ticket retargeted after milestone closed

comment:3 Changed 23 months ago by neteler

Is the suggested Python workaround a viable way to solve this ticket?

comment:4 Changed 18 months ago by martinl

Milestone: 7.2.3

Ticket retargeted after milestone closed

comment:5 Changed 18 months ago by martinl

Milestone: 7.2.4

comment:6 Changed 13 months ago by wenzeslaus

Keywords: spaces exec GRASS_BATCH_JOB Popen shell=True added
Milestone: 7.2.47.6.0
Owner: changed from grass-dev@… to wenzeslaus

For 7.2 and 7.4 (and maybe 7.6) the workarounds are:

  1. Don't use GRASS_BATCH_JOB, use more powerful --exec instead.
  2. Don't use spaces in the path. (And please let us know why do you prefer GRASS_BATCH_JOB rather over --exec.)

The spaces-in-path support can be implemented like this:

  • lib/init/grass.py

     
    15111511    batch_job_string = batch_job
    15121512    if not isinstance(batch_job, string_types):
    15131513        batch_job_string = ' '.join(batch_job)
    15141514    message(_("Executing <%s> ...") % batch_job_string)
    15151515    if isinstance(batch_job, string_types):
     1516        def quote(string):
     1517            if '"' in string:
     1518                return "'%s'" % batch_job
     1519            else:
     1520                return '"%s"' % batch_job
     1521        batch_job = quote(batch_job)
    15161522        proc = Popen(batch_job, shell=True)
    15171523    else:
    15181524        try:

Here shell=True (which is splitting the command using spaces) is used because it was used with GRASS_BATCH_JOB in the past. --exec is not using shell=True and allows spaces. Note that for 8.0 there might be changes to GRASS_BATCH_JOB in order to make it more aligned with --exec (see wiki:Grass8Planning).

Milestone is 7.6 or 7.8.

comment:7 in reply to:  6 ; Changed 13 months ago by marisn

Replying to wenzeslaus:

And please let us know why do you prefer GRASS_BATCH_JOB rather over --exec.

To be honest – I do not remember what was the reason of using GRASS_BATCH_JOB and not --exec. May be just experimenting and stumbled on a bug (as it is a bug per se).

comment:8 in reply to:  7 Changed 13 months ago by wenzeslaus

Replying to marisn:

Replying to wenzeslaus:

And please let us know why do you prefer GRASS_BATCH_JOB rather over --exec.

To be honest – I do not remember what was the reason of using GRASS_BATCH_JOB and not --exec. May be just experimenting and stumbled on a bug (as it is a bug per se).

Thanks, makes sense.

comment:9 Changed 13 months ago by wenzeslaus

In 73261:

init: support spaces in path for GRASS_BATCH_JOB (see #3170)

comment:10 Changed 12 months ago by wenzeslaus

Milestone: 7.6.07.8.0
Priority: normaltrivial
Resolution: fixed
Status: newclosed

Since there is --exec (see above) and spaces are unlikely here, backporting this is more trouble than it is worth. Let's have this more tested and shipped in (or around) 7.8. (Thanks for creating that milestone, BTW.)

Note: See TracTickets for help on using tickets.