Opened 6 years ago

Closed 3 years ago

#2008 closed defect (fixed)

grass.script's find_program() can't find modules

Reported by: hamish Owned by: grass-dev@…
Priority: critical Milestone: 6.4.6
Component: Python Version: svn-releasebranch64
Keywords: find_program() Cc:
CPU: x86-64 Platform: All

Description

Hi,

I'm trying to use grass.script's find_program() function in 6.4.3svn to check if an addon module exists, but it always returns False, even for standard modules like r.sun. Testing for other programs on the computer seems to work ok.

?

thanks, Hamish

Change History (36)

comment:1 Changed 6 years ago by hamish

Resolution: invalid
Status: newclosed

actually it works, what I was missing what that the argument needed to be in [square] brackets. (why?)

6.4.3svn on linux:

 import grass.script as grass
 grass.find_program('r.sun,', ['help'])
 True

I will fix/sync the purely parallel-evolution :) version of the test in the r.hazard.flood script in grass6 addons; the exercise came about since I was trying to test python addons scripts in grass 6.4 (works on linux with g.extension, not on Windows due to .bat wrapper files and .py extension $0 != $0 fun with g.parser).

Hamish

comment:2 in reply to:  1 ; Changed 6 years ago by glynn

Replying to hamish:

actually it works, what I was missing what that the argument needed to be in [square] brackets. (why?)

Because it's a list of arguments to be passed to the program.

It would be trivial to change the function, i.e.

-def find_program(pgm, args = []):
+def find_program(pgm, *args):

but it would also be necessary to change anything which uses it with arguments, currently:

  • raster/r.colors/thumbnails.py
  • vector/v.colors/thumbnails.py
  • gui/wxpython/gui_core/gselect.py
  • gui/wxpython/gui_core/mapdisp.py
  • gui/wxpython/core/render.py
  • scripts/i.in.spotvgt/i.in.spotvgt.py
  • scripts/r.in.aster/r.in.aster.py
  • scripts/i.spectral/i.spectral.py
  • scripts/g.extension/g.extension.py
  • scripts/v.db.univar/v.db.univar.py

If it's going to be changed, sooner is better than later.

PS: there's no reason for r.colors and v.colors to have near-identical copies of the thumbnails.py script. It should be moved to tools.

comment:3 in reply to:  1 Changed 6 years ago by neteler

Platform: LinuxAll
Priority: normalcritical
Resolution: invalid
Status: closedreopened

Replying to hamish:

actually it works, what I was missing what that the argument needed to be in [square] brackets.

For me that fails even after make distclean:

Welcome to wxGUI Interactive Python Shell 0.9.8

Type "help(grass)" for more GRASS scripting related information.
Type "AddLayer()" to add raster or vector to the layer tree.

