GNU bug report logs

#50349 [PATCH] packages: Add 'define-package' syntax.

PackageSource(s)Maintainer(s)
guix PTS Buildd Popcon
Reply or subscribe to this bug. View this bug as an mbox, status mbox, or maintainer mbox

Report forwarded to guix-patches@gnu.org:
bug#50349; Package guix-patches. (Fri, 03 Sep 2021 04:07:02 GMT) (full text, mbox, link).


Acknowledgement sent to Sarah Morgensen <iskarian@mgsn.dev>:
New bug report received and forwarded. Copy sent to guix-patches@gnu.org. (Fri, 03 Sep 2021 04:07:02 GMT) (full text, mbox, link).


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

From: Sarah Morgensen <iskarian@mgsn.dev>
To: guix-patches@gnu.org
Subject: [PATCH] packages: Add 'define-package' syntax.
Date: Thu, 2 Sep 2021 21:06:26 -0700
* guix/packages.scm (define-package): New syntax.
* .dir-locals.el: Add indent rule for 'define-package'.
* etc/indent-code.el (main): Adjust package search regex accordingly.
* etc/snippets/text-mode/guix-commit-message-add-cl-package: Likewise.
* etc/snippets/text-mode/guix-commit-message-add-package: Likewise.
* etc/snippets/text-mode/guix-commit-message-rename-package: Likewise.
* etc/snippets/text-mode/guix-commit-message-update-package: Likewise.
---
Hello Guix,

This patch adds a shorthand for "(define-public name (package ...))":

(define-package my-favorite-package
  (name "my-favorite-package")
  ...)

The purpose is primarily to save the horizontal indent, but IMO it looks
better, and is marginally more clear for newcomers.  I think ideally we could
eventually transition to using this syntax as the primary syntax and only use
'define-public' when necessary.

There are some downsides... it's one more form to keep track of, and 'let'
forms can't easily be used with it.

Since it's a syntax rule, it doesn't cause packages to rebuild (tested). I've
also tested the indentation rules, indent-code.el, and the snippets.

This probably deserves a documentation addition, but I wasn't sure where to
add it without confusing newcomers.  Suggestions welcome!

(If this is accepted, we'll also want to make a few changes to
emacs-guix/elisp/guix-devel.el, adding 'define-package' to
'guix-devel-keywords' and to 'guix-devel-scheme-indent' with level 1.)

What do you all think?
--
Sarah

 .dir-locals.el                                            | 1 +
 etc/indent-code.el                                        | 2 +-
 etc/snippets/text-mode/guix-commit-message-add-cl-package | 2 +-
 etc/snippets/text-mode/guix-commit-message-add-package    | 2 +-
 etc/snippets/text-mode/guix-commit-message-rename-package | 4 ++--
 etc/snippets/text-mode/guix-commit-message-update-package | 2 +-
 guix/packages.scm                                         | 8 ++++++++
 7 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/.dir-locals.el b/.dir-locals.el
index aaa48ab552..8141cf4fc2 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -71,6 +71,7 @@
    (eval . (put 'with-writable-file 'scheme-indent-function 2))
 
    (eval . (put 'package 'scheme-indent-function 0))
+   (eval . (put 'define-package 'scheme-indent-function 1))
    (eval . (put 'package/inherit 'scheme-indent-function 1))
    (eval . (put 'origin 'scheme-indent-function 0))
    (eval . (put 'build-system 'scheme-indent-function 0))
diff --git a/etc/indent-code.el b/etc/indent-code.el
index bdea8ee8bf..b83659f2f9 100755
--- a/etc/indent-code.el
+++ b/etc/indent-code.el
@@ -94,7 +94,7 @@
      ;; Indent the definition of PACKAGE-NAME in FILE-NAME.
      (find-file file-name)
      (goto-char (point-min))
