Package: guix-patches;
Reported by: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Date: Tue, 10 Oct 2023 12:56:02 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66436 in the body.
You can then email your comments to 66436 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 10 Oct 2023 12:56:02 GMT) Full text and rfc822 format available.Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
:guix-patches <at> gnu.org
.
(Tue, 10 Oct 2023 12:56:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: guix-patches <at> gnu.org, maxim.cournoyer <at> gmail.com Subject: [PATCH] doc: Add some guidelines for reviewing. Date: Tue, 10 Oct 2023 08:54:27 -0400
* 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 <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 10 Oct 2023 13:31:02 GMT) Full text and rfc822 format available.Message #8 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org Subject: Re: [bug#66436] [PATCH] doc: Add some guidelines for reviewing. Date: Tue, 10 Oct 2023 15:29:37 +0200
Hi, This looks great to me! Maxim Cournoyer <maxim.cournoyer <at> 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’.
maxim.cournoyer <at> gmail.com, ludo <at> gnu.org, guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Wed, 11 Oct 2023 00:25:02 GMT) Full text and rfc822 format available.Message #11 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: 66436 <at> debbugs.gnu.org Cc: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Subject: [PATCH v2] doc: Add some guidelines for reviewing. Date: Tue, 10 Oct 2023 20:24:05 -0400
* 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 <at> gnu.org Series-to: 66436 <at> debbugs.gnu.org Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b Co-authored-by: Ludovic Courtès <ludo <at> 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 <at> 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
maxim.cournoyer <at> gmail.com, ludo <at> gnu.org, guix <at> cbaines.net, dev <at> jpoiret.xyz, othacehe <at> gnu.org, rekado <at> elephly.net, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Thu, 12 Oct 2023 02:50:01 GMT) Full text and rfc822 format available.Message #14 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: 66436 <at> debbugs.gnu.org Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Subject: [PATCH 0/2] Add support for Git Large File Storage (LFS). Date: Wed, 11 Oct 2023 22:48:40 -0400
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
maxim.cournoyer <at> gmail.com, ludo <at> gnu.org, guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Thu, 12 Oct 2023 02:50:02 GMT) Full text and rfc822 format available.Message #17 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: 66436 <at> debbugs.gnu.org Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Subject: [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references. Date: Wed, 11 Oct 2023 22:48:41 -0400
* gnu/packages/version-control.scm (git-lfs) [arguments]: Add patch-/bin/sh phase. Change-Id: I2d455e683e4f6e30cd32f5b1fdaccac71616826c --- gnu/packages/version-control.scm | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-control.scm index d9c53af71c..04b52f2a85 100644 --- a/gnu/packages/version-control.scm +++ b/gnu/packages/version-control.scm @@ -3175,6 +3175,11 @@ (define-public git-lfs #:install-source? #f #:phases #~(modify-phases %standard-phases + (add-after 'unpack 'patch-/bin/sh + (lambda* (#:key inputs #:allow-other-keys) + (substitute* "src/github.com/git-lfs/git-lfs/lfs/hook.go" + (("/bin/sh") + (search-input-file inputs "bin/sh"))))) (add-after 'unpack 'fix-embed-x-net (lambda _ (delete-file-recursively "src/golang.org/x/net/publicsuffix/data") -- 2.41.0
maxim.cournoyer <at> gmail.com, ludo <at> gnu.org, guix <at> cbaines.net, dev <at> jpoiret.xyz, othacehe <at> gnu.org, rekado <at> elephly.net, zimon.toutoune <at> gmail.com, me <at> tobias.gr, guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Thu, 12 Oct 2023 02:50:02 GMT) Full text and rfc822 format available.Message #20 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: 66436 <at> debbugs.gnu.org Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Subject: [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS). Date: Wed, 11 Oct 2023 22:48:42 -0400
* guix/git-download.scm (<git-reference>) [lfs?]: New field. (git-fetch/in-band): 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/built-in): Add "lfs?" to #:env-vars. * 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/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?' environment variable. * doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?' field. (Requirements): Mention the optional 'git-lfs' dependency. * configure.ac: Add a check for the 'git-lfs' command. * guix/config.scm.in (%git-lfs): New variable. * guix/self.scm (%packages): Add git-lfs. (compiled-guix): Add git-lfs to guix-config. (make-config.scm): New #:git-lfs argument. Change-Id: I5b233b8642a7bdb8737b9d9b740e7254a89ccb25 --- configure.ac | 10 ++++++ doc/guix.texi | 9 +++++ 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 +++++- 7 files changed, 87 insertions(+), 25 deletions(-) diff --git a/configure.ac b/configure.ac index d817f620cf..5342c0f80e 100644 --- a/configure.ac +++ b/configure.ac @@ -208,6 +208,16 @@ if test "x$GIT" = "x"; then fi AC_SUBST([GIT]) +dnl Git Large File Storage is an optional dependency for the +dnl "builtin:git-download" derivation builder. +AC_PATH_PROG([GIT_LFS], [git-lfs]) +if test "x$GIT_LFS" = "x"; then + AC_MSG_WARN([Git Large File Storage (git-lfs) is missing; + The builtin:git-download derivation builder of the Guix daemon will + not be able to use it.]) +fi +AC_SUBST([GIT_LFS]) + LIBGCRYPT_LIBDIR="no" LIBGCRYPT_PREFIX="no" diff --git a/doc/guix.texi b/doc/guix.texi index dc16ec1d15..89faaa7b90 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -1020,6 +1020,11 @@ Requirements The following dependencies are optional: @itemize +@item +The daemon will be able to fetch from Git repositories using the +@uref{https://git-lfs.com/, Git Large File Storage} extension when the +@command{git-lfs} command is available. + @item @c Note: We need at least 0.13.0 for #:nodelay. Support for build offloading (@pxref{Daemon Offload Setup}) and @@ -8499,6 +8504,10 @@ origin Reference @command{git describe} style identifier such as @code{v1.0.1-10-g58d7909c97}. +@item @code{lfs?} (default: @code{#f}) +This Boolean indicates whether to fetch Git large file storage (LFS) +files. + @item @code{recursive?} (default: @code{#f}) This Boolean indicates whether to recursively fetch Git sub-modules. @end table diff --git a/guix/build/git.scm b/guix/build/git.scm index 0ff263c81b..82c31fabf1 100644 --- a/guix/build/git.scm +++ b/guix/build/git.scm @@ -33,10 +33,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 +60,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 +89,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/config.scm.in b/guix/config.scm.in index 62e15dd713..4997a1740e 100644 --- a/guix/config.scm.in +++ b/guix/config.scm.in @@ -36,6 +36,7 @@ (define-module (guix config) %system %git + %git-lfs %gzip %bzip2 %xz)) @@ -113,6 +114,9 @@ (define %system (define %git "@GIT@") +(define %git-lfs + "@GIT_LFS@") + (define %gzip "@GZIP@") diff --git a/guix/git-download.scm b/guix/git-download.scm index 5d5d73dc6b..6feeea6428 100644 --- a/guix/git-download.scm +++ b/guix/git-download.scm @@ -51,6 +51,7 @@ (define-module (guix git-download) git-reference? git-reference-url git-reference-commit + git-reference-lfs? git-reference-recursive? git-fetch @@ -71,7 +72,9 @@ (define-record-type* <git-reference> git-reference? (url git-reference-url) (commit git-reference-commit) - (recursive? git-reference-recursive? ; whether to recurse into sub-modules + (lfs? git-reference-lfs? ;whether to fetch LFS files + (default #f)) + (recursive? git-reference-recursive? ;whether to recurse into sub-modules (default #f))) (define (git-package) @@ -79,11 +82,17 @@ (define (git-package) (let ((distro (resolve-interface '(gnu packages version-control)))) (module-ref distro 'git-minimal))) +(define (git-lfs-package) + "Return the default 'git-lfs' package." + (let ((distro (resolve-interface '(gnu packages version-control)))) + (module-ref distro 'git-lfs))) + (define* (git-fetch/in-band ref hash-algo hash #:optional name #:key (system (%current-system)) (guile (default-guile)) - (git (git-package))) + (git (git-package)) + (git-lfs (git-lfs-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). @@ -91,18 +100,22 @@ (define* (git-fetch/in-band ref hash-algo hash It will be removed when versions of guix-daemon implementing \"builtin:git-download\" will be sufficiently widespread." (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-reference-lfs? ref) + (list (or git-lfs (git-lfs-package))) + '()) ,@(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 +139,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 +147,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 +160,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,13 +190,15 @@ (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?" . ,(object->string + (git-reference-lfs? ref)))) #: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 @@ -209,6 +226,7 @@ (define* (git-fetch/built-in ref hash-algo hash (_ (git-reference-url ref))))) ("commit" . ,(git-reference-commit ref)) + ("lfs?" . ,(object->string (git-reference-lfs? ref))) ("recursive?" . ,(object->string (git-reference-recursive? ref)))) #:leaked-env-vars '("http_proxy" "https_proxy" diff --git a/guix/scripts/perform-download.scm b/guix/scripts/perform-download.scm index 9aa0e61e9d..37904941d1 100644 --- a/guix/scripts/perform-download.scm +++ b/guix/scripts/perform-download.scm @@ -96,6 +96,7 @@ (define* (perform-git-download drv output 'bmRepair' builds." (derivation-let drv ((url "url") (commit "commit") + (lfs? "lfs?") (recursive? "recursive?")) (unless url (leave (G_ "~a: missing Git URL~%") (derivation-file-name drv))) @@ -103,6 +104,7 @@ (define* (perform-git-download drv output (leave (G_ "~a: missing Git commit~%") (derivation-file-name drv))) (let* ((url (call-with-input-string url read)) + (lfs? (and lfs? (call-with-input-string lfs? read))) (recursive? (and recursive? (call-with-input-string recursive? read))) (drv-output (assoc-ref (derivation-outputs drv) "out")) @@ -115,6 +117,7 @@ (define* (perform-git-download drv output (setenv "PATH" "/run/current-system/profile/bin:/bin:/usr/bin") (git-fetch-with-fallback url commit output + #:lfs? lfs? #:recursive? recursive? #:git-command %git)))) diff --git a/guix/self.scm b/guix/self.scm index a1f235659d..96021be6f6 100644 --- a/guix/self.scm +++ b/guix/self.scm @@ -69,6 +69,7 @@ (define %packages ("gzip" . ,(ref 'compression 'gzip)) ("bzip2" . ,(ref 'compression 'bzip2)) ("xz" . ,(ref 'compression 'xz)) + ("git-lfs" . ,(ref 'version-control 'git-lfs)) ("git-minimal" . ,(ref 'version-control 'git-minimal)) ("po4a" . ,(ref 'gettext 'po4a)) ("gettext-minimal" . ,(ref 'gettext 'gettext-minimal)) @@ -830,6 +831,9 @@ (define* (compiled-guix source #:key (define git (specification->package "git-minimal")) + (define git-lfs + (specification->package "git-lfs")) + (define dependencies (append-map transitive-package-dependencies (list guile-gcrypt guile-gnutls guile-git guile-avahi @@ -1004,6 +1008,7 @@ (define* (compiled-guix source #:key #:bzip2 bzip2 #:xz xz #:git git + #:git-lfs git-lfs #:package-name %guix-package-name #:package-version @@ -1109,7 +1114,7 @@ (define %default-config-variables (%storedir . "/gnu/store") (%sysconfdir . "/etc"))) -(define* (make-config.scm #:key gzip xz bzip2 git +(define* (make-config.scm #:key gzip xz bzip2 git git-lfs (package-name "GNU Guix") (package-version "0") (channel-metadata #f) @@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git %store-database-directory %config-directory %git + %git-lfs %gzip %bzip2 %xz)) @@ -1184,6 +1190,8 @@ (define* (make-config.scm #:key gzip xz bzip2 git (define %git #+(and git (file-append git "/bin/git"))) + (define %git-lfs + #+(and git-lfs (file-append git-lfs "/bin/git-lfs"))) (define %gzip #+(and gzip (file-append gzip "/bin/gzip"))) (define %bzip2 -- 2.41.0
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Thu, 12 Oct 2023 02:53:01 GMT) Full text and rfc822 format available.Message #23 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: 66436 <at> debbugs.gnu.org Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Ludovic Courtès <ludo <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>, Christopher Baines <guix <at> cbaines.net> Subject: Re: [bug#66436] [PATCH 0/2] Add support for Git Large File Storage (LFS). Date: Wed, 11 Oct 2023 22:51:48 -0400
Hello, Maxim Cournoyer <maxim.cournoyer <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Sat, 14 Oct 2023 17:06:02 GMT) Full text and rfc822 format available.Message #26 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org Subject: Re: [bug#66436] [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references. Date: Sat, 14 Oct 2023 19:04:44 +0200
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > * gnu/packages/version-control.scm (git-lfs) > [arguments]: Add patch-/bin/sh phase. LGTM!
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Sat, 14 Oct 2023 17:14:01 GMT) Full text and rfc822 format available.Message #29 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>, 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net> Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS). Date: Sat, 14 Oct 2023 19:12:42 +0200
Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > * guix/git-download.scm (<git-reference>) [lfs?]: New field. > (git-fetch/in-band): 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/built-in): Add "lfs?" to #:env-vars. > * 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/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?' > environment variable. > * doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?' > field. > (Requirements): Mention the optional 'git-lfs' dependency. > * configure.ac: Add a check for the 'git-lfs' command. > * guix/config.scm.in (%git-lfs): New variable. > * guix/self.scm (%packages): Add git-lfs. > (compiled-guix): Add git-lfs to guix-config. > (make-config.scm): New #:git-lfs argument. I wonder whether this is a desirable feature, in the sense that the store is not designed to hold large amounts of data: just like Git has Git-LFS and Git-Annex, we’d need Guix-Annex (in fact, the GWL is kinda doing that already!). Furthermore… > +dnl Git Large File Storage is an optional dependency for the > +dnl "builtin:git-download" derivation builder. > +AC_PATH_PROG([GIT_LFS], [git-lfs]) > +if test "x$GIT_LFS" = "x"; then > + AC_MSG_WARN([Git Large File Storage (git-lfs) is missing; > + The builtin:git-download derivation builder of the Guix daemon will > + not be able to use it.]) … I don’t think we want to spend more words on the effect of increasing the closure size and getting locked with Git (libgit2 doesn’t implement LFS I suppose.) To me this part is a showstopper; we should just make it clear that “builtin:git-download” does not implement LFS. Also, it is crucial for the “builtin:git-download” semantics to be the same across all installations and to be very stable. > +(define* (make-config.scm #:key gzip xz bzip2 git git-lfs > (package-name "GNU Guix") > (package-version "0") > (channel-metadata #f) > @@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git > %store-database-directory > %config-directory > %git > + %git-lfs This I’d like to avoid, too (for the size issue). Overall, I feel like LFS support, if needed, should be “on the side”, with a custom ‘git-fetch/lfs’ or something along these lines (just like we have ‘url-fetch/tarbomb’). We might still need to change (guix build git) to implement it, but I would make sure that “builtin:git-download” remains unaffected and never fetches LFS stuff, even if ‘git-lfs’ happens to be on $PATH or something. WDYT? Ludo’.
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Sun, 15 Oct 2023 09:56:02 GMT) Full text and rfc822 format available.Message #32 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Josselin Poiret <dev <at> jpoiret.xyz> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, 66436 <at> debbugs.gnu.org Cc: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Sun, 15 Oct 2023 11:55:15 +0200
[Message part 1 (text/plain, inline)]
Hi Maxim, Maxim Cournoyer <maxim.cournoyer <at> 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
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Mon, 16 Oct 2023 14:03:02 GMT) Full text and rfc822 format available.Message #35 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Josselin Poiret <dev <at> jpoiret.xyz> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Mon, 16 Oct 2023 10:02:12 -0400
Hello, Josselin Poiret <dev <at> jpoiret.xyz> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Mon, 16 Oct 2023 14:48:02 GMT) Full text and rfc822 format available.Message #38 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 66436 <at> debbugs.gnu.org Subject: Re: [bug#66436] [PATCH 1/2] gnu: git-lfs: Patch /bin/sh references. Date: Mon, 16 Oct 2023 10:46:32 -0400
Hi, Ludovic Courtès <ludo <at> gnu.org> writes: > Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > >> * gnu/packages/version-control.scm (git-lfs) >> [arguments]: Add patch-/bin/sh phase. > > LGTM! I've installed this commit, thanks. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Mon, 16 Oct 2023 15:11:01 GMT) Full text and rfc822 format available.Message #41 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>, 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net> Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS). Date: Mon, 16 Oct 2023 11:10:16 -0400
Hi Ludovic, Ludovic Courtès <ludo <at> gnu.org> writes: > Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis: > >> * guix/git-download.scm (<git-reference>) [lfs?]: New field. >> (git-fetch/in-band): 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/built-in): Add "lfs?" to #:env-vars. >> * 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/scripts/perform-download.scm (perform-git-download): Honor the 'lfs?' >> environment variable. >> * doc/guix.texi (origin Reference) <git-reference>: Document the new 'lfs?' >> field. >> (Requirements): Mention the optional 'git-lfs' dependency. >> * configure.ac: Add a check for the 'git-lfs' command. >> * guix/config.scm.in (%git-lfs): New variable. >> * guix/self.scm (%packages): Add git-lfs. >> (compiled-guix): Add git-lfs to guix-config. >> (make-config.scm): New #:git-lfs argument. > > I wonder whether this is a desirable feature, in the sense that the > store is not designed to hold large amounts of data: just like Git has > Git-LFS and Git-Annex, we’d need Guix-Annex (in fact, the GWL is kinda > doing that already!). Interesting thoughts, though in the case of running the 'orcus' package test suite, the "large" files are simply .ods files (Open Document spreadsheet). It seems LFS is useful to store binary data files of any kind, notwithstanding their size. > Furthermore… > >> +dnl Git Large File Storage is an optional dependency for the >> +dnl "builtin:git-download" derivation builder. >> +AC_PATH_PROG([GIT_LFS], [git-lfs]) >> +if test "x$GIT_LFS" = "x"; then >> + AC_MSG_WARN([Git Large File Storage (git-lfs) is missing; >> + The builtin:git-download derivation builder of the Guix daemon will >> + not be able to use it.]) > > … I don’t think we want to spend more words on the effect of increasing > the closure size and getting locked with Git (libgit2 doesn’t implement > LFS I suppose.) To me this part is a showstopper; we should just make > it clear that “builtin:git-download” does not implement LFS. I thought we already did get locked with having git as a hard dependency. I guess you are keeping some hope for that to be reversed in the future, pending new libgit2 developments such as gaining garbage collection (git gc) support? git-lfs would increase the closure of guix from 688 to 700 MiB. > Also, it is crucial for the “builtin:git-download” semantics to be the > same across all installations and to be very stable. > >> +(define* (make-config.scm #:key gzip xz bzip2 git git-lfs >> (package-name "GNU Guix") >> (package-version "0") >> (channel-metadata #f) >> @@ -1140,6 +1145,7 @@ (define* (make-config.scm #:key gzip xz bzip2 git >> %store-database-directory >> %config-directory >> %git >> + %git-lfs > > This I’d like to avoid, too (for the size issue). > > Overall, I feel like LFS support, if needed, should be “on the side”, > with a custom ‘git-fetch/lfs’ or something along these lines (just like > we have ‘url-fetch/tarbomb’). > > We might still need to change (guix build git) to implement it, but I > would make sure that “builtin:git-download” remains unaffected and never > fetches LFS stuff, even if ‘git-lfs’ happens to be on $PATH or > something. > > WDYT? Your arguments make sense. I'll try to see how git-fetch/lfs could be implemented. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Mon, 16 Oct 2023 15:19:02 GMT) Full text and rfc822 format available.Message #44 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Simon Tournier <zimon.toutoune <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>, 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net> Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS). Date: Mon, 16 Oct 2023 17:15:39 +0200
Hi, I reorder Ludo’s answers for clarity. On Sat, 14 Oct 2023 at 19:12, Ludovic Courtès <ludo <at> gnu.org> wrote: > Also, it is crucial for the “builtin:git-download” semantics to be the > same across all installations and to be very stable. I agree and that’s one strong argument for me. > Overall, I feel like LFS support, if needed, should be “on the side”, > with a custom ‘git-fetch/lfs’ or something along these lines (just like > we have ‘url-fetch/tarbomb’). I agree with this direction. Most of the fetching methods should not be implemented with “builtin“ but the converse, git-fetch/lfs. ( Even, I think the current git-fetch using out-of-band builtin recently introduced should be a ’git-fetch/bootstrap’ method, and git-fetch using in-band should be the default method. ) Cheers, simon
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Mon, 16 Oct 2023 16:25:02 GMT) Full text and rfc822 format available.Message #47 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Simon Tournier <zimon.toutoune <at> gmail.com> Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>, Ludovic Courtès <ludo <at> gnu.org>, Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>, 66436 <at> debbugs.gnu.org, Christopher Baines <guix <at> cbaines.net> Subject: Re: [bug#66436] [PATCH 2/2] git-download: Add support for Git Large File Storage (LFS). Date: Mon, 16 Oct 2023 12:23:56 -0400
Hi, Simon Tournier <zimon.toutoune <at> gmail.com> writes: > Hi, > > I reorder Ludo’s answers for clarity. > > On Sat, 14 Oct 2023 at 19:12, Ludovic Courtès <ludo <at> gnu.org> wrote: > >> Also, it is crucial for the “builtin:git-download” semantics to be the >> same across all installations and to be very stable. > > I agree and that’s one strong argument for me. > >> Overall, I feel like LFS support, if needed, should be “on the side”, >> with a custom ‘git-fetch/lfs’ or something along these lines (just like >> we have ‘url-fetch/tarbomb’). > > I agree with this direction. Most of the fetching methods should not be > implemented with “builtin“ but the converse, git-fetch/lfs. > > ( Even, I think the current git-fetch using out-of-band builtin > recently introduced should be a ’git-fetch/bootstrap’ method, and > git-fetch using in-band should be the default method. ) Thanks for tipping in, I'll look into adding a whole new 'git-fetch/lfs' procedure. -- Thanks, Maxim
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Fri, 20 Oct 2023 08:13:01 GMT) Full text and rfc822 format available.Message #50 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Fri, 20 Oct 2023 10:12:11 +0200
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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Fri, 20 Oct 2023 23:03:02 GMT) Full text and rfc822 format available.Message #53 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Clément Lassieur <clement <at> lassieur.org> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Fri, 20 Oct 2023 19:01:41 -0400
Hi, Clément Lassieur <clement <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Sun, 22 Oct 2023 20:05:01 GMT) Full text and rfc822 format available.Message #56 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Clément Lassieur <clement <at> lassieur.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Sun, 22 Oct 2023 22:03:12 +0200
Hi Maxim, Maxim Cournoyer <maxim.cournoyer <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Mon, 23 Oct 2023 01:57:02 GMT) Full text and rfc822 format available.Message #59 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Clément Lassieur <clement <at> lassieur.org> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Sun, 22 Oct 2023 21:55:54 -0400
Hi, Clément Lassieur <clement <at> lassieur.org> writes: > Hi Maxim, > > Maxim Cournoyer <maxim.cournoyer <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 24 Oct 2023 09:02:03 GMT) Full text and rfc822 format available.Message #62 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Simon Tournier <zimon.toutoune <at> gmail.com> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Clément Lassieur <clement <at> lassieur.org> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Tue, 24 Oct 2023 10:49:35 +0200
Hi, On Fri, 20 Oct 2023 at 19:01, Maxim Cournoyer <maxim.cournoyer <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 24 Oct 2023 09:02:04 GMT) Full text and rfc822 format available.Message #65 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Simon Tournier <zimon.toutoune <at> gmail.com> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Clément Lassieur <clement <at> lassieur.org> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Tue, 24 Oct 2023 10:59:45 +0200
Re, On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 24 Oct 2023 15:55:01 GMT) Full text and rfc822 format available.Message #68 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Tue, 24 Oct 2023 17:54:11 +0200
Hi, Maxim Cournoyer <maxim.cournoyer <at> 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 <at> 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’.
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Wed, 25 Oct 2023 13:57:01 GMT) Full text and rfc822 format available.Message #71 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Simon Tournier <zimon.toutoune <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Wed, 25 Oct 2023 15:53:04 +0200
Hi On Tue, 24 Oct 2023 at 17:54, Ludovic Courtès <ludo <at> 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 <at> 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 <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Sun, 29 Oct 2023 14:54:02 GMT) Full text and rfc822 format available.Message #74 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Josselin Poiret <dev <at> jpoiret.xyz> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Sun, 29 Oct 2023 15:52:21 +0100
[Message part 1 (text/plain, inline)]
Hi Maxim, Maxim Cournoyer <maxim.cournoyer <at> 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
[signature.asc (application/pgp-signature, inline)]
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 31 Oct 2023 18:55:02 GMT) Full text and rfc822 format available.Message #77 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Simon Tournier <zimon.toutoune <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>, Clément Lassieur <clement <at> lassieur.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Tue, 31 Oct 2023 14:53:45 -0400
Hi Simon, Simon Tournier <zimon.toutoune <at> gmail.com> writes: > Re, > > On Tue, 24 Oct 2023 at 10:49, Simon Tournier <zimon.toutoune <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Tue, 31 Oct 2023 19:05:02 GMT) Full text and rfc822 format available.Message #80 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Simon Tournier <zimon.toutoune <at> gmail.com> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org>, Clément Lassieur <clement <at> lassieur.org> Subject: Re: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing. Date: Tue, 31 Oct 2023 20:03:47 +0100
Hi Maxim, On Tue, 31 Oct 2023 at 19:53, Maxim Cournoyer <maxim.cournoyer <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Wed, 01 Nov 2023 19:25:01 GMT) Full text and rfc822 format available.Message #83 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: 66436 <at> debbugs.gnu.org Cc: dev <at> jpoiret.xyz, ludo <at> gnu.org, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, zimon.toutoune <at> gmail.com Subject: [PATCH v3] doc: Add some guidelines for reviewing. Date: Wed, 1 Nov 2023 15:23:23 -0400
* 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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 <at> 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
guix-patches <at> gnu.org
:bug#66436
; Package guix-patches
.
(Sun, 05 Nov 2023 14:53:01 GMT) Full text and rfc822 format available.Message #86 received at 66436 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> Cc: 66436 <at> debbugs.gnu.org, dev <at> jpoiret.xyz, zimon.toutoune <at> gmail.com Subject: Re: [PATCH v3] doc: Add some guidelines for reviewing. Date: Sun, 05 Nov 2023 15:51:32 +0100
Hello, Maxim Cournoyer <maxim.cournoyer <at> 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’.
Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
:Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
:Message #91 received at 66436-done <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 66436-done <at> debbugs.gnu.org, dev <at> jpoiret.xyz, zimon.toutoune <at> gmail.com Subject: Re: [PATCH v3] doc: Add some guidelines for reviewing. Date: Tue, 07 Nov 2023 11:14:00 -0500
Hello, Ludovic Courtès <ludo <at> gnu.org> writes: > Hello, > > Maxim Cournoyer <maxim.cournoyer <at> 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
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org
.
(Wed, 06 Dec 2023 12:24:05 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.