GNU bug report logs - #40426
[PATCH] Add g-golf

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 40426 in the body.
You can then email your comments to 40426 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>
To: "guix-patches <at> gnu.org" <guix-patches <at> gnu.org>
Subject: [PATCH] Add g-golf
Date: Sat, 04 Apr 2020 11:58:15 +0000

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/html, inline)]

[0001-gnu-Add-g-golf.patch (text/x-patch, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Rosset <mike.rosset <at> gmail.com>
To: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>,
 Vitaliy Shatrov via Guix-patches <guix-patches <at> gnu.org>
Cc: 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Sat, 04 Apr 2020 19:56:01 -0700

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

Message #14 received at 40426 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>
Cc: 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Tue, 14 Apr 2020 19:10:37 +0100

[Message part 1 (text/plain, inline)]

Vitaliy Shatrov via Guix-patches via <guix-patches <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Rosset <mike.rosset <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>, 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Tue, 14 Apr 2020 22:56:24 -0700

Christopher Baines writes:

> Vitaliy Shatrov via Guix-patches via <guix-patches <at> 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

Message #23 received at 40426 <at> debbugs.gnu.org (full text, mbox):

From: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>
To: "40426 <at> debbugs.gnu.org" <40426 <at> debbugs.gnu.org>
Subject: v2, cleared
Date: Wed, 15 Apr 2020 07:15:59 +0000

[Message part 1 (text/plain, inline)]

tests #t -- removed;
trying another desc.

[Message part 2 (text/html, inline)]

[0001-gnu-Add-g-golf.patch (text/x-patch, attachment)]

Message #26 received at 40426 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Mike Rosset <mike.rosset <at> gmail.com>
Cc: D0dyBo0D0dyBo0 <at> protonmail.com, 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Wed, 15 Apr 2020 09:24:47 +0100

[Message part 1 (text/plain, inline)]

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

> Christopher Baines writes:
>
>> Vitaliy Shatrov via Guix-patches via <guix-patches <at> 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

[signature.asc (application/pgp-signature, inline)]

Message #31 received at 40426-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>
Cc: 40426-done <at> debbugs.gnu.org
Subject: Re: [bug#40426] v2, cleared
Date: Wed, 15 Apr 2020 13:22:43 +0100

[Message part 1 (text/plain, inline)]

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

> tests #t -- removed;
> trying another desc.

Thanks, I've gone ahead and pushed this to master as
dfe277a5ce60d487fe44840506206fea8507bc69.

I moved the package definition up a bit in the file, just so its not at
the bottom, which can help to avoid merge conflicts.

I also used the synopsis/description from the initial patch, mostly to
avoid the lint warnings.

Thanks again,

Chris

[signature.asc (application/pgp-signature, inline)]

Message #34 received at 40426 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>
Cc: 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Wed, 15 Apr 2020 15:57:03 +0200

[Message part 1 (text/plain, inline)]

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

[signature.asc (application/pgp-signature, inline)]

Message #37 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Rosset <mike.rosset <at> gmail.com>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Tobias Geerinckx-Rice via Guix-patches <guix-patches <at> gnu.org>
Cc: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>, 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Wed, 15 Apr 2020 09:47:01 -0700

[Message part 1 (text/plain, inline)]

Tobias Geerinckx-Rice via Guix-patches via writes:

> 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!

Tobias, this has already been merged with
dfe277a5ce60d487fe44840506206fea8507bc69. I will though address your
comments with this attached patch.

[0001-gnu-g-golf-Fix-version-to-0.0.0-0.4a4edf2.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

> 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.
>

No.  I can't change this to guile-g-golf.  The author of g-golf has
explicated stated they would like the package name to be 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…

I've added a comment for why git sources are used.

> …‘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?
>

This is fixed now, this got carried over from me tracking upstream
revisions. I've reset it to version 0.0.0 and revision 0. This is
probably the most important fix.

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

done

>
> 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)

done

> No need to put the file name on its own line here.
>
> +           (add-before 'configure 'substitute-libs

done

Message #43 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Mike Rosset <mike.rosset <at> gmail.com>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Tobias Geerinckx-Rice via Guix-patches <guix-patches <at> gnu.org>
Cc: Vitaliy Shatrov <D0dyBo0D0dyBo0 <at> protonmail.com>, 40426 <at> debbugs.gnu.org
Subject: Re: [bug#40426] [PATCH] Add g-golf
Date: Wed, 15 Apr 2020 16:02:33 -0700

[Message part 1 (text/plain, inline)]

Tobias Geerinckx-Rice via Guix-patches via writes:

>
> …‘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")) …


Slight change to my follow up patch, version should be 0.1.0.

[0001-gnu-g-golf-Fix-version-to-0.1.0-0.4a4edf2.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.