[bug#66436] [PATCH] doc: Add some guidelines for reviewing.

[bug#66436] [PATCH] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
* doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
information and document the 'reviewed-looks-good' usertag.

Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
---
 doc/contributing.texi | 53 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 864190b119..b09e9cc299 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -605,7 +606,7 @@ Packaging Guidelines
 * Version Numbers::             When the name is not enough.
 * Synopses and Descriptions::   Helping users find the right package.
 * Snippets versus Phases::      Whether to use a snippet, or a build phase.
-* Cyclic Module Dependencies::   Going full circle.
+* Cyclic Module Dependencies::  Going full circle.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -1972,7 +1973,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs@footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1985,9 +1991,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2005,6 +2011,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2237,6 +2246,40 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, building, linting and running other
+people's series and sharing your comments about your experience will
+give some confidence to committers that should result in the proposed
+change being merged faster.
+
+@cindex LGTM, Looks Good To Me
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  The following well understood/codified @acronym{LGTM,
+Looks Good To Me} phrases should be used to sign off as a reviewer,
+meaning you have reviewed the change and that it looks good to you:
+
+@enumerate
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with a @samp{This series LGTM!} to the cover page if it has
+one, or to the last patch of the series otherwise.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with @samp{LGTM!} to that commit
+message.
+@end enumerate
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 

base-commit: 619ff2fa1d5b60b5fe33216ca2d6219c04a5515b
-- 
2.41.0

[bug#66436] [PATCH] doc: Add some guidelines for reviewing.

[Ludovic Courtès]

Hi,

This looks great to me!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> +@cindex LGTM, Looks Good To Me
> +@cindex reviewing, guidelines
> +Review comments should be unambiguous; be as clear and explicit as you
> +can about what you think should be changed, ensuring the author can take
> +action on it.

I’d add a few guidelines here, and perhaps we can make it a bullet
list:

  1. Be clear and explicit about changes you are suggesting, ensuring
     the author can take action on it.  In particular, it is a good idea
     to explicitly ask for new revisions when you want it.

  2. Remain focused: do not change the scope of the work being reviewed.
     For example, if the contribution touches a code that follows a
     pattern deemed unwieldy, it would be unfair to ask the submitter to
     fix all occurrences of that pattern in the code; to put it simply,
     if an problem unrelated to the patch at hand was already there, do
     not ask the submitter to fix it.

  3. Ensure progress.  As they respond to review, submitters may submit
     new revisions of their changes; avoid requesting changes that you
     did not request in the previous round of comments.  Overall, the
     submitter should get a clear sense of progress; the number of items
     open for discussion should clearly decrease over time.

  4. Review is a discussion.  The submitter's and reviewer's views on
     how to achieve a particular change may not always be aligned.  As a
     reviewer, try hard to explain the rationale for suggestions you
     make, and to understand and take into account the submitter's
     motivation for doing things in a certain way.

  5. Aim for finalization.  Reviewing code is time-consuming.  Your goal
     as a reviewer is to put the process on a clear path towards
     integration, possibly with agreed-upon changes, or rejection, with
     a clear and mutually-understood reasoning.  Avoid leaving the
     review process in a lingering state with no clear way out.

I just made it up but I’m sure there are good review guidelines out
there that we could use as an example.

My 2¢,
Ludo’.

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
* doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
information and document the 'reviewed-looks-good' usertag.

Series-version: 2
Series-cc: ludo@gnu.org
Series-to: 66436@debbugs.gnu.org
Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
Co-authored-by: Ludovic Courtès <ludo@gnu.org>
---
v2: integrate guidelines suggested by Ludovic

 doc/contributing.texi | 93 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 88 insertions(+), 5 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 864190b119..b8a0839c7f 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -605,7 +606,7 @@ Packaging Guidelines
 * Version Numbers::             When the name is not enough.
 * Synopses and Descriptions::   Helping users find the right package.
 * Snippets versus Phases::      Whether to use a snippet, or a build phase.
-* Cyclic Module Dependencies::   Going full circle.
+* Cyclic Module Dependencies::  Going full circle.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -1972,7 +1973,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs@footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1985,9 +1991,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2005,6 +2011,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2237,6 +2246,80 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers, and should result in
+the proposed change being merged faster.
+
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  Please try to keep the following guidelines in mind
+during review:
+
+@enumerate
+@item
+@emph{Be clear and explicit about changes you are suggesting}, ensuring
+the author can take action on it.  In particular, it is a good idea to
+explicitly ask for new revisions when you want it.
+
+@item
+@emph{Remain focused: do not change the scope of the work being
+reviewed.}  For example, if the contribution touches code that follows a
+pattern deemed unwieldy, it would be unfair to ask the submitter to fix
+all occurrences of that pattern in the code; to put it simply, if a
+problem unrelated to the patch at hand was already there, do not ask the
+submitter to fix it.
+
+@item
+@emph{Ensure progress.}  As they respond to review, submitters may
+submit new revisions of their changes; avoid requesting changes that you
+did not request in the previous round of comments.  Overall, the
+submitter should get a clear sense of progress; the number of items open
+for discussion should clearly decrease over time.
+
+@item
+@emph{Review is a discussion.}  The submitter's and reviewer's views on
+how to achieve a particular change may not always be aligned.  As a
+reviewer, try hard to explain the rationale for suggestions you make,
+and to understand and take into account the submitter's motivation for
+doing things in a certain way.
+
+@item
+@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
+goal as a reviewer is to put the process on a clear path towards
+integration, possibly with agreed-upon changes, or rejection, with a
+clear and mutually-understood reasoning.  Avoid leaving the review
+process in a lingering state with no clear way out.
+@end enumerate
+
+@cindex LGTM, Looks Good To Me
+When you deem the proposed change adequate and ready for inclusion
+within Guix, the following well understood/codified @acronym{LGTM, Looks
+Good To Me} phrases should be used to sign off as a reviewer, meaning
+you have reviewed the change and that it looks good to you:
+
+@itemize
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with a @samp{This series LGTM!} to the cover page if it has
+one, or to the last patch of the series otherwise.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with @samp{LGTM!} to that commit
+message.
+@end itemize
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 

base-commit: 5a8f9d32f5196263bc50c2059bac4c4226784a59
-- 
2.41.0

[bug#66436] [PATCH 0/2] Add support for Git Large File Storage (LFS).

[Maxim Cournoyer]

While updating orcus to its latest version (0.19.0), I stumbled on a test a
failure, which ended up being caused by the lack of test files in the
repository checkout.  These files are stored using the LFS extension in the
repo; when not using LFS, empty text stubs with some metadata are left in
their place.

I've opted to keep the dependency on git-lfs optional for now for the daemon.
The git in-band downloader will only add the git-lfs dependency when the
git-reference object has its lfs? field set to true.

Maxim Cournoyer (2):
  gnu: git-lfs: Patch /bin/sh references.
  git-download: Add support for Git Large File Storage (LFS).

 configure.ac                      | 10 ++++++
 doc/guix.texi                     |  9 +++++
 gnu/packages/version-control.scm  |  5 +++
 guix/build/git.scm                | 18 +++++++---
 guix/config.scm.in                |  4 +++
 guix/git-download.scm             | 58 ++++++++++++++++++++-----------
 guix/scripts/perform-download.scm |  3 ++
 guix/self.scm                     | 10 +++++-
 8 files changed, 92 insertions(+), 25 deletions(-)


base-commit: d6d706a58b8159748d3a46fa97cae18850487c8a
--
2.41.0

[bug#66436] [PATCH 0/2] Add support for Git Large File Storage (LFS).

[Maxim Cournoyer]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> While updating orcus to its latest version (0.19.0), I stumbled on a test a
> failure, which ended up being caused by the lack of test files in the
> repository checkout.  These files are stored using the LFS extension in the
> repo; when not using LFS, empty text stubs with some metadata are left in
> their place.
>
> I've opted to keep the dependency on git-lfs optional for now for the daemon.
> The git in-band downloader will only add the git-lfs dependency when the
> git-reference object has its lfs? field set to true.
>
> Maxim Cournoyer (2):
>   gnu: git-lfs: Patch /bin/sh references.
>   git-download: Add support for Git Large File Storage (LFS).

Apologies for sending this series twice; the first time I sent it
erroneously to 66436 after forgetting to run 'mumi new'.

-- 
Thanks,
Maxim

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Josselin Poiret via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> +@cindex LGTM, Looks Good To Me
> +When you deem the proposed change adequate and ready for inclusion
> +within Guix, the following well understood/codified @acronym{LGTM, Looks
> +Good To Me} phrases should be used to sign off as a reviewer, meaning
> +you have reviewed the change and that it looks good to you:
> +
> +@itemize
> +@item
> +If the @emph{whole} series (containing multiple commits) looks good to
> +you, reply with a @samp{This series LGTM!} to the cover page if it has
> +one, or to the last patch of the series otherwise.
> +
> +@item
> +If you instead want to mark a @emph{single commit} as reviewed (but not
> +the whole series), simply reply with @samp{LGTM!} to that commit
> +message.
> +@end itemize

Some mbox -> patches tools (like b4) are able to automatically apply
trailers like "Reviewed-by: some ident <...>" found as replies to patch
series [1].  I also like the very clear meaning of it: there's no way
you would type this without meaning "Yes, I officially vouch that this
patch series has been reviewed by me".  Maybe this could be codified
here?

Otherwise, seem good to me.

[1] `b4 am`'s -t option, see
https://b4.docs.kernel.org/en/latest/maintainer/am-shazam.html

Best,
-- 
Josselin Poiret

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 682 bytes --]

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

Hello,

Josselin Poiret <dev@jpoiret.xyz> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> +@cindex LGTM, Looks Good To Me
>> +When you deem the proposed change adequate and ready for inclusion
>> +within Guix, the following well understood/codified @acronym{LGTM, Looks
>> +Good To Me} phrases should be used to sign off as a reviewer, meaning
>> +you have reviewed the change and that it looks good to you:
>> +
>> +@itemize
>> +@item
>> +If the @emph{whole} series (containing multiple commits) looks good to
>> +you, reply with a @samp{This series LGTM!} to the cover page if it has
>> +one, or to the last patch of the series otherwise.
>> +
>> +@item
>> +If you instead want to mark a @emph{single commit} as reviewed (but not
>> +the whole series), simply reply with @samp{LGTM!} to that commit
>> +message.
>> +@end itemize
>
> Some mbox -> patches tools (like b4) are able to automatically apply
> trailers like "Reviewed-by: some ident <...>" found as replies to patch
> series [1].  I also like the very clear meaning of it: there's no way
> you would type this without meaning "Yes, I officially vouch that this
> patch series has been reviewed by me".  Maybe this could be codified
> here?

Reading about it at
<https://b4.docs.kernel.org/en/latest/maintainer/am-shazam.html>, it
seems 'b4 -t' is about adding the 'Review-by' git trailers found in
replies to *cover letters*, and inserting them on each commit of the
series when applied.

Do you use such a tool when applying patches working with Guix review?
I'm not too fond of adding tool-specific bits to the otherwise
tool-agnostic section above, but perhaps we could add a note or
something.

-- 
Thanks,
Maxim

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Clément Lassieur]

Hi,

These are a few questions regarding reviewing.

1. What should the reviewer do with old-style patches, like the ones
   that don't use G-Expressions?  Should we tell the submitter to use
   them when possible or is it only a matter of style that is up to the
   submitter?  Obviously they are hard to grasp for newcomers.

   It's probably good for newcomers if we teach them how to use
   G-Expressions but we don't really have time to do so, given the
   number of patches waiting to be reviewed.

   This question could be extended to style issues.  Like using %var
   versus var.

2. What should the reviewer do when only small changes are required?
   The reviewer could do these changes in seconds whereas asking for a
   new revision could take days.  These changes could be indentation
   fixes, removing of unused code, but they could also be more
   substantial, like adding a missing `file-name` field.  Or changing
   old-style to G-Expressions?

   If the reviewer makes such changes and pushes them right away, I
   imagine they should be documented and explained.

3. Should the reviewer run the program being packaged?  The above
   guidelines speak about applying, reading, building and linting but
   not running.  (Making sure it works as expected.)

Thanks,
Clément

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

Hi,

Clément Lassieur <clement@lassieur.org> writes:

> Hi,
>
> These are a few questions regarding reviewing.
>
> 1. What should the reviewer do with old-style patches, like the ones
>    that don't use G-Expressions?  Should we tell the submitter to use
>    them when possible or is it only a matter of style that is up to the
>    submitter?  Obviously they are hard to grasp for newcomers.
>
>    It's probably good for newcomers if we teach them how to use
>    G-Expressions but we don't really have time to do so, given the
>    number of patches waiting to be reviewed.
>
>    This question could be extended to style issues.  Like using %var
>    versus var.

I think we should now make sure all new submissions use the current
style; if they aren't we can demand of the contributors to adjust it.
There is a blog post and enough examples in the code base already that
should make this not too difficult.

> 2. What should the reviewer do when only small changes are required?
>    The reviewer could do these changes in seconds whereas asking for a
>    new revision could take days.  These changes could be indentation
>    fixes, removing of unused code, but they could also be more
>    substantial, like adding a missing `file-name` field.  Or changing
>    old-style to G-Expressions?
>
>    If the reviewer makes such changes and pushes them right away, I
>    imagine they should be documented and explained.

In general, I think it's best to let the contributor know about the
small problems so they can submit a v2, as they'll learn to pay
attention to these.  For first submissions, we can make the experience
easier by adjusting locally and pushing directly for small things.  This
also holds for very old contributions where the original author is no
longer around.

I think these two points could be expound as new subsections of the
'Coding Style' section; the current scope is about codifying the human
interactions more than the technical details.

> 3. Should the reviewer run the program being packaged?  The above
>    guidelines speak about applying, reading, building and linting but
>    not running.  (Making sure it works as expected.)

From the diff:

--8<---------------cut here---------------start------------->8---
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers, and should result in
+the proposed change being merged faster.
--8<---------------cut here---------------end--------------->8---

So it does mention trying out the software ("running").

-- 
Thanks,
Maxim

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Clément Lassieur]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

>> 1. What should the reviewer do with old-style patches, like the ones
>>    that don't use G-Expressions?  Should we tell the submitter to use
>>    them when possible or is it only a matter of style that is up to the
>>    submitter?  Obviously they are hard to grasp for newcomers.
>>
>>    It's probably good for newcomers if we teach them how to use
>>    G-Expressions but we don't really have time to do so, given the
>>    number of patches waiting to be reviewed.
>>
>>    This question could be extended to style issues.  Like using %var
>>    versus var.
>
> I think we should now make sure all new submissions use the current
> style; if they aren't we can demand of the contributors to adjust it.
> There is a blog post and enough examples in the code base already that
> should make this not too difficult.

Are you referring to this one?
https://guix.gnu.org/en/blog/2023/dissecting-guix-part-3-g-expressions/

>> 2. What should the reviewer do when only small changes are required?
>>    The reviewer could do these changes in seconds whereas asking for a
>>    new revision could take days.  These changes could be indentation
>>    fixes, removing of unused code, but they could also be more
>>    substantial, like adding a missing `file-name` field.  Or changing
>>    old-style to G-Expressions?
>>
>>    If the reviewer makes such changes and pushes them right away, I
>>    imagine they should be documented and explained.
>
> +Perhaps the biggest action you can do to help GNU Guix grow as a project
> +is to review the work contributed by others.  You do not need to be a
> +committer to do so; applying, reading the source, building, linting and
> +running other people's series and sharing your comments about your
> +experience will give some confidence to committers, and should result in
> +the proposed change being merged faster.
>
> So it does mention trying out the software ("running").

Yes indeed!  I didn't see it.

Thanks for your reply!
Clément

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

Hi,

Clément Lassieur <clement@lassieur.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>>> 1. What should the reviewer do with old-style patches, like the ones
>>>    that don't use G-Expressions?  Should we tell the submitter to use
>>>    them when possible or is it only a matter of style that is up to the
>>>    submitter?  Obviously they are hard to grasp for newcomers.
>>>
>>>    It's probably good for newcomers if we teach them how to use
>>>    G-Expressions but we don't really have time to do so, given the
>>>    number of patches waiting to be reviewed.
>>>
>>>    This question could be extended to style issues.  Like using %var
>>>    versus var.
>>
>> I think we should now make sure all new submissions use the current
>> style; if they aren't we can demand of the contributors to adjust it.
>> There is a blog post and enough examples in the code base already that
>> should make this not too difficult.
>
> Are you referring to this one?
> https://guix.gnu.org/en/blog/2023/dissecting-guix-part-3-g-expressions/

Rather to the one corresponding to the 1.4.0 release, which introduced
these new changes: <https://guix.gnu.org/en/blog/2022/gnu-guix-1.4.0-released/>.

-- 
Thanks,
Maxim

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Simon Tournier]

Hi,

On Fri, 20 Oct 2023 at 19:01, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

>> 3. Should the reviewer run the program being packaged?  The above
>>    guidelines speak about applying, reading, building and linting but
>>    not running.  (Making sure it works as expected.)

> --8<---------------cut here---------------start------------->8---
> +Perhaps the biggest action you can do to help GNU Guix grow as a project
> +is to review the work contributed by others.  You do not need to be a
> +committer to do so; applying, reading the source, building, linting and
> +running other people's series and sharing your comments about your
> +experience will give some confidence to committers, and should result in
> +the proposed change being merged faster.
> --8<---------------cut here---------------end--------------->8---
>
> So it does mention trying out the software ("running").

If LGTM also implies “I run it and it is OK”, then submitter should
provide how to run it.

Otherwise, for what it is worth, I will stop to review stuff that I do
not use myself because reviewing is asking me too much: read some doc
about how to run something that I do not care.

    ( Similarly, if LGTM also implies “I have read the source code and
it is OK”, it appears to me too much. )

Well, “running” and “trying out the software” seems ambiguous.  What
does it mean “run IceCat” or “run Gmsh” or else?

What I like with the proposal is that it makes better defined what are
the expectations behind LGTM.  But what means “running” is not clear for
me.

IMHO, it is worth to clearly state:

 1. what helps the review process:

    « applying, reading the source, building, linting and running other
    people's series and sharing your comments about your experience will
    give some confidence to committers, and should result in the
    proposed change being merged faster. »

 2. what means LGTM, from my understanding: applying, building, linting,
    carefully checking the code that is merged to Guix.

Cheers,
simon

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Simon Tournier]