Python 2.7.3 (default, Aug  9 2012, 17:23:57) 
[GCC 4.7.1 20120720 (Red Hat 4.7.1-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.

import grass.script as grass
grass.find_program('r.sun,', ['help'])
False
grass.version()
{'build_date': '2013-05-19', 'proj4': '4.8.0', 'geos': '', 'sqlite': '3.7.13', 'libgis_revision': '56211', 'libgis_date': '2013-05-12 13:07:51 +0200 (Sun, 12 May 2013)', 'version': '7.0.svn', 'date': '2013', 'gdal': '1.9.1', 'revision': '56793M'}

comment:4 Changed 6 years ago by hamish

Glynn wrote:

+def find_program(pgm, *args):

would that allow it to be called like:

grass.find_program('r.sun', 'help')

? (I tried a quick test, it didn't like it)

the goal here is to avoid needless extra typing when the only possible extra options are the $@'s.

If it's going to be changed, sooner is better than later.

i.e. before 6.4.3 is released and we have to stay backwards compatible with the awkward version for the life of 6.x. I think it is still early enough to change it everything that uses it if we like to, but the window is closing fast.

Hamish wrote:

grass.find_program('r.sun,', help?)

MarkusN:

For me that fails even after make distclean:

sorry, I had a typo in it, 'r.sun' not 'r.sun,'.

another question with it: should it be changed to test if the program exists in the PATH and is executable, instead of testing if the program returns an exit code of 0? (e.g. "gdalwarp" with no options returns '1' but 'xml2' with no options returns '0')

Hamish

comment:5 in reply to:  2 ; Changed 6 years ago by zarch

Replying to glynn:

Replying to hamish:

actually it works, what I was missing what that the argument needed to be in [square] brackets. (why?)

Because it's a list of arguments to be passed to the program.

It would be trivial to change the function, i.e.

-def find_program(pgm, args = []):
+def find_program(pgm, *args):

Moreover insert a mutable object as default arguments it's dangerous and should be avoid, see the example below:

>>> def myfunc(mylist=[]):
...     mylist.append(5)
...     return mylist
...
>>> myfunc()
[5]
>>> myfunc()
[5, 5]
>>> myfunc()
[5, 5, 5]
>>> myfunc()
[5, 5, 5, 5]

Why not change the function in:

>>> def find_program(cmd):
...     return cmd in grass.get_commands()[0]
... 
>>> find_program('r.sun')
True

comment:6 in reply to:  5 ; Changed 6 years ago by hamish

Replying to zarch:

Why not change the function in:

>>> def find_program(cmd):
...     return cmd in grass.get_commands()[0]
... 
>>> find_program('r.sun')
True

because we want to use the function as a replacement for which, so find arbitrary helper apps like xml2 and gdalwarp which are not grass modules. Also, get_commands() is not very robust.

thanks, Hamish

comment:7 in reply to:  6 ; Changed 6 years ago by zarch

Replying to hamish:

Replying to zarch:

Why not change the function in:

>>> def find_program(cmd):
...     return cmd in grass.get_commands()[0]
... 
>>> find_program('r.sun')
True

because we want to use the function as a replacement for which, so find arbitrary helper apps like xml2 and gdalwarp which are not grass modules. Also, get_commands() is not very robust.

What about:

def which(pgm):
    for p in os.getenv('PATH').split(os.path.pathsep):
        p = os.path.join(p, pgm)
        if os.path.exists(p) and os.access(p, os.X_OK):
            return p

In python3.3 they add this function in "shutil.which"

http://hg.python.org/cpython/file/6860263c05b3/Lib/shutil.py#l1068

comment:8 in reply to:  7 ; Changed 6 years ago by annakrat

Replying to zarch:

What about:

def which(pgm):
    for p in os.getenv('PATH').split(os.path.pathsep):
        p = os.path.join(p, pgm)
        if os.path.exists(p) and os.access(p, os.X_OK):
            return p

In python3.3 they add this function in "shutil.which"

http://hg.python.org/cpython/file/6860263c05b3/Lib/shutil.py#l1068

The function from the link is more complicated, than what you suggest above. Why not to use the official one?

comment:9 in reply to:  8 ; Changed 6 years ago by zarch

Replying to annakrat:

http://hg.python.org/cpython/file/6860263c05b3/Lib/shutil.py#l1068

The function from the link is more complicated, than what you suggest above. Why not to use the official one?

Yes, I completely agree, the official one is much more save and it is the one that we should use.

My function was just a proof of concept and then I realize that the python developers already implemented it.

comment:10 in reply to:  9 ; Changed 6 years ago by annakrat

Replying to zarch:

Replying to annakrat:

http://hg.python.org/cpython/file/6860263c05b3/Lib/shutil.py#l1068

The function from the link is more complicated, than what you suggest above. Why not to use the official one?

Yes, I completely agree, the official one is much more save and it is the one that we should use.

My function was just a proof of concept and then I realize that the python developers already implemented it.

What about the name? I suggest to keep the old one (find_program) but it's not strong opinion.

comment:11 in reply to:  10 Changed 6 years ago by annakrat

Replying to annakrat:

Replying to zarch:

Replying to annakrat:

http://hg.python.org/cpython/file/6860263c05b3/Lib/shutil.py#l1068

The function from the link is more complicated, than what you suggest above. Why not to use the official one?

Yes, I completely agree, the official one is much more save and it is the one that we should use.

My function was just a proof of concept and then I realize that the python developers already implemented it.

What about the name? I suggest to keep the old one (find_program) but it's not strong opinion.

ok, please test new implementation of find_program (see link above) r56800 in grass 7. Changes applied also to addons (r56801).

comment:12 in reply to:  7 ; Changed 6 years ago by glynn

Replying to zarch:

What about:

def which(pgm):
    for p in os.getenv('PATH').split(os.path.pathsep):
        p = os.path.join(p, pgm)
        if os.path.exists(p) and os.access(p, os.X_OK):
            return p

That won't work on windows, where PATHEXT also has to be considered.

There are probably other cases which it doesn't handle, e.g. the executable exists but cannot be run for whatever reason. The previous implementation (which will be reinstated today unless someone provides a legitimate reason not to) is robust against such issues.

comment:13 in reply to:  4 Changed 6 years ago by glynn

Replying to hamish:

Glynn wrote:

+def find_program(pgm, *args):

would that allow it to be called like:

grass.find_program('r.sun', 'help')

Yes.

another question with it: should it be changed to test if the program exists in the PATH and is executable,

No.

instead of testing if the program returns an exit code of 0? (e.g. "gdalwarp" with no options returns '1' but 'xml2' with no options returns '0')

The exit code test can be changed if that's a problem. subprocess.call() will raise an OSError (with errno=ENOENT) if the executable doesn't exist.

comment:14 in reply to:  12 ; Changed 6 years ago by zarch

Replying to glynn:

[snip] That won't work on windows, where PATHEXT also has to be considered.

There are probably other cases which it doesn't handle, e.g. the executable exists but cannot be run for whatever reason. The previous implementation (which will be reinstated today unless someone provides a legitimate reason not to) is robust against such issues.

As I already said we don't have to use my implementation... But why not use the one developed by the python developers?

If the meaning of this function is just a python replacement for the equivalent "which" bash command. The implementation using Popen, it does not provide any information about where the program is, therefore is conceptually different from "which", and personally I find much more clean ask only for the program, instead of asking for the program AND one option/parameter...

Moreover asking only for the program is less prone to errors, that can be introduce by change of the command interface, and this is especially true if we use this function for library developed by others communities. But anyway it is not a strong opinion.

comment:15 in reply to:  14 ; Changed 6 years ago by annakrat

Replying to zarch:

If the meaning of this function is just a python replacement for the equivalent "which" bash command. The implementation using Popen, it does not provide any information about where the program is, therefore is conceptually different from "which", and personally I find much more clean ask only for the program, instead of asking for the program AND one option/parameter...

The 'which' implementation of function find_program more suits to the name. But in this case we probably want to only test if we can run it (and we don't want to find it).

Is there a way to get rid of the dummy parameters (like 'help') and keep the interface clean while keeping the previous implementation?

comment:16 in reply to:  15 ; Changed 6 years ago by zarch

Replying to annakrat:

The 'which' implementation of function find_program more suits to the name. But in this case we probably want to only test if we can run it (and we don't want to find it).

If you want to test if you can run the command or not, why not simply:

try:
    ret = self._runCommand(RunCommand, prog='g.proj',
                           read=True, flags='p')
except OSError:
    sys.exit(_("GRASS module '%s' not found. Unable to start map "
               "display window.") % 'g.proj')

instead of: http://trac.osgeo.org/grass/browser/grass/trunk/gui/wxpython/core/render.py?rev=56445#L454

comment:17 Changed 6 years ago by wenzeslaus

About the implementation choice, g.manual(.py) is currently not working on MS Win (7, 8) (probably, mainly) because it uses find_program to check for the web browser. Running 'explorer' without parameters or with invalid parameters (almost everything except for file names and URLs) returns non-zore return code, so find_program considers it as an error and gfatal in g.manual ends the manual with error. Moreover, it starts file explorer ('This computer' thing).

(Don't be confused with the system web browser in MS Win and explorer. The explorer program works like e.g. gnome open, it can run another app, in case of HTML page, and system web browser, e.g. Firefox.)

The result is that there is some group of programs which we cannot run without consequences, explorer on MS Win is one of them. We can either introduce another win-specific or even explorer specific conditions and hope that other programs behaves more like grass commands, or we can use implementation from Python authors.

I understand that with return code we simple know the most (e.g. the broken program), but it is necessary to have GRASS so robust, especially when there can be some consequences (such as opened window of explorer on MS Win)?

comment:18 in reply to:  14 Changed 6 years ago by glynn

Replying to zarch:

But why not use the one developed by the python developers?

Because it's an order of magnitude more complex than it needs to be, and less robust as a consequence. Also, it doesn't test whether the program actually works. An installed executable with missing shared libraries, an installed executable for the wrong architecture, or an installed script with a missing interpreter would all pass the new test. For a locked-down SELinux system, the correlation between whether the test passes and whether the program works could be quite close to zero.

The Python community often advocates the use of EAFP ("Easier to Ask for Forgiveness than Permission", i.e. try it and see if it works) in preference to LBYL ("Look Before You Leap", i.e. do lots of tests to try to predict whether something will work).

That principle doesn't appear to have been followed with shutil.which(). But then shutil.which() is trying to mimic the Unix "which" command, which isn't actually a test, it's a query for the filename corresponding to a command. What we want is a test.

If the meaning of this function is just a python replacement for the equivalent "which" bash command.

Not entirely. We want to know if a particular program is present and functional on the system. We don't care where it is, but we do care whether it works (which the replacement doesn't test).

Moreover asking only for the program is less prone to errors, that can be introduce by change of the command interface, and this is especially true if we use this function for library developed by others communities.

In order to test that the program works, and to perform the test with minimal side-effects, it's sometimes necessary to pass an argument (e.g. --help or --version). Running the program with no arguments may do too much (e.g. fire up a GUI).

comment:19 in reply to:  15 Changed 6 years ago by glynn

Replying to annakrat:

The 'which' implementation of function find_program more suits to the name. But in this case we probably want to only test if we can run it (and we don't want to find it).

Indeed.

Is there a way to get rid of the dummy parameters (like 'help') and keep the interface clean while keeping the previous implementation?

Some of the programs which are tested for need a parameter like "help" to make them return immediately rather than e.g. starting up a GUI. But if we ignore the exit code, we can probably remove the parameter from many of them (without e.g. a --help switch, many programs will complain about missing parameters and return a non-zero exit code).

comment:20 in reply to:  17 Changed 6 years ago by glynn

Replying to wenzeslaus:

The result is that there is some group of programs which we cannot run without consequences, explorer on MS Win is one of them. We can either introduce another win-specific or even explorer specific conditions and hope that other programs behaves more like grass commands, or we can use implementation from Python authors.

find_program('explorer') would be pointless; it can't fail to exist. Also, running explorer explicitly is probably wrong; a more usual approach is to just "execute" the document with shell=True, or use "rundll shell32 ...".

comment:21 in reply to:  16 ; Changed 6 years ago by glynn

Replying to zarch:

The 'which' implementation of function find_program more suits to the name. But in this case we probably want to only test if we can run it (and we don't want to find it).

If you want to test if you can run the command or not, why not simply:

try:
    ret = self._runCommand(RunCommand, prog='g.proj',

What is self._runCommand? Or what is "self" for that matter? find_program() is a function, not a method. And it isn't part of the wxGUI, so it can't use any functions from it.

comment:22 in reply to:  21 Changed 6 years ago by zarch

Replying to glynn:

What is self._runCommand? Or what is "self" for that matter? find_program() is a function, not a method. And it isn't part of the wxGUI, so it can't use any functions from it.

In my example I'm not using any method... I only replace the function "find_program" with "try", in the example that you can find: http://trac.osgeo.org/grass/browser/grass/trunk/gui/wxpython/core/render.py?rev=56800#L454

Index: gui/wxpython/core/render.py
===================================================================
--- gui/wxpython/core/render.py (revision 56810)
+++ gui/wxpython/core/render.py (working copy)
@@ -451,12 +451,13 @@
         """!Return region projection and map units information
         """
         projinfo = dict()
-        if not grass.find_program('g.proj'):
-            sys.exit(_("GRASS module '%s' not found. Unable to start map "
-                       "display window.") % 'g.proj')
         
+        try:
         ret = self._runCommand(RunCommand, prog = 'g.proj',
                                read = True, flags = 'p')
+        except OSError:
+            sys.exit(_("GRASS module '%s' not found. Unable to start map "
+                       "display window.") % 'g.proj')
         
         if not ret:
             return projinfo

As you see probably we don't need to define a new function...

comment:23 in reply to:  2 Changed 6 years ago by glynn

Replying to glynn:

It would be trivial to change the function, i.e.

-def find_program(pgm, args = []):
+def find_program(pgm, *args):

Done in r56867.

Also, it now ignores the exit code, so it shouldn't be necessary to pass additional arguments unless they're necessary to prevent the program from e.g. firing up a GUI.

comment:24 Changed 6 years ago by wenzeslaus

I've committed the small fix for r56867 in r56869 (Python says can only concatenate list (not "tuple") to list).

There is except without OSError, mentioned before, is there any reason for this?

I was about to commit the special case for the explorer cmd on MS Win. However, there is also a special case for xdg-open, I'm not sure why. Moreover, I tested also other command and e.g. firefox blocks the g.manual (it also blocks the cmd line when launched without &). On the other hand, in case of g.manual it opens the requested page, so it means that it goes behind the call function in find_program. This confuses me, I'would expect that the wait() call in call function implementation should block the interpreter sooner (i.e. in find_program).

We probably cannot find safe (help or version) arguments for all possible web browsers.

comment:25 in reply to:  24 ; Changed 6 years ago by annakrat

Replying to wenzeslaus:

We probably cannot find safe (help or version) arguments for all possible web browsers.

Why not to have two functions: find_program - behaves like which command, test_program - or something similar which tests the program by calling it. For web browsers we could use the which implementation.

comment:26 in reply to:  25 Changed 6 years ago by wenzeslaus

Replying to wenzeslaus:

I was about to commit the special case for the explorer cmd on MS Win. However, there is also a special case for xdg-open, I'm not sure why. Moreover, I tested also other command and e.g. firefox blocks the g.manual (it also blocks the cmd line when launched without &). On the other hand, in case of g.manual it opens the requested page, so it means that it goes behind the call function in find_program. This confuses me, I'would expect that the wait() call in call function implementation should block the interpreter sooner (i.e. in find_program).

I tested it again and I probably did some mistake before. firefox and chromium-browser blocks g.manual in find_program (browser starts empty). xdg-open, gnome-open and kde-open works well.

I still don't know why there was special case for xdg-open.

Alternatively, we don't need to do find_program/test_program for browser, we can just run and report only after os.execlp (now there is an error message anyway).

Replying to annakrat:

Why not to have two functions: find_program - behaves like which command, test_program - or something similar which tests the program by calling it. For web browsers we could use the which implementation.

Maybe, also the third one for test_(grass_)module for GRASS modules? (Than, we would use test_module('r.sun') instead of test_program('r.sun', '--help')) The point is that we need special cases anyway, so why not to make it explicit.

comment:27 Changed 6 years ago by wenzeslaus

After playing with g.manual longer, I think that the safest way is to not test browser at all and just run it as I suggested before.

In r56871 (which is otherwise neutral to this ticked) I've fixed use of os.execlp by using try-except, so g.manual is ready for deletion of the find_program test if there are no objections.

comment:28 in reply to:  24 ; Changed 6 years ago by glynn

Replying to wenzeslaus:

I've committed the small fix for r56867 in r56869 (Python says can only concatenate list (not "tuple") to list).

Er, right.

There is except without OSError, mentioned before, is there any reason for this?

Nothing particular. It's mainly a case of determining "works" versus "doesn't work" without caring about the reasons for the latter. If other errors are possible, should the caller be concerned with them? I.e. should we just allow exceptions other than OSError to propagate up to the caller? In any case, it should probably be restricted at least to either StandardError or Exception, so that it doesn't catch KeyboardInterrupt.

I was about to commit the special case for the explorer cmd on MS Win. However, there is also a special case for xdg-open, I'm not sure why.

I think that the "browser != 'xdg-open'" check is just because xdg-open returns a non-zero exit code when run without arguments.

Moreover, I tested also other command and e.g. firefox blocks the g.manual (it also blocks the cmd line when launched without &).

I don't think that find_program() is appropriate for GUI applications. E.g. on Windows, many of them will spawn a GUI regardless of any arguments given.

More generally, there should probably be separate functions for "opening" a file or URL in the user's desktop environment.

comment:29 in reply to:  28 ; Changed 6 years ago by wenzeslaus

Replying to glynn:

Replying to wenzeslaus:

I was about to commit the special case for the explorer cmd on MS Win. However, there is also a special case for xdg-open, I'm not sure why.

I think that the "browser != 'xdg-open'" check is just because xdg-open returns a non-zero exit code when run without arguments.

Moreover, I tested also other command and e.g. firefox blocks the g.manual (it also blocks the cmd line when launched without &).

I don't think that find_program() is appropriate for GUI applications. E.g. on Windows, many of them will spawn a GUI regardless of any arguments given.

Yes, this would lead to conservative shutil.which which just looks.

More generally, there should probably be separate functions for "opening" a file or URL in the user's desktop environment.

Sorry, I don't get your last point. Would you like to have open_something function in GRASS or do you expect this function to be available from system or something else?

Anyway, in r56880 I have deleted the test in g.manual since I don't see any reason for it. The execlp function tests it sufficiently without consequences and moreover it is Pythonic EAFP.

Considering r56880, we don't need the shutil.which now, but we may add it for/in the future.

Diverging from the original ticked topic, the GRASS_HTML_BROWSER=firefox blocks the command line even after r56880 when no firefox session is running. However, this is expected and I think there is no way how to avoid this when we want (probably because of cmd line web browsers) to use execlp.

Ticked summary

In r56880, g.manual is not using find_program or any other test function.

Some special apps (especially those with GUI) may require shutil.which to avoid strange situations.

It could be convenient (for both the interface and implementation) to have special function to test presents of GRASS modules.

The interface to of find_program was changed to be less error-prone in r56867 (and r56869).

...other things were invalid, if not or if I miss something please add.

comment:30 in reply to:  29 ; Changed 6 years ago by hamish

Replying to wenzeslaus:

Anyway, in r56880 I have deleted the test in g.manual since I don't see any reason for it.

In r56882 I have put it back. It is nice to have the clearer error message on Linux (where the error could come up a lot), and in cases where GRASS_HTML_BROWSER is either unset of malformed. It may need to be improved or fixed, but it should be there.

No idea about the reasoning for special xdg-open tests. But not understanding is a reason to keep it there, not to delete it.

Glynn:

I don't think that find_program() is appropriate for GUI applications.

mmmph, it either works or it doesn't. The use case shouldn't matter. e.g. the r.in.wms wxPython wrapper in GRASS 6 needs the xml2 program to be present, and needs to gracefully fail out with a suggestion to run the module from the command line if it isn't there.

wenzeslaus:

Diverging from the original ticked topic, the GRASS_HTML_BROWSER=firefox blocks the command line even after r56880 when no firefox session is running.

there was another ticket for that (probably several over the years), closed as invalid AFAIU. To mitigate the problem I guess we could white-list some known GUI browser names and selectively background them?

However, this is expected and I think there is no way how to avoid this when we want (probably because of cmd line web browsers) to use execlp.

exactly, mainly for links & lynx; although I suppose some rare but possible scripted html2txt/html2tex variants could exist.

regards, Hamish

comment:31 in reply to:  29 ; Changed 6 years ago by hamish

Replying to wenzeslaus:

It could be convenient (for both the interface and implementation) to have special function to test presents of GRASS modules.

just to note that the last version of that I saw failed to pick up Addon modules. (installed in either the GRASS_ADDON_PATH(s) &/or ~/bin/)

regards, Hamish

comment:32 in reply to:  29 Changed 6 years ago by glynn

Replying to wenzeslaus:

More generally, there should probably be separate functions for "opening" a file or URL in the user's desktop environment.

Sorry, I don't get your last point. Would you like to have open_something function in GRASS or do you expect this function to be available from system or something else?

I think that GRASS should have something. On Windows, this doesn't require testing for specific programs; subprocess.call(file, shell=True) should suffice.

Diverging from the original ticked topic, the GRASS_HTML_BROWSER=firefox blocks the command line even after r56880 when no firefox session is running. However, this is expected and I think there is no way how to avoid this when we want (probably because of cmd line web browsers) to use execlp.

GRASS_HTML_BROWSER is probably too simplistic. E.g. a GUI browser should be executed in the background, whereas e.g. lynx shouldn't. But something like xdg-open should probably be preferred.

comment:33 in reply to:  30 Changed 6 years ago by glynn

Replying to hamish:

No idea about the reasoning for special xdg-open tests. But not understanding is a reason to keep it there, not to delete it.

I'd say the opposite. Otherwise, we will continually accumulate cruft which no-one understands.

I don't think that find_program() is appropriate for GUI applications.

mmmph, it either works or it doesn't. The use case shouldn't matter. e.g. the r.in.wms wxPython wrapper in GRASS 6 needs the xml2 program to be present, and needs to gracefully fail out with a suggestion to run the module from the command line if it isn't there.

Just in case it wasn't clear, I mean that it's not appropriate for testing for the existence of GUI programs, not that it shouldn't be used from GUI programs.

A test requires a yes-or-no answer in a reasonable space of time. Attempting to test for a program which runs interactively either produces a quick "no" or it hangs indefinitely.

comment:34 in reply to:  31 Changed 6 years ago by wenzeslaus

find_program

Replying to hamish:

Replying to wenzeslaus:

It could be convenient (for both the interface and implementation) to have special function to test presents of GRASS modules.

just to note that the last version of that I saw failed to pick up Addon modules. (installed in either the GRASS_ADDON_PATH(s) &/or ~/bin/)

Did you test r56869? To be honest, I never remember if GRASS_ADDON_PATH is in PATH. If it is not in PATH, find_program (test/run_program) probably not work for addons. Maybe another reason for creating new function for modules. We can even call it is_module_present/here to create nicely readable code. You can provide your suggestions, especially those English-correct. The implementation is clear: same as current (glynn's) find_program (run the module), help parameter added and all GRASS acceptable paths used.

test in g.manual

Replying to hamish:

Replying to wenzeslaus:

Anyway, in r56880 I have deleted the test in g.manual since I don't see any reason for it.

In r56882 I have put it back. It is nice to have the clearer error message on Linux (where the error could come up a lot), and in cases where GRASS_HTML_BROWSER is either unset of malformed. It may need to be improved or fixed, but it should be there.

Tell me examples, please. I don't see much difference between "Browser '%s' not found" and "Error starting browser '%(browser)s' for HTML file '%(path)s".

Since the test (find_program) does not work for clear examples such as firefox, I don't see the reason to keep the test there to produce just slightly better error message in other cases (moreover, the meaning of the message is absolutely clear only if you know, which function/test caused this message, IMHO).

Anyway, if you think that the test is necessary, we can use the shutil.which function to do the test. Thus, we will have two functions, as already proposed by annakrat in comment:25.

opening HTML page

Replying to glynn:

Replying to wenzeslaus:

More generally, there should probably be separate functions for "opening" a file or URL in the user's desktop environment.

Sorry, I don't get your last point. Would you like to have open_something function in GRASS or do you expect this function to be available from system or something else?

I think that GRASS should have something. On Windows, this doesn't require testing for specific programs; subprocess.call(file, shell=True) should suffice.

Diverging from the original ticked topic, the GRASS_HTML_BROWSER=firefox blocks the command line even after r56880 when no firefox session is running. However, this is expected and I think there is no way how to avoid this when we want (probably because of cmd line web browsers) to use execlp.

GRASS_HTML_BROWSER is probably too simplistic. E.g. a GUI browser should be executed in the background, whereas e.g. lynx shouldn't. But something like xdg-open should probably be preferred.

As for the opening file topic, we shall create new ticked. However, for now, I have to say that I don't like the idea of re-implementation of the xdg-open-like functionality in GRASS, especially when primary backend will be something like xdg-open or standard windows functionality. Although I agree that the current state is insufficient.

comment:35 Changed 3 years ago by neteler

Milestone: 6.4.46.4.6

comment:36 Changed 3 years ago by mlennert

Resolution: fixed
Status: reopenedclosed

It seems to be that the original issue is solved. I can even find addons.

Closing as fixed. Please open another bug if some of the other issues discussed here are still valid.

Note: See TracTickets for help on using tickets.