[bug#40426] Add g-golf

str1ngs and nly want it to be submitted to guix, and i was proudly take this task.  Package was copied "as-is", and tested as per Guix manual.
   There is desire to package be named "g-golf", and not as "guile-g-golf", as the package name stands for "Gnome: Guile Object Library For".

Sent with [ProtonMail](https://protonmail.com) Secure Email.

Vitaliy Shatrov via Guix-patches via writes:

> str1ngs and nly want it to be submitted to guix, and i was proudly take this task.  Package was copied "as-is", and tested as per Guix manual.
>    There is desire to package be named "g-golf", and not as "guile-g-golf", as the package name stands for "Gnome: Guile Object Library For".
>
> Sent with [ProtonMail](https://protonmail.com) Secure Email.


Thank you Vitaliy,

  I appreciate you submitting this patch for us.

  We've been using g-golf sometime in Nomad, though Nomad and g-golf are
still pretty much WIP. We thought it would be nice to get g-golf
included into guix as a preliminary for Nomad's next replease, which ports
most of the C code to scheme using g-golf.

We've been using this current declaration for sometime and it's been
working quite well so far.

If anyone has some feedbak on it please let us know.

Mike

Vitaliy Shatrov via Guix-patches via <guix-patches@gnu.org> writes:

> str1ngs and nly want it to be submitted to guix, and i was proudly
> take this task.  Package was copied "as-is", and tested as per Guix
> manual.  There is desire to package be named "g-golf", and not as
> "guile-g-golf", as the package name stands for "Gnome: Guile Object
> Library For".

Thanks for the patch,

> +(define-public g-golf
> +  (let ((commit "4a4edf25e4877df9182c77843bdd98ab59e13ef7"))
> +    (package
> +      (name "g-golf")
> +      (version (git-version "1" "683" commit))
> +      (source (origin
> +                (method git-fetch)
> +                (uri (git-reference
> +                      (url "https://git.savannah.gnu.org/git/g-golf.git")
> +                      (commit commit)))
> +                (file-name (git-file-name name version))
> +                (sha256
> +                 (base32
> +                  "09p0gf71wbmlm9kri693a8fvr9hl3hhlmlidyadwjdh7853xg0h8"))))
> +      (build-system gnu-build-system)
> +      (native-inputs
> +       `(("autoconf" ,autoconf)
> +         ("automake" ,automake)
> +         ("texinfo" ,texinfo)
> +         ("gettext" ,gettext-minimal)
> +         ("libtool" ,libtool)
> +         ("pkg-config" ,pkg-config)))
> +      (inputs
> +       `(("guile" ,guile-2.2)

Does g-golf work with Guile 3 yet? If not, that's OK.

> +         ("guile-lib" ,guile-lib)
> +         ("clutter" ,clutter)
> +         ("gtk" ,gtk+)
> +         ("glib" ,glib)))
> +      (propagated-inputs
> +       `(("gobject-introspection" ,gobject-introspection)))
> +      (arguments
> +       `(#:tests? #t

I'd remove the #:tests? argument given the default value of #t is fine.

> +         #:phases
> +         (modify-phases %standard-phases
> +           (add-before 'configure 'tests-work-arounds
> +             (lambda* (#:key inputs #:allow-other-keys)
> +               ;; In build environment, There is no /dev/tty
> +               (substitute*
> +                   "test-suite/tests/gobject.scm"
> +                 (("/dev/tty") "/dev/null"))))
> +           (add-before 'configure 'substitute-libs
> +             (lambda* (#:key inputs outputs #:allow-other-keys)
> +               (let* ((get (lambda (key lib)
> +                             (string-append (assoc-ref inputs key) "/lib/" lib)))
> +                      (libgi      (get "gobject-introspection" "libgirepository-1.0"))
> +                      (libglib    (get "glib" "libglib-2.0"))
> +                      (libgobject (get "glib" "libgobject-2.0"))
> +                      (libgdk     (get "gtk" "libgdk-3")))
> +                 (substitute* "configure"
> +                   (("SITEDIR=\"\\$datadir/g-golf\"")
> +                    "SITEDIR=\"$datadir/guile/site/$GUILE_EFFECTIVE_VERSION\"")
> +                   (("SITECCACHEDIR=\"\\$libdir/g-golf/")
> +                    "SITECCACHEDIR=\"$libdir/"))
> +                 (substitute* "g-golf/init.scm"
> +                   (("libgirepository-1.0") libgi)
> +                   (("libglib-2.0") libglib)
> +                   (("libgdk-3") libgdk)
> +                   (("libgobject-2.0") libgobject)
> +                   (("\\(dynamic-link \"libg-golf\"\\)")
> +                    (format #f "~s"
> +                            `(dynamic-link
> +                              (format #f "~alibg-golf"
> +                                      (if (getenv "GUILE_GGOLF_UNINSTALLED")
> +                                          ""
> +                                          ,(format #f "~a/lib/"
> +                                                   (assoc-ref outputs "out"))))))))
> +                 (setenv "GUILE_AUTO_COMPILE" "0")
> +                 (setenv "GUILE_GGOLF_UNINSTALLED" "1")

I don't quite follow this GUILE_GGOLF_UNINSTALLED environment
variable. Why not just use the absolute filename for the so file
(without the extension I think)?

Also, maybe delete the strip phase, as I don't think that does anything
apart from producing a load of warnings.

> +                 #t))))))
> +      (home-page "https://www.gnu.org/software/g-golf/")
> +      (synopsis "Guile bindings for GObject Introspection")
> +      (description
> +       "G-Golf (Gnome: (Guile Object Library for)) is a library for developing
> +applications in Guile Scheme.  It comprises a direct binding to the low level
> +GObject Introspection API and higher-level functionality for importing Gnome
> +libraries and making GObject classes (and methods) available in Guile's

This looks pretty good to me, just a few things to clear up :)

Thanks,

Chris

Christopher Baines writes:

> Vitaliy Shatrov via Guix-patches via <guix-patches@gnu.org> writes:
>
> Does g-golf work with Guile 3 yet? If not, that's OK.
>
Hello Christopher, thanks for looking at this.

  I did quick look at seeing if g-golf would work with guile 3.0. But I
ran into some issue with core module bindings.  Since g-golf very much
WIP and like wise Nomad which is my primary use for g-golf is also
WIP. I've kept strictly to guile 2.2 for now to maintain a little extra
stability. I'll follow up on it when both nomad a g-golf are more
stable.

>
> I'd remove the #:tests? argument given the default value of #t is fine.
>

  Not a problem either myself or Vitality will add a follow up
patch. This was orphaned since we just recently got tests working.

> I don't quite follow this GUILE_GGOLF_UNINSTALLED environment
> variable. Why not just use the absolute filename for the so file
> (without the extension I think)?


  The problem here is that libg-golf is needed both at compile time and
runtime.  So it can not be substituted say after the unpack stage. So
this just checks if GUILE_GGOLF_UNINSTALLED is set.  Then it will
use libg-golf with normal dynamic-link search paths.  Other wise it uses
the full store path.

  I had discussed this scenario with the g-golf author, his
recommendation was to use this approach it's the same one used for the
guile-cv declaration.  He's the author of guile-cv as well.

> Also, maybe delete the strip phase, as I don't think that does anything
> apart from producing a load of warnings.
>

   I think the strip phase strips dynamic elf libraries as well?  I
don't think it would hurt here to keep it for libg-golf at least?

  Would you also be able to look at bug#40512 Christopher? That one is a
trivial review just some upstream bug fixes for Emacsy and a hash bump
in the package declaration.  Technically not related to the patch. But
eventually I'll need both of these when I release the next version of
nomad.

Regards,

Mike

Mike Rosset <mike.rosset@gmail.com> writes:

> Christopher Baines writes:
>
>> Vitaliy Shatrov via Guix-patches via <guix-patches@gnu.org> writes:
>>
>> Does g-golf work with Guile 3 yet? If not, that's OK.
>>
> Hello Christopher, thanks for looking at this.
>
>   I did quick look at seeing if g-golf would work with guile 3.0. But I
> ran into some issue with core module bindings.  Since g-golf very much
> WIP and like wise Nomad which is my primary use for g-golf is also
> WIP. I've kept strictly to guile 2.2 for now to maintain a little extra
> stability. I'll follow up on it when both nomad a g-golf are more
> stable.

Sure, no problem.

>> I don't quite follow this GUILE_GGOLF_UNINSTALLED environment
>> variable. Why not just use the absolute filename for the so file
>> (without the extension I think)?
>
>
>   The problem here is that libg-golf is needed both at compile time and
> runtime.  So it can not be substituted say after the unpack stage. So
> this just checks if GUILE_GGOLF_UNINSTALLED is set.  Then it will
> use libg-golf with normal dynamic-link search paths.  Other wise it uses
> the full store path.
>
>   I had discussed this scenario with the g-golf author, his
> recommendation was to use this approach it's the same one used for the
> guile-cv declaration.  He's the author of guile-cv as well.

Right, so it's something to help build the Guix package. I think I
understand now.

>> Also, maybe delete the strip phase, as I don't think that does anything
>> apart from producing a load of warnings.
>>
>
>    I think the strip phase strips dynamic elf libraries as well?  I
> don't think it would hurt here to keep it for libg-golf at least?

Sure, it's OK to keep it as well.

>   Would you also be able to look at bug#40512 Christopher? That one is a
> trivial review just some upstream bug fixes for Emacsy and a hash bump
> in the package declaration.  Technically not related to the patch. But
> eventually I'll need both of these when I release the next version of
> nomad.

I've gone ahead and merged the emacsy-minimal update, I'll send an email
to that bug in a minute.

Thanks,

Chris

Vitaliy,

Vitaliy Shatrov via Guix-patches via 写道：
> str1ngs and nly want it to be submitted to guix, and i was 
> proudly take this task.  Package was copied "as-is", and tested 
> as per Guix manual.

Thank you!

> There is desire to package be named "g-golf", and not as 
> "guile-g-golf", as the package name stands for "Gnome: Guile 
> Object Library For".

Too clever for me :-)  It's a Guile library; hence the correct 
Guix name (and variable) is ‘guile-g-golf’.  We have plenty of 
‘python-pyfoo’ packages to keep it company.

> Subject: [PATCH] gnu: Add g-golf
>
> * gnu/packages/guile-xyz.scm (g-golf): New variable

Nitpick: both lines should end with a full stop.

> +(define-public g-golf

Could you add a comment here explaining why we use a git commit, 
instead of a release tarball or tag?  I assume there are none; 
that would do as comment.  However…

> +  (let ((commit "4a4edf25e4877df9182c77843bdd98ab59e13ef7"))
> +    (package
> +      (name "g-golf")
> +      (version (git-version "1" "683" commit))

…‘1’ means the project has released version 1 prior to this 
commit, or at least regards this commit as part of the ‘1’ series. 
I didn't spot any version number on the home page, NEWS file, git 
tags, …

If there is no ‘1’ release, use ‘0.0.0’.

The second field (REVISION) should be ‘0’, since this is the first 
*Guix* revision of this package.  The idea is that you increment 
the revision each time you change COMMIT, so Guix knows which 
commit is newer and can ‘guix package -u’ properly.

Since the 2 should be updated together, bind them together:

  (let ((commit "f00")
        (revision "0")) …

You obviously got ‘683’ from somewhere though.  Where?

> +       `(#:tests? #t

Does the guile-build-system disable tests by default?  (I skimmed 
the code but didn't find anything.)  Otherwise, this can be 
omitted.

> +         #:phases
> +         (modify-phases %standard-phases
> +           (add-before 'configure 'tests-work-arounds

Prefer ‘verb-thing’; makes it much easier to skim unfamiliar 
packages.

In this case we're not really working around the tests themselves, 
so I'd go with the boring ‘patch-tests’.

+             (lambda* (#:key inputs #:allow-other-keys)
+               ;; In build environment, There is no /dev/tty
+               (substitute*
+                   "test-suite/tests/gobject.scm"
+                 (("/dev/tty") "/dev/null"))))

For now, all phases must return #t.  SUBSTITUTE* doesn't, so we 
need

             (lambda …
               …
               (substitute* "test-suite/tests/gobject.scm"
                 (("/dev/tty") "/dev/null"))
               #t)

No need to put the file name on its own line here.

+           (add-before 'configure 'substitute-libs

Bytes are cheap: ‘libraries’.

Kind regards,

T G-R