-     (if (re-search-forward (concat "^(define\\(\\|-public\\) +"
+     (if (re-search-forward (concat "^(define\\(\\|-public\\|-package\\) +"
                                     package-name)
                             nil t)
          (let ((indent-tabs-mode nil))
diff --git a/etc/snippets/text-mode/guix-commit-message-add-cl-package b/etc/snippets/text-mode/guix-commit-message-add-cl-package
index e255736b05..eb0de677e7 100644
--- a/etc/snippets/text-mode/guix-commit-message-add-cl-package
+++ b/etc/snippets/text-mode/guix-commit-message-add-cl-package
@@ -7,7 +7,7 @@ gnu: Add ${1:`(with-temp-buffer
                 (magit-git-wash #'magit-diff-wash-diffs
                   "diff" "--staged")
                 (beginning-of-buffer)
-                (when (search-forward "+(define-public " nil 'noerror)
+                (when (re-search-forward "+(define-\\(public\\|package\\) " nil 'noerror)
                   (replace-regexp-in-string
 		   "^sbcl-" ""
 		   (thing-at-point 'sexp 'no-properties))))`}.
diff --git a/etc/snippets/text-mode/guix-commit-message-add-package b/etc/snippets/text-mode/guix-commit-message-add-package
index 7cebd4023a..11aeceb129 100644
--- a/etc/snippets/text-mode/guix-commit-message-add-package
+++ b/etc/snippets/text-mode/guix-commit-message-add-package
@@ -7,7 +7,7 @@ gnu: Add ${1:`(with-temp-buffer
                 (magit-git-wash #'magit-diff-wash-diffs
                   "diff" "--staged")
                 (goto-char (point-min))
-                (when (re-search-forward "\\+(define-public \\(\\S-+\\)" nil 'noerror)
+                (when (re-search-forward "\\+(define-\\(?:public\\|package\\) \\(\\S-+\\)" nil 'noerror)
                   (match-string-no-properties 1)))`}.
 
 * `(car (magit-staged-files))` ($1): New variable.
\ No newline at end of file
diff --git a/etc/snippets/text-mode/guix-commit-message-rename-package b/etc/snippets/text-mode/guix-commit-message-rename-package
index 9695ca1b3d..2315443573 100644
--- a/etc/snippets/text-mode/guix-commit-message-rename-package
+++ b/etc/snippets/text-mode/guix-commit-message-rename-package
@@ -7,12 +7,12 @@ gnu: ${1:`(with-temp-buffer
            (magit-git-wash #'magit-diff-wash-diffs
              "diff" "--staged")
            (beginning-of-buffer)
-           (when (search-forward "-(define-public " nil 'noerror)
+           (when (re-search-forward "-(define-\\(public\\|package\\) " nil 'noerror)
              (thing-at-point 'sexp 'no-properties)))`}: Rename package to ${2:`(with-temp-buffer
            (magit-git-wash #'magit-diff-wash-diffs
              "diff" "--staged")
            (beginning-of-buffer)
-           (when (search-forward "+(define-public " nil 'noerror)
+           (when (re-search-forward "+(define-\\(public\\|package\\) " nil 'noerror)
              (thing-at-point 'sexp 'no-properties)))`}.
 
 * `(car (magit-staged-files))` ($1): Define in terms of
diff --git a/etc/snippets/text-mode/guix-commit-message-update-package b/etc/snippets/text-mode/guix-commit-message-update-package
index f187419aa2..1d39e28b77 100644
--- a/etc/snippets/text-mode/guix-commit-message-update-package
+++ b/etc/snippets/text-mode/guix-commit-message-update-package
@@ -8,7 +8,7 @@ gnu: ${1:`(with-temp-buffer
            (magit-git-wash #'magit-diff-wash-diffs
              "diff" "--staged")
            (goto-char (point-min))
-           (when (re-search-forward "(define-public \\(\\S-+\\)" nil 'noerror)
+           (when (re-search-forward "(define-\\(?:public\\|package\\) \\(\\S-+\\)" nil 'noerror)
              (match-string-no-properties 1)))`}: Update to ${2:`(with-temp-buffer
     (magit-git-wash #'magit-diff-wash-diffs
       "diff" "--staged")
diff --git a/guix/packages.scm b/guix/packages.scm
index c825f427d8..ecd0b7e47d 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -6,6 +6,7 @@
 ;;; Copyright © 2017, 2019, 2020 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2019 Marius Bakke <mbakke@fastmail.com>
 ;;; Copyright © 2021 Chris Marusich <cmmarusich@gmail.com>
+;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -99,6 +100,7 @@ (define-module (guix packages)
             package-supported-systems
             package-properties
             package-location
+            define-package
             hidden-package
             hidden-package?
             package-superseded
@@ -425,6 +427,12 @@ (define-record-type* <package>
                                                        package)
                                                       16)))))
 
+(define-syntax-rule (define-package name body ...)
+  "Equivalent to (define-public name (package body ...))."
+  (define-public name
+    (package
+      body ...)))
+
 (define-syntax-rule (package/inherit p overrides ...)
   "Like (package (inherit P) OVERRIDES ...), except that the same
 transformation is done to the package P's replacement, if any.  P must be a bare

base-commit: 95c29d2746943733cbe8df7013854d45bb0df413
-- 
2.31.1





Information forwarded to guix-patches@gnu.org:
bug#50349; Package guix-patches. (Fri, 03 Sep 2021 05:58:02 GMT) (full text, mbox, link).


Message #8 received at 50349@debbugs.gnu.org (full text, mbox, reply):

From: zimoun <zimon.toutoune@gmail.com>
To: Sarah Morgensen <iskarian@mgsn.dev>, 50349@debbugs.gnu.org
Subject: Re: [bug#50349] [PATCH] packages: Add 'define-package' syntax.
Date: Fri, 03 Sep 2021 07:41:18 +0200
Hi Sarah,

On Thu, 02 Sep 2021 at 21:06, Sarah Morgensen <iskarian@mgsn.dev> wrote:

> (define-package my-favorite-package
>   (name "my-favorite-package")
>   ...)
>
> The purpose is primarily to save the horizontal indent, but IMO it looks
> better, and is marginally more clear for newcomers.  I think ideally we could
> eventually transition to using this syntax as the primary syntax and only use
> 'define-public' when necessary.

On one hand, I think it is a good idea; especially for newcomers.  On
the other hand, it will break ’git-blame’, isn’t it?

Therefore, I am not convinced such change is worth for ’gnu/packages/’.
Instead it seems worth only for teaching custom packages.  Explaining to
people in my labs, they are often confused between ’define’ and
’define-public’.  But then, there is two “styles” and people could be
more confused.

Well, my feelings are mixed.  Thanks for opening the discussion. :-)


Cheers,
simon




Information forwarded to guix-patches@gnu.org:
bug#50349; Package guix-patches. (Fri, 03 Sep 2021 21:57:02 GMT) (full text, mbox, link).


Message #11 received at 50349@debbugs.gnu.org (full text, mbox, reply):

From: Sarah Morgensen <iskarian@mgsn.dev>
To: zimoun <zimon.toutoune@gmail.com>
Cc: 50349@debbugs.gnu.org
Subject: Re: [bug#50349] [PATCH] packages: Add 'define-package' syntax.
Date: Fri, 03 Sep 2021 14:56:36 -0700
Hi,

zimoun <zimon.toutoune@gmail.com> writes:

> Hi Sarah,
>
> On Thu, 02 Sep 2021 at 21:06, Sarah Morgensen <iskarian@mgsn.dev> wrote:
>
>> (define-package my-favorite-package
>>   (name "my-favorite-package")
>>   ...)
>>
>> The purpose is primarily to save the horizontal indent, but IMO it looks
>> better, and is marginally more clear for newcomers.  I think ideally we could
>> eventually transition to using this syntax as the primary syntax and only use
>> 'define-public' when necessary.
>
> On one hand, I think it is a good idea; especially for newcomers.  On
> the other hand, it will break ’git-blame’, isn’t it?

Yes, there would be a one-time discontinuity.  Reformats like this can
be ignored with the `--ignore-ref' option, or with a file and a config
option:

.git-blame-ignore-revs:
--8<---------------cut here---------------start------------->8---
# Convert 'define-public' forms to 'define-package' forms
15d01b32313f5f2f291b120597719ae92bd26acd
--8<---------------cut here---------------end--------------->8---

.git/config:
--8<---------------cut here---------------start------------->8---
[blame]
        ignoreRevsFile = .git-blame-ignore-revs
--8<---------------cut here---------------end--------------->8---

We could include the latter in e.g. a `.gitconfig' file committed to the
repo, but in order to use config settings from the file, users would
have to first run

  git config --local include.path ../.gitconfig

Thankfully, this only has to be done once per clone.  If you really
wanted to make sure it got run, I suppose you could add it to a 'make'
target.

--
Sarah




Information forwarded to guix-patches@gnu.org:
bug#50349; Package guix-patches. (Sat, 04 Sep 2021 08:43:01 GMT) (full text, mbox, link).


Message #14 received at 50349@debbugs.gnu.org (full text, mbox, reply):

From: Maxime Devos <maximedevos@telenet.be>
To: Sarah Morgensen <iskarian@mgsn.dev>, 50349@debbugs.gnu.org
Subject: Re: [bug#50349] [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 04 Sep 2021 10:42:02 +0200
[Message part 1 (text/plain, inline)]
Sarah Morgensen schreef op do 02-09-2021 om 21:06 [-0700]:
> Hello Guix,
> 
> This patch adds a shorthand for "(define-public name (package ...))":
> 
> (define-package my-favorite-package
>   (name "my-favorite-package")
>   ...)

This could be even shorter in the special case that the variable name
and package name are the same (modulo types):

(define-package "my-favorite-package"
  (version ...)
  ...)

'datum->syntax' and 'string->symbol' can be used to turn "my-favorite-package"
into an identifier.

A 'define-unexported-package' might be required in some places.

> The purpose is primarily to save the horizontal indent, but IMO it looks
> better, and is marginally more clear for newcomers.  I think ideally we could
> eventually transition to using this syntax as the primary syntax and only use
> 'define-public' when necessary.
> 
> There are some downsides... it's one more form to keep track of, and 'let'
> forms can't easily be used with it.
> 
> Since it's a syntax rule, it doesn't cause packages to rebuild (tested). I've
> also tested the indentation rules, indent-code.el, and the snippets.
> 
> This probably deserves a documentation addition, but I wasn't sure where to
> add it without confusing newcomers.  Suggestions welcome!

‘Defining Packages’ would be a good place I think.

> What do you all think?

This looks nice to me.  IIUC, the define-package is intended to be clearer
to newcomers, so you might want to ask for feedback on the new syntax on
help-guix@gnu.org.

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

Severity set to 'wishlist' from 'normal' Request was from Tobias Geerinckx-Rice <me@tobias.gr> to control@debbugs.gnu.org. (Sat, 04 Sep 2021 10:21:01 GMT) (full text, mbox, link).


bug reassigned from package 'guix-patches' to 'guix'. Request was from Tobias Geerinckx-Rice <me@tobias.gr> to control@debbugs.gnu.org. (Sat, 04 Sep 2021 10:21:01 GMT) (full text, mbox, link).


Merged 15284 50349. Request was from Tobias Geerinckx-Rice <me@tobias.gr> to control@debbugs.gnu.org. (Sat, 04 Sep 2021 10:21:01 GMT) (full text, mbox, link).


Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 10:31:02 GMT) (full text, mbox, link).


Message #23 received at submit@debbugs.gnu.org (full text, mbox, reply):

From: Tobias Geerinckx-Rice <me@tobias.gr>
To: Maxime Devos <maximedevos@telenet.be>
Cc: Sarah Morgensen <iskarian@mgsn.dev>, 50349@debbugs.gnu.org, guix-patches@gnu.org
Subject: Re: [bug#50349] [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 04 Sep 2021 12:09:26 +0200
[Message part 1 (text/plain, inline)]
All,

To keep a link with previous ‘define-package’ discussion, I've 
merged this bug with #15284.  It was never resolved IMO and things 
have changed since 2013 with the label-less input style.

Maxime Devos 写道:
> This could be even shorter in the special case that the variable 
> name
> and package name are the same (modulo types):
>
> (define-package "my-favorite-package"
>   (version ...)
>   ...)

(define-anything STRING ...) is just too weird to ack.  Are there 
any package names that aren't currently valid symbols?  Is there a 
good reason for them?

Kind regards,

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

Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 10:31:02 GMT) (full text, mbox, link).


Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 14:30:02 GMT) (full text, mbox, link).


Message #29 received at submit@debbugs.gnu.org (full text, mbox, reply):

From: Taylan Kammer <taylan.kammer@gmail.com>
To: Tobias Geerinckx-Rice <me@tobias.gr>, Maxime Devos <maximedevos@telenet.be>
Cc: Sarah Morgensen <iskarian@mgsn.dev>, 50349@debbugs.gnu.org, guix-patches@gnu.org
Subject: Re: bug#50349: [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 4 Sep 2021 16:29:49 +0200
On 04.09.2021 12:09, Tobias Geerinckx-Rice via Bug reports for GNU Guix wrote:
> All,
> 
> To keep a link with previous ‘define-package’ discussion, I've merged this bug with #15284.  It was never resolved IMO and things have changed since 2013 with the label-less input style.
> 
> Maxime Devos 写道:
>> This could be even shorter in the special case that the variable name
>> and package name are the same (modulo types):
>>
>> (define-package "my-favorite-package"
>>   (version ...)
>>   ...)
> 
> (define-anything STRING ...) is just too weird to ack.  Are there any package names that aren't currently valid symbols?  Is there a good reason for them?
> 
> Kind regards,
> 
> T G-R

To me the most obvious thing to do seems

  (define-package foo ...)  ;no explicit name needed

to bind the variable 'foo' and use symbol->string for the name of the
package, with the possibility to override the name like

  (define-package foo (name "foobar") ...)

which would bind the variable 'foo' to a package named "foobar".

Here's a syntax-case definition:

  (define-syntax define-package
    (lambda (stx)
      (syntax-case stx ()
        ((_ <name>
           (<field> <value> ...)
           ...)
         (if (memq 'name (map syntax->datum #'(<field> ...)))
             #'(define-public <name>
                 (package
                   (<field> <value> ...)
                   ...))
             #`(define-public <name>
                 (package
                   (name #,(symbol->string (syntax->datum #'<name>)))
                   (<field> <value> ...)
                   ...)))))))

-- 
Taylan




Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 14:30:02 GMT) (full text, mbox, link).


Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 15:02:02 GMT) (full text, mbox, link).