Re,

On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune@gmail.com> wrote:

>> --8<---------------cut here---------------start------------->8---
>> +Perhaps the biggest action you can do to help GNU Guix grow as a project
>> +is to review the work contributed by others.  You do not need to be a
>> +committer to do so; applying, reading the source, building, linting and
>> +running other people's series and sharing your comments about your
>> +experience will give some confidence to committers, and should result in
>> +the proposed change being merged faster.
>> --8<---------------cut here---------------end--------------->8---

[...]

> IMHO, it is worth to clearly state:
>
>  1. what helps the review process:
>
>     « applying, reading the source, building, linting and running other
>     people's series and sharing your comments about your experience will
>     give some confidence to committers, and should result in the
>     proposed change being merged faster. »

Although I do not know if it is the right place, I would mention that
“building” also means that the dependants still build or clearly
indicate if something breaks.


Cheers,
simon

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Ludovic Courtès]

Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
> section.
> * doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
    ^
Nitpick: no need to repeat the file name.

> +@node Reviewing the Work of Others
> +@section Reviewing the Work of Others

[...]

> +@cindex reviewing, guidelines
> +Review comments should be unambiguous; be as clear and explicit as you
> +can about what you think should be changed, ensuring the author can take
> +action on it.  Please try to keep the following guidelines in mind
> +during review:

