Hello Wiktor,

Wiktor Żelazny <wz@freeshell.de> writes:

> From: Wiktor Żelazny <wzelazny@vurv.cz>
>
> * gnu/packages/geo.scm (grass): New variable.
> No GUI due to wxpython not found on runtime.
> ---
> Another GIS beast. This one builds, at least on my machine, but
>    $ grass78
>    Starting GRASS GIS...
>    ERROR: wxGUI requires wxPython. No module named 'wx'
> Perhaps this is something specific to Python module treatment by Guix.

No doubt!

> Only one of these
>    guix environment python python-wxpython
>    guix environment python --ad-hoc python-wxpython
>    guix environment --ad-hoc python python-wxpython
> , for instance, gives a desired result.

I'm guessing it's the latter?  :-)

The issue has to do with how Python modules are located at run-time.
Adding 'python' to the environment will populate the PYTHONPATH variable
with all packages in the profile that have a
'/lib/python3.7/site-packages' directory in its output.  Without
'python', Guix does not know that this variable should exist (see
<https://bugs.gnu.org/22138>).

To overcome this, the common approach is to wrap the executable with the
required variables, along these lines:

(wrap-program (string-append out "/bin/grass")
  `("PYTHONPATH" ":" prefix (,(string-append wxpython "/lib/python"
                                             (python-version python)
                                             "/site-packages"))))

Can you try that?

Some comments on the patch itself:

> +(define-public grass
> +  (package
> +    (name "grass")
> +    (version "7.8.0")
> +    (source
> +      (origin
> +        (method url-fetch)
> +        (uri (string-append
> +               "https://grass.osgeo.org/" name
> +               (match (string-split version #\.)
> +                      ((major minor _ ...)
> +                       (string-append major minor)))
> +               "/source/" name "-" version ".tar.gz"))
> +        ;; md5sum checked

We don't really need to acknowledge that we've verified checksums; it's
explicitly part of the packaging process per the "Submitting patches"
section of the manual.  Besides, MD5SUMs are not useful for security
these days, as it has become trivial to forge.

> +        (sha256
> +          (base32
> +            "1ynclznkpnm18vi0brmbfcplgi15yd7lwd424abgv7wm9qlr44ab"))))

Indentation seems to be off here.  If you are not using Emacs, please
run ./etc/indent-code.el on this file.

> +    (build-system gnu-build-system)
> +    (arguments
> +     '(#:phases
> +       (modify-phases %standard-phases
> +         ;; Replace configure phase as the ./configure script does not like
> +         ;; CONFIG_SHELL and SHELL passed as parameters
> +         (replace 'configure
> +           (lambda* (#:key outputs build target #:allow-other-keys)
> +             (let* ((out   (assoc-ref outputs "out"))
> +                    (bash  (which "bash"))
> +                    (flags `(,(string-append "--prefix=" out)

Note: you can use (list (string-append ...) ...) here instead of
quote/unquote.

> +                             ,(string-append "--build=" build)
> +                             ;; Fix `Unable to locate FreeType includes.'
> +                             ,(string-append
> +                                  "--with-freetype-includes="
> +                                  (assoc-ref %build-inputs "freetype")
> +                                  "/include/freetype2")
> +                             ;; Fix `Unable to locate PROJ data files.'

I'm not sure this (and the previous) comment add anything: it's fairly
obvious that these flags are here to tell the build system where to find
these files.

> +                             ,(string-append
> +                                  "--with-proj-share="
> +                                  (assoc-ref %build-inputs "proj.4")
> +                                  "/share/proj")
> +                             ,"--with-postgres")))
                                ^
This line does not need to be unquoted.

> +               (setenv "CONFIG_SHELL" bash)
> +               (apply invoke bash "./configure" flags)))))
> +        #:tests? #f))  ;no check target
> +    (native-inputs
> +      `(("flex" ,flex)
> +        ("bison" ,bison)
> +        ("pkg-config" ,pkg-config)
> +        ("python" ,python)
> +        ("python-six" ,python-six)))

I'm surprised to see python-six as a native-input, do you know what it's
used for?

> +    (inputs
> +      `(("proj.4" ,proj.4)
> +        ("gdal" ,gdal)
> +        ("zlib" ,zlib)
> +        ("zstd:lib" ,zstd "lib")
> +        ("libtiff" ,libtiff)
> +        ("libpng" ,libpng)
> +        ("sqlite" ,sqlite)
> +        ("freeglut" ,freeglut)
> +        ("fftw" ,fftw)
> +        ("cairo" ,cairo)
> +        ("freetype" ,freetype)
> +        ("libxt" ,libxt)
> +        ("python-wxpython" ,python-wxpython) ;; for gui
                                                ^
Nit-pick: Only one semicolon in margin comments.

Apart from these minor remarks, the patch looks great :-)