Message #35 received at submit@debbugs.gnu.org (full text, mbox, reply):

From: Tobias Geerinckx-Rice <me@tobias.gr>
To: Taylan Kammer <taylan.kammer@gmail.com>
Cc: 50349@debbugs.gnu.org, Sarah Morgensen <iskarian@mgsn.dev>, Maxime Devos <maximedevos@telenet.be>, guix-patches@gnu.org
Subject: Re: bug#50349: [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 04 Sep 2021 16:44:49 +0200
[Message part 1 (text/plain, inline)]
Taylan Kammer 写道:
> To me the most obvious thing to do seems
>
>   (define-package foo ...)  ;no explicit name needed
>
> to bind the variable 'foo' and use symbol->string for the name 
> of the
> package, with the possibility to override the name like
>
>   (define-package foo (name "foobar") ...)
>
> which would bind the variable 'foo' to a package named "foobar".

Right, that's what I meant, and it's how I read bug #15284, and it 
looks remarkably like the form I use in my personal channels (and 
I'm sure I'm not the first! :-).

You're much better at the language/implementation side of things 
than I am, Taylan.  Would this negatively affect performance 
(including ‘guix pull’ compilation)?

Kind regards,

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

Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 15:02:02 GMT) (full text, mbox, link).


Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 17:24:01 GMT) (full text, mbox, link).