[...]

> +@item
> +@emph{Remain focused: do not change the scope of the work being
> +reviewed.}  For example, if the contribution touches code that follows a
> +pattern deemed unwieldy, it would be unfair to ask the submitter to fix
> +all occurrences of that pattern in the code; to put it simply, if a
> +problem unrelated to the patch at hand was already there, do not ask the
> +submitter to fix it.

Another item came to mind, that could be phrased like this:

  Spend time proportional to the stakes.  Ensure the discussion focuses
  on important aspects of the changes; do not let it be derailed by
  objectively unimportant issues@footnote{This situation is often
  referred to as ``bikeshedding'', where much time is spent discussing
  each one's preference for the color of the shed at the expense
  progress made on the project to keep bikes dry.}.  In particular,
  issues such as code formatting and other conventions can be dealt with
  through automation---e.g., @command{guix lint} and @command{guix
  style}---or through documentation (@pxref{Packaging Guidelines}, as an
  example).

Maybe that makes the guidelines too long though; your call.

Otherwise LGTM!

Ludo’.

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Simon Tournier]

Hi

On Tue, 24 Oct 2023 at 17:54, Ludovic Courtès <ludo@gnu.org> wrote:

>> +@item
>> +@emph{Remain focused: do not change the scope of the work being
>> +reviewed.}  For example, if the contribution touches code that follows a
>> +pattern deemed unwieldy, it would be unfair to ask the submitter to fix
>> +all occurrences of that pattern in the code; to put it simply, if a
>> +problem unrelated to the patch at hand was already there, do not ask the
>> +submitter to fix it.