Message #41 received at submit@debbugs.gnu.org (full text, mbox, reply):

From: Taylan Kammer <taylan.kammer@gmail.com>
To: Tobias Geerinckx-Rice <me@tobias.gr>
Cc: 50349@debbugs.gnu.org, Sarah Morgensen <iskarian@mgsn.dev>, Maxime Devos <maximedevos@telenet.be>, guix-patches@gnu.org
Subject: Re: bug#50349: [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 4 Sep 2021 19:23:29 +0200
[Message part 1 (text/plain, inline)]
On 04.09.2021 16:44, Tobias Geerinckx-Rice wrote:
> Taylan Kammer 写道:
>> To me the most obvious thing to do seems
>>
>>   (define-package foo ...)  ;no explicit name needed
>>
>> to bind the variable 'foo' and use symbol->string for the name of the
>> package, with the possibility to override the name like
>>
>>   (define-package foo (name "foobar") ...)
>>
>> which would bind the variable 'foo' to a package named "foobar".
> 
> Right, that's what I meant, and it's how I read bug #15284, and it looks remarkably like the form I use in my personal channels (and I'm sure I'm not the first! :-).
> 
> You're much better at the language/implementation side of things than I am, Taylan.  Would this negatively affect performance (including ‘guix pull’ compilation)?
> 
> Kind regards,
> 
> T G-R

I'm flattered, but don't really know the answer, so I decided to attempt
some sort of benchmark. :-P

test1.scm uses the syntax-case macro, test2.scm just define-public.

I don't actually import the Guix modules, so the (package ...) form isn't
macro-expanded, regardless of whether it's used directly or results from
expanding define-package.

This way, the impact of define-package should dominate the time difference.

The results are... interesting.  I started out with 256 definitions in the
files, and the define-package one would take about 4.2s to compile while
the regular one about 3.9s.  There was some jitter in the results though
after running it several times so I thought, let's increase the number of
packages to reduce noise.

With 512 packages, the one using regular define-public takes a whole
minute to compile, whereas the define-package one just ~14 seconds!

So no idea what's going on there, and how the use of this macro in the
actual (gnu packages ...) modules would affect performance. :-)

-- 
Taylan
[test1.scm (text/plain, attachment)]
[test2.scm (text/plain, attachment)]

Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 17:24:02 GMT) (full text, mbox, link).


Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 18:54:02 GMT) (full text, mbox, link).


Message #47 received at 50349@debbugs.gnu.org (full text, mbox, reply):

From: Sarah Morgensen <iskarian@mgsn.dev>
To: Taylan Kammer <taylan.kammer@gmail.com>
Cc: Maxime Devos <maximedevos@telenet.be>, Tobias Geerinckx-Rice <me@tobias.gr>, 50349@debbugs.gnu.org
Subject: Re: bug#50349: [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 04 Sep 2021 11:53:03 -0700
Hi Taylan,