For me this item is clear…

> Another item came to mind, that could be phrased like this:

…while this new is unclear…

>   Spend time proportional to the stakes.  Ensure the discussion focuses
>   on important aspects of the changes; do not let it be derailed by
>   objectively unimportant issues@footnote{This situation is often
>   referred to as ``bikeshedding'', where much time is spent discussing
>   each one's preference for the color of the shed at the expense
>   progress made on the project to keep bikes dry.}.  In particular,
>   issues such as code formatting and other conventions can be dealt with
>   through automation---e.g., @command{guix lint} and @command{guix
>   style}---or through documentation (@pxref{Packaging Guidelines}, as an
>   example).

…especially in the light of these other items:

        +@item
        +@emph{Review is a discussion.}  The submitter's and reviewer's views on
        +how to achieve a particular change may not always be aligned.  As a
        +reviewer, try hard to explain the rationale for suggestions you make,
        +and to understand and take into account the submitter's motivation for
        +doing things in a certain way.

        +@item
        +@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
        +goal as a reviewer is to put the process on a clear path towards
        +integration, possibly with agreed-upon changes, or rejection, with a
        +clear and mutually-understood reasoning.  Avoid leaving the review
        +process in a lingering state with no clear way out.


Well, I do not like: « discussion focuses on important aspects of the
changes; do not let it be derailed by objectively unimportant issues »
because it is not clear for me what means “important aspects” or
“objectively unimportant issues”.  How do I gauge?

Sometimes, what does not appear to me “important” at first has then, at
the end, an impact that cannot be neglected.  This new item appears to
me as: it is not a open discussion and you should refrain from
commenting if you are not sure your point is *absolutely* important.

Instead of this new item – where my understanding is: avoid bikeshed :-)
and I agree – I propose to amend the item @emph{Review is a discussion.}
by explicitly refer to the 3 other items; which are, IMHO, the guards
against bikeshedding.  Something along:

--8<---------------cut here---------------start------------->8---
@item
@emph{Review is a discussion.}  The submitter's and reviewer's views on
how to achieve a particular change may not always be aligned.  The
discussion is lead by remain focused, ensure progress and aim for
finalization; spend time proportional to the stakes@footnote{This
situation is often referred to as ``bikeshedding'', where much time is
spent discussing each one's preference for the color of the shed at the
expense progress made on the project to keep bikes dry.}.  As a
reviewer, try hard to explain the rationale for suggestions you make,
and to understand and take into account the submitter's motivation for
doing things in a certain way.
--8<---------------cut here---------------end--------------->8---

WDYT?  Does it capture the idea?

If yes, I would pick this order for the enumeration:

 1. Be clear and explicit about changes you are suggesting 
 2. Remain focused
 3. Ensure progress
 4. Aim for finalization
 5. Review is a discussion


Cheers,
simon

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Josselin Poiret via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Do you use such a tool when applying patches working with Guix review?

Yes, it's very useful.

> I'm not too fond of adding tool-specific bits to the otherwise
> tool-agnostic section above, but perhaps we could add a note or
> something.

You can view this as a tool-agnostic but clear way way of formally
saying "I've reviewed this".  Otherwise, it's not entirely clear when
someone has reviewed a patch series IMO.

Best,
-- 
Josselin Poiret

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 682 bytes --]

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

Hi Simon,

Simon Tournier <zimon.toutoune@gmail.com> writes:

> Re,
>
> On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune@gmail.com> wrote:
>
>>> --8<---------------cut here---------------start------------->8---
>>> +Perhaps the biggest action you can do to help GNU Guix grow as a project
>>> +is to review the work contributed by others.  You do not need to be a
>>> +committer to do so; applying, reading the source, building, linting and
>>> +running other people's series and sharing your comments about your
>>> +experience will give some confidence to committers, and should result in
>>> +the proposed change being merged faster.
>>> --8<---------------cut here---------------end--------------->8---
>
> [...]
>
>> IMHO, it is worth to clearly state:
>>
>>  1. what helps the review process:
>>
>>     « applying, reading the source, building, linting and running other
>>     people's series and sharing your comments about your experience will
>>     give some confidence to committers, and should result in the
>>     proposed change being merged faster. »
>
> Although I do not know if it is the right place, I would mention that
> “building” also means that the dependants still build or clearly
> indicate if something breaks.

That's already mentioned in 'Submitting Patches':

  9. Check that dependent packages (if applicable) are not affected by
     the change; ‘guix refresh --list-dependent PACKAGE’ will help you
     do that (*note Invoking guix refresh::).

I don't think we should repeat it here :-) (also, we now have CI, which
should be more apt at catching breakage here).

--
Thanks,
Maxim

[bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.

[Simon Tournier]

Hi Maxim,

On Tue, 31 Oct 2023 at 19:53, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

> >> IMHO, it is worth to clearly state:

[...]

> That's already mentioned in 'Submitting Patches':

[...]

> I don't think we should repeat it here :-) (also, we now have CI, which
> should be more apt at catching breakage here).

We are listing the expectations for the Review process, therefore
repeat that the dependencies need to be checked at Review time makes
sense to me.  It can be a sentence as: "make sure CI does not report
new error" or something like that.

Cheers,
simon