Taylan Kammer <taylan.kammer@gmail.com> writes:

> On 04.09.2021 16:44, Tobias Geerinckx-Rice wrote:
>> Taylan Kammer 写道:
>>> To me the most obvious thing to do seems
>>>
>>>   (define-package foo ...)  ;no explicit name needed
>>>
>>> to bind the variable 'foo' and use symbol->string for the name of the
>>> package, with the possibility to override the name like
>>>
>>>   (define-package foo (name "foobar") ...)
>>>
>>> which would bind the variable 'foo' to a package named "foobar".
>> 
>> Right, that's what I meant, and it's how I read bug #15284, and it looks remarkably like the form I use in my personal channels (and I'm sure I'm not the first! :-).
>> 
>> You're much better at the language/implementation side of things than I am, Taylan.  Would this negatively affect performance (including ‘guix pull’ compilation)?
>> 
>> Kind regards,
>> 
>> T G-R

I agree; if we added that magic, that's definitely how it should be.

>
> I'm flattered, but don't really know the answer, so I decided to attempt
> some sort of benchmark. :-P
>
> test1.scm uses the syntax-case macro, test2.scm just define-public.
>
> I don't actually import the Guix modules, so the (package ...) form isn't
> macro-expanded, regardless of whether it's used directly or results from
> expanding define-package.
>
> This way, the impact of define-package should dominate the time difference.
>
> The results are... interesting.  I started out with 256 definitions in the
> files, and the define-package one would take about 4.2s to compile while
> the regular one about 3.9s.  There was some jitter in the results though
> after running it several times so I thought, let's increase the number of
> packages to reduce noise.
>
> With 512 packages, the one using regular define-public takes a whole
> minute to compile, whereas the define-package one just ~14 seconds!
>
> So no idea what's going on there, and how the use of this macro in the
> actual (gnu packages ...) modules would affect performance. :-)

Thanks for running some benchmarks. Were both those latter runs with a
warm cache?

If so, is it possible that due to a compilation optimization, many of
the global symbol lookups only happen once with the define-package
macro?

If you were really interested, I suppose you could test with compilation
optimization off, but I'd be more interested in the performance impact
with (guix packages) imported.

--
Sarah




Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 21:02:01 GMT) (full text, mbox, link).


Message #50 received at 50349@debbugs.gnu.org (full text, mbox, reply):

From: Taylan Kammer <taylan.kammer@gmail.com>
To: Sarah Morgensen <iskarian@mgsn.dev>
Cc: Maxime Devos <maximedevos@telenet.be>, Tobias Geerinckx-Rice <me@tobias.gr>, 50349@debbugs.gnu.org
Subject: Re: bug#50349: [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 4 Sep 2021 23:01:07 +0200
[Message part 1 (text/plain, inline)]
On 04.09.2021 20:53, Sarah Morgensen wrote:

> 
> If you were really interested, I suppose you could test with compilation
> optimization off, but I'd be more interested in the performance impact
> with (guix packages) imported.
> 
> --
> Sarah
> 

Good questions.  Let's see.  Caching shouldn't be an issue by the way since
I always time a command several times and make sure it's consistent.

With -O0 and -O1, both files take a negligible amount of time to compile,
approximately 0.25s and 0.3s.  A difference of 0.5s for 512 packages means
about 0.001s per package, which at 100K packages would be 100s.

That's without importing (guix packages) though.  When I import it, then
at -O0 and -O1 (I think these are equivalent), the define-packages one
takes about 3.8s and the regular one about 3.5s.  So the difference has
actually shrunk down to about 0.3s now.

With (guix packages) and no special optimization flag, the define-packages
one takes about 26s, and the regular one still shows the strange behavior
where the time explodes to over a minute.

If I remember correctly though, Guix uses -O1 to compile packages anyway.

So all in all I *think* we can say that the macro induces no important
performance hit.  (And could for some reason significantly improve it if
we compile a large chunk of packages on -O2...)

One thing I should note though is that I'm using a top-of-the-line typical
consumer CPU (Ryzen 9 3900X) so on an older machine, or a CPU brand that
puts more value into freedom and security than performance, the results
may be different.  I still doubt that the impact would be big.

Attached are new scm files since I had to add some fields to make sure
the package macro from (guix packages) doesn't abort the compilation.

-- 
Taylan
[test1.scm (text/plain, attachment)]
[test2.scm (text/plain, attachment)]

Information forwarded to bug-guix@gnu.org:
bug#50349; Package guix. (Sat, 04 Sep 2021 23:18:02 GMT) (full text, mbox, link).