[bug#66475] [PATCH v2 1/4] git-download: Add support for Git Large File Storage (LFS).

[Maxim Cournoyer]

* guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
(git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
* guix/git-download.scm (git-lfs-package): New procedure.
(git-fetch/in-band*): New procedure, made of the logic of git-fetch/in-band,
with new git-lfs specifics, with the following changes:
New #:git-lfs argument.
<inputs>: Remove labels.  Conditionally add git-lfs.
<build>: Read "git lfs?" environment
variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
Use INPUTS directly; update comment.
<gexp->derivation>: Add "git lfs?" to #:env-vars.
(git-fetch/in-band): Express in terms of git-fetch/in-band*.
(git-fetch/lfs): New procedure.
* doc/guix.texi (origin Reference): Document it.

Change-Id: I5b233b8642a7bdb8737b9d9b740e7254a89ccb25
---

Changes in v2:
 - Do not add lfs? to <git-reference>; instead add a git-fetch/lfs procedure.

 doc/guix.texi         |  7 ++++
 guix/build/git.scm    | 19 +++++++--
 guix/git-download.scm | 97 ++++++++++++++++++++++++++++++-------------
 3 files changed, 91 insertions(+), 32 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index b90078be06..0076e27939 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -8375,6 +8375,13 @@ origin Reference
 the file name, or a generic name if @code{#f}.
 @end deffn
 
+@deffn {Procedure} git-fetch/lfs ref hash-algo hash
+This is a variant of the @code{git-fetch} procedure that supports the
+Git @acronym{LFS, Large File Storage} extension.  This may be useful to
+pull some binary test data to run the test suite of a package, for
+example.
+@end deffn
+
 @deftp {Data Type} git-reference
 This data type represents a Git reference for @code{git-fetch} to
 retrieve.
diff --git a/guix/build/git.scm b/guix/build/git.scm
index 0ff263c81b..867cade2c4 100644
--- a/guix/build/git.scm
+++ b/guix/build/git.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2014, 2016, 2019, 2023 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -33,10 +34,13 @@ (define-module (guix build git)
 ;;; Code:
 
 (define* (git-fetch url commit directory
-                    #:key (git-command "git") recursive?)
+                    #:key (git-command "git")
+                    lfs? recursive?)
   "Fetch COMMIT from URL into DIRECTORY.  COMMIT must be a valid Git commit
-identifier.  When RECURSIVE? is true, all the sub-modules of URL are fetched,
-recursively.  Return #t on success, #f otherwise."
+identifier.  When LFS? is true, configure Git to also fetch Large File
+Storage (LFS) files; it assumes that the @code{git-lfs} extension is available
+in the environment.  When RECURSIVE? is true, all the sub-modules of URL are
+fetched, recursively.  Return #t on success, #f otherwise."
 
   ;; Disable TLS certificate verification.  The hash of the checkout is known
   ;; in advance anyway.
@@ -57,6 +61,11 @@ (define* (git-fetch url commit directory
     (with-directory-excursion directory
       (invoke git-command "init" "--initial-branch=main")
       (invoke git-command "remote" "add" "origin" url)
+
+      (when lfs?
+        (setenv "HOME" "/tmp")
+        (invoke git-command "lfs" "install"))
+
       (if (zero? (system* git-command "fetch" "--depth" "1" "origin" commit))
           (invoke git-command "checkout" "FETCH_HEAD")
           (begin
@@ -81,11 +90,13 @@ (define* (git-fetch url commit directory
 
 
 (define* (git-fetch-with-fallback url commit directory
-                                  #:key (git-command "git") recursive?)
+                                  #:key (git-command "git")
+                                  lfs? recursive?)
   "Like 'git-fetch', fetch COMMIT from URL into DIRECTORY, but fall back to
 alternative methods when fetching from URL fails: attempt to download a nar,
 and if that also fails, download from the Software Heritage archive."
   (or (git-fetch url commit directory
+                 #:lfs? lfs?
                  #:recursive? recursive?
                  #:git-command git-command)
       (download-nar directory)
diff --git a/guix/git-download.scm b/guix/git-download.scm
index 5d5d73dc6b..3de6ae970d 100644
--- a/guix/git-download.scm
+++ b/guix/git-download.scm
@@ -4,6 +4,7 @@
 ;;; Copyright © 2017 Christopher Baines <mail@cbaines.net>
 ;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
 ;;; Copyright © 2023 Simon Tournier <zimon.toutoune@gmail.com>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -54,6 +55,7 @@ (define-module (guix git-download)
             git-reference-recursive?
 
             git-fetch
+            git-fetch/lfs
             git-version
             git-file-name
             git-predicate))
@@ -79,30 +81,36 @@ (define (git-package)
   (let ((distro (resolve-interface '(gnu packages version-control))))
     (module-ref distro 'git-minimal)))
 
-(define* (git-fetch/in-band ref hash-algo hash
-                            #:optional name
-                            #:key (system (%current-system))
-                            (guile (default-guile))
-                            (git (git-package)))
-  "Return a fixed-output derivation that performs a Git checkout of REF, using
-GIT and GUILE (thus, said derivation depends on GIT and GUILE).
+(define (git-lfs-package)
+  "Return the default 'git-lfs' package."
+  (let ((distro (resolve-interface '(gnu packages version-control))))
+    (module-ref distro 'git-lfs)))
 
-This method is deprecated in favor of the \"builtin:git-download\" builder.
-It will be removed when versions of guix-daemon implementing
-\"builtin:git-download\" will be sufficiently widespread."
+(define* (git-fetch/in-band* ref hash-algo hash
+                             #:optional name
+                             #:key (system (%current-system))
+                             (guile (default-guile))
+                             (git (git-package))
+                             git-lfs)
+  "Shared implementation code for git-fetch/in-band & friends.  Refer to their
+respective documentation."
   (define inputs
-    `(("git" ,(or git (git-package)))
-
-      ;; When doing 'git clone --recursive', we need sed, grep, etc. to be
-      ;; available so that 'git submodule' works.
+    `(,(or git (git-package))
+      ,@(if git-lfs
+            (list git-lfs)
+            '())
       ,@(if (git-reference-recursive? ref)
-            (standard-packages)
+            ;; TODO: remove (standard-packages) after
+            ;; 48e528a26f9c019eeaccf5e3de3126aa02c98d3b is merged into master;
+            ;; currently when doing 'git clone --recursive', we need sed, grep,
+            ;; etc. to be available so that 'git submodule' works.
+            (map second (standard-packages))
 
             ;; The 'swh-download' procedure requires tar and gzip.
-            `(("gzip" ,(module-ref (resolve-interface '(gnu packages compression))
-                                   'gzip))
-              ("tar" ,(module-ref (resolve-interface '(gnu packages base))
-                                  'tar))))))
+            (list (module-ref (resolve-interface '(gnu packages compression))
+                              'gzip)
+                  (module-ref (resolve-interface '(gnu packages base))
+                              'tar)))))
 
   (define guile-json
     (module-ref (resolve-interface '(gnu packages guile)) 'guile-json-4))
@@ -126,7 +134,7 @@ (define* (git-fetch/in-band ref hash-algo hash
 
   (define build
     (with-imported-modules modules
-      (with-extensions (list guile-json gnutls    ;for (guix swh)
+      (with-extensions (list guile-json gnutls ;for (guix swh)
                              guile-lzlib)
         #~(begin
             (use-modules (guix build git)
@@ -134,6 +142,9 @@ (define* (git-fetch/in-band ref hash-algo hash
                           #:select (set-path-environment-variable))
                          (ice-9 match))
 
+            (define lfs?
+              (call-with-input-string (getenv "git lfs?") read))
+
             (define recursive?
               (call-with-input-string (getenv "git recursive?") read))
 
@@ -144,18 +155,17 @@ (define* (git-fetch/in-band ref hash-algo hash
                     #+(file-append glibc-locales "/lib/locale"))
             (setlocale LC_ALL "en_US.utf8")
 
-            ;; The 'git submodule' commands expects Coreutils, sed,
-            ;; grep, etc. to be in $PATH.
-            (set-path-environment-variable "PATH" '("bin")
-                                           (match '#+inputs
-                                             (((names dirs outputs ...) ...)
-                                              dirs)))
+            ;; The 'git submodule' commands expects Coreutils, sed, grep,
+            ;; etc. to be in $PATH.  This also ensures that git extensions are
+            ;; found.
+            (set-path-environment-variable "PATH" '("bin") '#+inputs)
 
             (setvbuf (current-output-port) 'line)
             (setvbuf (current-error-port) 'line)
 
             (git-fetch-with-fallback (getenv "git url") (getenv "git commit")
                                      #$output
+                                     #:lfs? lfs?
                                      #:recursive? recursive?
                                      #:git-command "git")))))
 
@@ -175,18 +185,49 @@ (define* (git-fetch/in-band ref hash-algo hash
                                          (git-reference-url ref))))
                         ("git commit" . ,(git-reference-commit ref))
                         ("git recursive?" . ,(object->string
-                                              (git-reference-recursive? ref))))
+                                              (git-reference-recursive? ref)))
+                        ("git lfs?" . ,(if git-lfs "#t" "#f")))
                       #:leaked-env-vars '("http_proxy" "https_proxy"
                                           "LC_ALL" "LC_MESSAGES" "LANG"
                                           "COLUMNS")
 
                       #:system system
-                      #:local-build? #t           ;don't offload repo cloning
+                      #:local-build? #t ;don't offload repo cloning
                       #:hash-algo hash-algo
                       #:hash hash
                       #:recursive? #t
                       #:guile-for-build guile)))
 
+(define* (git-fetch/in-band ref hash-algo hash
+                             #:optional name
+                             #:key (system (%current-system))
+                             (guile (default-guile))
+                             (git (git-package)))
+  "Return a fixed-output derivation that performs a Git checkout of REF, using
+GIT and GUILE (thus, said derivation depends on GIT and GUILE).
+
+This method is deprecated in favor of the \"builtin:git-download\" builder.
+It will be removed when versions of guix-daemon implementing
+\"builtin:git-download\" will be sufficiently widespread."
+  (git-fetch/in-band* ref hash-algo hash name
+                      #:system system
+                      #:guile guile
+                      #:git git))
+
+(define* (git-fetch/lfs ref hash-algo hash
+                        #:optional name
+                        #:key (system (%current-system))
+                        (guile (default-guile))
+                        (git (git-package))
+                        (git-lfs (git-lfs-package)))
+  "Like git-fetch/in-band, but with support for the Git Large File
+Storage (LFS) extension."
+  (git-fetch/in-band* ref hash-algo hash name
+                      #:system system
+                      #:guile guile
+                      #:git git
+                      #:git-lfs git-lfs))
+
 (define* (git-fetch/built-in ref hash-algo hash
                              #:optional name
                              #:key (system (%current-system)))

base-commit: d96a9c7473a6d07747f59eeda7d4085173c25383
-- 
2.41.0

[bug#66475] [PATCH v2 2/4] gnu: mdds: Update to 2.1.1.

[Maxim Cournoyer]

* gnu/packages/boost.scm (mdds): Update to 2.1.1.
[source]: Fetch from git.
[native-inputs]: New field.

Change-Id: I4e71d5c360f4b7305cffd7008e2bbbfcaad2f897
---

(no changes since v1)

 gnu/packages/boost.scm | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/gnu/packages/boost.scm b/gnu/packages/boost.scm
index 98dccf7f16..71999709ed 100644
--- a/gnu/packages/boost.scm
+++ b/gnu/packages/boost.scm
@@ -8,7 +8,7 @@
 ;;; Copyright © 2018 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2018, 2019, 2021 Ricardo Wurmus <rekado@elephly.net>
-;;; Copyright © 2018, 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2018, 2020, 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2018, 2020 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2019 Mathieu Othacehe <m.othacehe@gmail.com>
 ;;; Copyright © 2019, 2020 Giacomo Leidi <goodoldpaul@autistici.org>
@@ -44,6 +44,7 @@ (define-module (gnu packages boost)
   #:use-module (guix build-system gnu)
   #:use-module (guix build-system trivial)
   #:use-module (gnu packages)
+  #:use-module (gnu packages autotools)
   #:use-module (gnu packages compression)
   #:use-module (gnu packages icu4c)
   #:use-module (gnu packages llvm)
@@ -445,15 +446,18 @@ (define-public boost-mpi
 (define-public mdds
   (package
     (name "mdds")
-    (version "2.0.3")
+    (version "2.1.1")
     (source (origin
-             (method url-fetch)
-             (uri (string-append "https://kohei.us/files/mdds/src/mdds-"
-                                 version ".tar.xz"))
-             (sha256
-              (base32
-               "1r68kxqppmhfg0dhz54d0hqzs5882cqrv1x6wpg7lak6gyyws0bc"))))
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://gitlab.com/mdds/mdds")
+                    (commit version)))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "0866020brc1kmiryh7dmhjamnywlsd56ks649hy87283k0p7d3bb"))))
     (build-system gnu-build-system)
+    (native-inputs (list autoconf automake))
     (propagated-inputs
       (list boost)) ; inclusion of header files
     (home-page "https://gitlab.com/mdds/mdds")
-- 
2.41.0

[bug#66475] [PATCH v2 3/4] gnu: ixion: Update to 0.19.0.

[Maxim Cournoyer]

* gnu/packages/libreoffice.scm (ixion): Update to 0.19.0.
[source]: Use git.
[native-inputs]: Add autoconf, automake and libtool.

Change-Id: I245457d7c99b6adfb895dc46276f8008ff13d0cd
---

(no changes since v1)

 gnu/packages/libreoffice.scm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/gnu/packages/libreoffice.scm b/gnu/packages/libreoffice.scm
index 71131ca1f3..e38095fb1e 100644
--- a/gnu/packages/libreoffice.scm
+++ b/gnu/packages/libreoffice.scm
@@ -14,6 +14,7 @@
 ;;; Copyright © 2019 Chris Marusich <cmmarusich@gmail.com>
 ;;; Copyright © 2020 Marcin Karpezo <sirmacik@wioo.waw.pl>
 ;;; Copyright © 2023 Nicolas Graves <ngraves@ngraves.fr>
+;;; Copyright © 2023 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -92,20 +93,19 @@ (define-module (gnu packages libreoffice)
 (define-public ixion
   (package
     (name "ixion")
-    (version "0.17.0")
-    (source
-     (origin
-       (method url-fetch)
-       (uri (string-append "http://kohei.us/files/ixion/src/libixion-"
-                           version ".tar.xz"))
-       (sha256
-        (base32
-         "07hhqkvns4da8xv990gr1smqz1zf40m531lg95nphfrz48wp3jak"))))
+    (version "0.19.0")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://gitlab.com/ixion/ixion")
+                    (commit version)))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "0nycbs3765wkaw9ff7aflm56ayxkn15dlfl5pbbb9b5i2rcv3dq6"))))
     (build-system gnu-build-system)
-    (native-inputs
-     (list pkg-config))
-    (inputs
-     (list mdds python spdlog))
+    (native-inputs (list autoconf automake libtool pkg-config))
+    (inputs (list mdds python spdlog))
     (home-page "https://gitlab.com/ixion/ixion")
     (synopsis "General purpose formula parser and interpreter")
     (description "Ixion is a library for calculating the results of formula
-- 
2.41.0

[bug#66475] [PATCH v2 4/4] gnu: orcus: Update to 0.19.0.

[Maxim Cournoyer]

* gnu/packages/libreoffice.scm (orcus): Update to 0.19.0.
[source]: Use git-fetch/lfs.
[arguments]: Use gexps.
[native-inputs]: Add autoconf, automake and libtool.

Change-Id: I76efbc57ca4acf8ffd5154a72e49b4aedd071a76
---

(no changes since v1)

 gnu/packages/libreoffice.scm | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/gnu/packages/libreoffice.scm b/gnu/packages/libreoffice.scm
index e38095fb1e..572077a0ee 100644
--- a/gnu/packages/libreoffice.scm
+++ b/gnu/packages/libreoffice.scm
@@ -117,22 +117,22 @@ (define-public ixion
 (define-public orcus
   (package
     (name "orcus")
-    (version "0.17.2")
-    (source
-     (origin
-       (method url-fetch)
-       (uri (string-append "http://kohei.us/files/orcus/src/lib"
-                           "orcus-" version ".tar.xz"))
-       (sha256
-        (base32
-         "1as04qb74jnlnwy4wh5jwaw2nnzgn2s3230ymvh3kx1w9r0rsl1h"))))
+    (version "0.19.0")
+    (source (origin
+              ;; The test suite requires data files store with Git Large
+              ;; File Storage.
+              (method git-fetch/lfs)
+              (uri (git-reference
+                    (url "https://gitlab.com/orcus/orcus")
+                    (commit version)))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "02prj6kgph56fkr89k8wlqarrmx65cq92863i4rrny5sqr8c2llr"))))
     (build-system gnu-build-system)
-    (arguments
-     `(#:configure-flags '("--disable-static")))
-    (native-inputs
-     (list pkg-config))
-    (inputs
-     (list ixion mdds python zlib))
+    (arguments (list #:configure-flags #~(list "--disable-static")))
+    (native-inputs (list autoconf automake libtool pkg-config))
+    (inputs (list ixion mdds python zlib))
     (home-page "https://gitlab.com/orcus/orcus")
     (synopsis "File import filter library for spreadsheet documents")
     (description "Orcus is a library that provides a collection of standalone
-- 
2.41.0

[bug#66436] [PATCH v3] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
(Debbugs Usertags): Expound with Emacs Debbugs information and document the
'reviewed-looks-good' usertag.

Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
---

Changes in v3:
 - Replace LGTM with Reviewed-by Git tag
 - Add b4 config
 - Link to the Submitting Patches section for check list
 - Fuse further suggestions by both Ludovic and Simon

 doc/contributing.texi | 111 ++++++++++++++++++++++++++++++++++++++++--
 etc/git/gitconfig     |   7 +++
 2 files changed, 114 insertions(+), 4 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 30876447d4..023478c98d 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -1981,7 +1982,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs@footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1994,9 +2000,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2014,6 +2020,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2283,6 +2292,100 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers.  Basically, you gmust
+ensure the check list found in the @ref{Submitting Patches} section has
+been correctly followed.  A reviewed patch series should give the best
+chances for the proposed change to be merged faster, so if a change you
+would like to see merged hasn't yet been reviewed, this is the most
+appropriate thing to do!
+
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  Please try to keep the following guidelines in mind
+during review:
+
+@enumerate
+@item
+@emph{Be clear and explicit about changes you are suggesting}, ensuring
+the author can take action on it.  In particular, it is a good idea to
+explicitly ask for new revisions when you want it.
+
+@item
+@emph{Remain focused: do not change the scope of the work being
+reviewed.}  For example, if the contribution touches code that follows a
+pattern deemed unwieldy, it would be unfair to ask the submitter to fix
+all occurrences of that pattern in the code; to put it simply, if a
+problem unrelated to the patch at hand was already there, do not ask the
+submitter to fix it.
+
+@item
+@emph{Ensure progress.}  As they respond to review, submitters may
+submit new revisions of their changes; avoid requesting changes that you
+did not request in the previous round of comments.  Overall, the
+submitter should get a clear sense of progress; the number of items open
+for discussion should clearly decrease over time.
+
+@item
+@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
+goal as a reviewer is to put the process on a clear path towards
+integration, possibly with agreed-upon changes, or rejection, with a
+clear and mutually-understood reasoning.  Avoid leaving the review
+process in a lingering state with no clear way out.
+
+@item
+@emph{Review is a discussion.}  The submitter's and reviewer's views on
+how to achieve a particular change may not always be aligned.  To lead
+the discussion, remain focused, ensure progress and aim for
+finalization, spending time proportional to the stakes@footnote{The
+tendency to discuss minute details at length is often referred to as
+``bikeshedding'', where much time is spent discussing each one's
+preference for the color of the shed at the expense of progress made on
+the project to keep bikes dry.}.  As a reviewer, try hard to explain the
+rationale for suggestions you make, and to understand and take into
+account the submitter's motivation for doing things in a certain way.
+@end enumerate
+
+@cindex LGTM, Looks Good To Me
+@cindex review tags
+@cindex Reviewed-by, git trailer
+When you deem the proposed change adequate and ready for inclusion
+within Guix, the following well understood/codified
+@samp{Reviewed-by:@tie{}Your@tie{}Name<your-email@@example.com>}
+@footnote{The @samp{Reviewed-by} Git trailer is used by other projects
+such as Linux, and is understood by third-party tools such as the
+@samp{b4 am} sub-command, which is able to retrieve the complete
+submission email thread from a public-inbox instance and add the Git
+trailers found in replies to the commit patches.} line should be used to
+sign off as a reviewer, meaning you have reviewed the change and that it
+looks good to you:
+
+@itemize
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with @samp{Reviewed-by:@tie{}Your@tie{}Name<your-email@@example.com>}
+to the cover page if it has one, or to the last patch of the series
+otherwise, adding another @samp{(for the whole series)} comment on the
+line below to explicit this fact.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with
+@samp{Reviewed-by:@tie{}Your@tie{}Name<your-email@@example.com>} to that
+commit message.
+@end itemize
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 
diff --git a/etc/git/gitconfig b/etc/git/gitconfig
index 907ad01804..654a630b18 100644
--- a/etc/git/gitconfig
+++ b/etc/git/gitconfig
@@ -16,3 +16,10 @@
         to = guix-patches@gnu.org
         headerCmd = etc/teams.scm cc-members-header-cmd
         thread = no
+
+[b4]
+        attestation-check-dkim = off
+        attestation-policy = off
+        linkmask = https://yhetil.org/guix/%s
+        linktrailermask = https://yhetil.org/guix/%s
+        midmask = https://yhetil.org/guix/%s

base-commit: a96f1c1bc0fa186414359890025e8acacbb1de02
-- 
2.41.0

[bug#66475] [PATCH v2 1/4] git-download: Add support for Git Large File Storage (LFS).

[Ludovic Courtès]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> * guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
> (git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
> * guix/git-download.scm (git-lfs-package): New procedure.
> (git-fetch/in-band*): New procedure, made of the logic of git-fetch/in-band,
> with new git-lfs specifics, with the following changes:
> New #:git-lfs argument.
> <inputs>: Remove labels.  Conditionally add git-lfs.
> <build>: Read "git lfs?" environment
> variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
> Use INPUTS directly; update comment.
> <gexp->derivation>: Add "git lfs?" to #:env-vars.
> (git-fetch/in-band): Express in terms of git-fetch/in-band*.
> (git-fetch/lfs): New procedure.
> * doc/guix.texi (origin Reference): Document it.
>
> Change-Id: I5b233b8642a7bdb8737b9d9b740e7254a89ccb25
> ---
>
> Changes in v2:
>  - Do not add lfs? to <git-reference>; instead add a git-fetch/lfs procedure.

I haven’t tested it but it looks great to me, thank you!

Ludo’.

[bug#66436] [PATCH v3] doc: Add some guidelines for reviewing.

[Ludovic Courtès]

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
> section.
> (Debbugs Usertags): Expound with Emacs Debbugs information and document the
> 'reviewed-looks-good' usertag.
>
> Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
> ---
>
> Changes in v3:
>  - Replace LGTM with Reviewed-by Git tag
>  - Add b4 config
>  - Link to the Submitting Patches section for check list
>  - Fuse further suggestions by both Ludovic and Simon

Could you mention the b4 change in the commit log?  Otherwise LGTM!

> +[b4]
> +        attestation-check-dkim = off
> +        attestation-policy = off
> +        linkmask = https://yhetil.org/guix/%s
> +        linktrailermask = https://yhetil.org/guix/%s
> +        midmask = https://yhetil.org/guix/%s

Really cool to have the b4 workflow documented and a default config in
place!

Ludo’.

bug#66436: [PATCH v3] doc: Add some guidelines for reviewing.

[Maxim Cournoyer]

Hello,

Ludovic Courtès <ludo@gnu.org> writes:

> Hello,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> * doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
>> section.
>> (Debbugs Usertags): Expound with Emacs Debbugs information and document the
>> 'reviewed-looks-good' usertag.
>>
>> Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
>> ---
>>
>> Changes in v3:
>>  - Replace LGTM with Reviewed-by Git tag
>>  - Add b4 config
>>  - Link to the Submitting Patches section for check list
>>  - Fuse further suggestions by both Ludovic and Simon
>
> Could you mention the b4 change in the commit log?  Otherwise LGTM!

Done, thanks for the review.

>> +[b4]
>> +        attestation-check-dkim = off
>> +        attestation-policy = off
>> +        linkmask = https://yhetil.org/guix/%s
>> +        linktrailermask = https://yhetil.org/guix/%s
>> +        midmask = https://yhetil.org/guix/%s
>
> Really cool to have the b4 workflow documented and a default config in
> place!

Now we could document more extensively some successful workflows in the
Cookbook (Debbugs/Gnus-based, QA-based, b4-based, etc.) :-).

-- 
Thanks,
Maxim

bug#66475: [PATCH v2 1/4] git-download: Add support for Git Large File Storage (LFS).

[Maxim Cournoyer]

Hi,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> * guix/build/git.scm (git-fetch) [lfs?]: New argument, doc and setup code.
>> (git-fetch-with-fallback) [lfs?]: New argument.  Pass it to git-fetch.
>> * guix/git-download.scm (git-lfs-package): New procedure.
>> (git-fetch/in-band*): New procedure, made of the logic of git-fetch/in-band,
>> with new git-lfs specifics, with the following changes:
>> New #:git-lfs argument.
>> <inputs>: Remove labels.  Conditionally add git-lfs.
>> <build>: Read "git lfs?" environment
>> variable and pass its value to the #:lfs? argument of git-fetch-with-fallback.
>> Use INPUTS directly; update comment.
>> <gexp->derivation>: Add "git lfs?" to #:env-vars.
>> (git-fetch/in-band): Express in terms of git-fetch/in-band*.
>> (git-fetch/lfs): New procedure.
>> * doc/guix.texi (origin Reference): Document it.
>>
>> Change-Id: I5b233b8642a7bdb8737b9d9b740e7254a89ccb25
>> ---
>>
>> Changes in v2:
>>  - Do not add lfs? to <git-reference>; instead add a git-fetch/lfs procedure.
>
> I haven’t tested it but it looks great to me, thank you!

Thanks for taking the time to review it!  I'll push it shortly.

-- 
Thanks,
Maxim