Message #53 received at 50349@debbugs.gnu.org (full text, mbox, reply):

From: Sarah Morgensen <iskarian@mgsn.dev>
To: Maxime Devos <maximedevos@telenet.be>
Cc: Taylan Kammer <taylan.kammer@gmail.com>, Tobias Geerinckx-Rice <me@tobias.gr>, 50349@debbugs.gnu.org
Subject: Re: [bug#50349] [PATCH] packages: Add 'define-package' syntax.
Date: Sat, 04 Sep 2021 16:17:16 -0700
Hi all,

Thanks for your comments.  I'm replying specifically to this message but
these thoughts apply to the issue as a whole.

Maxime Devos <maximedevos@telenet.be> writes:

> Sarah Morgensen schreef op do 02-09-2021 om 21:06 [-0700]:
>> Hello Guix,
>> 
>> This patch adds a shorthand for "(define-public name (package ...))":
>> 
>> (define-package my-favorite-package
>>   (name "my-favorite-package")
>>   ...)
>
> This could be even shorter in the special case that the variable name
> and package name are the same (modulo types):
>
> (define-package "my-favorite-package"
>   (version ...)
>   ...)
>
> 'datum->syntax' and 'string->symbol' can be used to turn "my-favorite-package"
> into an identifier.
>
> A 'define-unexported-package' might be required in some places.

Sure, or perhaps 'define-private-package'.  I think this is pretty rare,
though?  And often in those cases other forms are used which may be
incompatible with the macro, so they'll have to use the original syntax
anyway.  Either way is fine IMO.

There are also about 150 packages which use 'package/inherit'.  Should
we introduce special syntax for them?  What about 'hidden-package'
(about 60 packages)?  (And 11 use both!)

About the only form I can think of that wouldn't break the composability
of these kinds of things is something like

  (define-package* hello (hidden)
    (name "hello")
    ...)

or

  (define-package* hello (hidden inherit-replacements)
    (name "hello")
    ...)

Where 'hidden', 'inherit-replacements', and so on would be procs to
apply (in the same order as compose?) that each transform the package.
Or we could even have them transform the package syntax directly.

But that's even more magic; it would take a fair amount of work, and be
hard to get right.  (How well would it hold up to syntax errors?)

>
> [...]
> This looks nice to me.  IIUC, the define-package is intended to be clearer
> to newcomers, so you might want to ask for feedback on the new syntax on
> help-guix@gnu.org.

Thanks for the suggestion, I definitely will.

With an eye toward newcomers, I think one "gotcha" of the "optional
name" version is inheritance.  If I have

(define-package rust-actix-0.10
  (name "rust-actix")
  ...)

and then I write

(define-package rust-actix-0.20
  (inherit rust-actix-0.10)
  ...)

At best, I would be unsure about whether this package would inherit the
name.  At worst, I would assume the name is inherited, and I would be
wrong.  If I have to write the name, there is no ambiguity.

For the automatic naming (because of gotchas like that), and for
possible extensions discussed above, I think right now I'm tempted to
agree with Ludo's comment when this last came around (thanks to Tobias
for pointing out the conversation)[0]:

ludo@gnu.org (Ludovic Courtès) writes:

> However, I prefer treating packages just like any other Scheme object,
> and to avoid introducing “magic” with macros like this.

I could be convinced with an elegant enough implementation though! ;)

[0] https://lists.gnu.org/archive/html/bug-guix/2013-09/msg00005.html
--
Sarah




Send a report that this bug log contains spam.


debbugs.gnu.org maintainers <help-debbugs@gnu.org>. Last modified: Sun Dec 22 11:06:08 2024; Machine Name: wallace-server

GNU bug tracking system

Debbugs is free software and licensed under the terms of the GNU Public License version 2. The current version can be obtained from https://bugs.debian.org/debbugs-source/.

Copyright © 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson, 2005-2017 Don Armstrong, and many other contributors.