| Message ID | 20220909123051.15747-1-maximedevos@telenet.be |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#57598,v2] doc: Update contribution guidelines on patches, etc. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git-branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi! Thanks for this welcome addition! Modulo the cosmetic suggestions below, I think it’s fine. Maintainers, if you have something to say on the guidelines, now’s the time! https://issues.guix.gnu.org/57598 Maxime Devos <maximedevos@telenet.be> skribis: > * doc/contributing.texi (Modifying Sources): Replaced with ... > ("Modifying Sources"): ... this. List more use cases and some principles. > > This patch incorporates some text written by Liliana Marie Prikler. The > structure is based on a proposal by Julien Lepiller. The text has been > revised based on reviews by David Larsson and blake. > > (! remove following text before committing !) You can write comments below the --- and diffstats. That way, ‘git am’ will ignore them when applying the patch. > +Guix has tree main ways of modifying the source code of a package, that > +you as a packager may use. These are patches, snippets and phases. ^ “may use: patches, snippets, and build phases.” > +Each one has its strengths and drawbacks. To decide between the three, s/decide between the three/choose among these/ > +there are a few guiding principles to satisfy simultanuously where > +possible: > + > +@itemize > +@item > +In principle, Guix only has free software; when the upstream source s/In principle, Guix only has free software/Every package in Guix is free software/g > +community (i.e., patterns that appear throughout @code{gnu/packages/...}) Normally such parenthetical expressions go between em dashes: community---i.e., patterns that appear throughout @file{gnu/packages}---and … > +To make things more concrete and to resolve conflicts between the > +principles, a few cases have been worked out: > + > +@subsubsection Removing non-free software > +Non-free software has to be removed in snippets; the reason is that > +patches or phases will not work. You can’t have a colon between the section heading. Instead, you can write “a few cases have been worked out and will be illustrated in the following sections.” Section titles should be capitalized. Leave a blank line after the section title. (Same comment for the sections that come after.) Instead of “will not work”, which is vague, I’d suggest more explicit wording: “would not be appropriate because they would expose the offending code.” > +For patches, the problem is that a patch removing a non-free file > +automatically contains the non-free file@footnote{It has been noted that > +git patches support removing files without including the file in the > +patch in > +@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If > +it is verified that the @command{patch} utility supports such patches, > +this method can be used and this policy adjusted appropriately.}, and we > +do not want anything non-free in Guix even if only in its patches. I’d drop the footnote, it’s already dense enough. > +@code{delete-file-recursively}. There are a few benefits for snippets here: > + > +When using snippets, the bundled library does not occur in the source s/snippets here:/snippets here./ s/When using snippets/First, when using snippets/ > +As such, snippets are recommended here. s/are recommended here/are the recommended way to delete non-free material/ > +@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...) In addition to capitalizing, please remove the parenthetical bit. > +@subsubsection Adding new functionality > +To add new functionality, a patch is almost always the most convenient > +choice of the three -- patches are usually multi-line changes, which are s/three -- patches/three---patches/ That’s all I have to say! I think what the text says is fine. It’s dense and rather long though, which means that people may overlook things. OTOH it’s structured so it’s easier to skim through it, and I wouldn’t know what to remove. Could you send an updated version? Thanks, Ludo’.
On 24-09-2022 15:03, Ludovic Courtès wrote: > Hi! > > Thanks for this welcome addition! Modulo the cosmetic suggestions > below, I think it’s fine. > > Maintainers, if you have something to say on the guidelines, now’s the > time! > > https://issues.guix.gnu.org/57598 > > Maxime Devos <maximedevos@telenet.be> skribis: > >> * doc/contributing.texi (Modifying Sources): Replaced with ... >> ("Modifying Sources"): ... this. List more use cases and some principles. >> >> This patch incorporates some text written by Liliana Marie Prikler. The >> structure is based on a proposal by Julien Lepiller. The text has been >> revised based on reviews by David Larsson and blake. >> >> (! remove following text before committing !) > > You can write comments below the --- and diffstats. That way, ‘git am’ > will ignore them when applying the patch. I know of '---', but AFAICT this is undocumented behaviour, so I'm hesitant to rely on that. > [...] > Could you send an updated version? I intend to do so after bringing antiox to 100%. Greetings, Maxime.
Maxime Devos writes: > On 24-09-2022 15:03, Ludovic Courtès wrote: [...] >> You can write comments below the --- and diffstats. That way, ‘git am’ >> will ignore them when applying the patch. > > I know of '---', but AFAICT this is undocumented behaviour, so I'm > hesitant to rely on that. I don't think you should be worried about Git changing that behavior. It's used heavily by the Git project itself as well as many other projects. Here are some mentions in git.git (4fd6c5e444): ,----[ Documentation/SubmittingPatches ] | You often want to add additional explanation about the patch, | other than the commit message itself. Place such "cover letter" | material between the three-dash line and the diffstat. For | patches requiring multiple iterations of review and discussion, | an explanation of changes between each iteration can be kept in | Git-notes and inserted automatically following the three-dash | line via `git format-patch --notes`. `---- ,----[ Documentation/git-notes.txt ] | Notes can also be added to patches prepared with `git format-patch` by | using the `--notes` option. Such notes are added as a patch commentary | after a three dash separator line. `---- ,----[ Documentation/user-manual.txt ] | `git format-patch` can include an initial "cover letter". You can insert | commentary on individual patches after the three dash line which | `format-patch` places after the commit message but before the patch | itself. If you use `git notes` to track your cover letter material, | `git format-patch --notes` will include the commit's notes in a similar | manner. `---- ,----[ Documentation/git-format-patch.txt ] | Typically it will be placed in a MUA's drafts folder, edited to add | timely commentary that should not go in the changelog after the three | dashes, and then sent as a message whose body, in our example, starts | with "arch/arm config files were...". On the receiving end, readers [...] `----
Hi, Ludovic Courtès <ludo@gnu.org> writes: > Hi! > > Thanks for this welcome addition! Modulo the cosmetic suggestions > below, I think it’s fine. > > Maintainers, if you have something to say on the guidelines, now’s the > time! > > https://issues.guix.gnu.org/57598 A couple things, on top of things you spot yourself and discussed by Liliana (although I haven't read the full thread, which is getting rather long): 1. s/tree/three/ 2. I think it'd be nice to mention that that the work of freeing software should not be done in Guix, but in upstream or if that fails as a second upstream project. That's easier to maintain longer term (more interested parties can share the burden), and friendlier to other FSDG distributions. Also, software containing nonfree software is risky: we have no reasons to trust they won't add more nonfree things to it, putting our users at risk, so deciding to include such software should be made carefully. It may be preferable to simply not include it, if the risks outweighs the benefits. 3. "guiding principles" sounds a bit too serious/strong to my taste. I think it's more guidelines we're discussing. 4. The source of the package needs to correspond to what is actually built (i.e., act as the corresponding source), to fulfill our ethical and legal obligations. This also seems worded too strongly -- I don't think Guix is bound to the concept of the "corresponding source", although it is a nice property. I find the new text rather long but it does add value, and I wouldn't know how to structure or improve it much, so I'm in favor of including it. Thanks, Maxim
Hi, Some work and energy had been put in these patches #57598 [1,2] and they appear to me a nice improvement. So what is missing? Some rewording here or there? Merge some wording from one to the other? 1: <https://issues.guix.gnu.org/issue/57598#msgid-1ba11b761cf81336b9788d5f0650b3742eed9d7f> 2: <https://issues.guix.gnu.org/issue/57598#msgid-7b0e5632669defb4d951b1cec54515bacc1c0a89> Cheers, simon
diff --git a/doc/contributing.texi b/doc/contributing.texi index 02c7c5ae59..bd2ae8d9b6 100644 --- a/doc/contributing.texi +++ b/doc/contributing.texi @@ -441,7 +441,7 @@ needed is to review and apply the patch. * Package Naming:: What's in a name? * 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. +* Modifying Sources:: Whether to use a snippet, or a build phase. * Emacs Packages:: Your Elisp fix. * Python Modules:: A touch of British comedy. * Perl Modules:: Little pearls. @@ -698,20 +698,130 @@ Gettext}): for the X11 resize-and-rotate (RandR) extension. @dots{}") @end lisp -@node Snippets versus Phases -@subsection Snippets versus Phases - -@cindex snippets, when to use -The boundary between using an origin snippet versus a build phase to -modify the sources of a package can be elusive. Origin snippets are -typically used to remove unwanted files such as bundled libraries, -nonfree sources, or to apply simple substitutions. The source derived -from an origin should produce a source that can be used to build the -package on any system that the upstream package supports (i.e., act as -the corresponding source). In particular, origin snippets must not -embed store items in the sources; such patching should rather be done -using build phases. Refer to the @code{origin} record documentation for -more information (@pxref{origin Reference}). +@node Modifying Sources +@subsection Modifying Sources + +Guix has tree main ways of modifying the source code of a package, that +you as a packager may use. These are patches, snippets and phases. +Each one has its strengths and drawbacks. To decide between the three, +there are a few guiding principles to satisfy simultanuously where +possible: + +@itemize +@item +In principle, Guix only has free software; when the upstream source +contains some non-free software, it has to be removed such that +@command{guix build --source} returns the ‘freed’ source code rather than +the unmodified upstream source (@pxref{Software Freedom}). +@item +The source of the package needs to correspond to what is actually built +(i.e., act as the corresponding source), to fulfill our ethical and +legal obligations. +@item +The source needs to actually work, not only on your Guix system but also +on other Guix systems. It preferably should also work on non-Guix +systems if supported by upstream. This requires some care for +substitutions involving store items and other architecture-specific +changes. +@item +When there is more than one way to do something, choose whichever method +is the simplest. Sometimes this is subjective, which is also fine. +What matters is that you use techniques that are common within the +community (i.e., patterns that appear throughout @code{gnu/packages/...}) +and are thus clearly legible for reviewers. +@end itemize + +To make things more concrete and to resolve conflicts between the +principles, a few cases have been worked out: + +@subsubsection Removing non-free software +Non-free software has to be removed in snippets; the reason is that +patches or phases will not work. + +For patches, the problem is that a patch removing a non-free file +automatically contains the non-free file@footnote{It has been noted that +git patches support removing files without including the file in the +patch in +@url{https://yhetil.org/guix/8b13e899-eb60-490b-a268-639249698c81@@www.fastmail.com/}. If +it is verified that the @command{patch} utility supports such patches, +this method can be used and this policy adjusted appropriately.}, and we +do not want anything non-free in Guix even if only in its patches. + +For phases, the problem is that phases do not influence the result of +@command{guix build --source}. + +@subsubsection Removing bundled libraries +Bundled libraries should not be removed with patches, because then the +patch would contain the full bundled library, which can be large. They +can be removed either in snippets or phases, often using the procedure +@code{delete-file-recursively}. There are a few benefits for snippets here: + +When using snippets, the bundled library does not occur in the source +returned by @code{guix build --source}, so users and reviewers do not +have to worry about whether the bundled library contains malware, +whether it is non-free, if it contains pre-compiled binaries ... There +are also less licensing concerns: if the bundled libraries are removed, +it becomes less likely that the licensing conditions apply to people +sharing the source returned by @command{guix build --source}, especially if +the bundled library is not actually used on Guix systems.@footnote{This +is @emph{not} a claim that you can simply ignore the licenses of +libraries when they are unbundled and replaced by Guix packages -- there +are less concerns, not none.} + +As such, snippets are recommended here. + +@subsubsection Fixing technical issues (compilation errors, test failures, other bugs ...) +Usually, a bug fix comes in the form of a patch copied from upstream or +another distribution. In that case, simply adding the patch to the +@code{patches} field is the most convenient and usually does not cause +any problems; there is no need to rewrite it as a snippet or a phase. + +If no ready-made patch already exists, then choosing between a patch or +a snippet is a matter of convenience. However, there are two things to +keep in mind: + +First, when the fix is not Guix-specific, upstreaming the fix is +strongly desired to avoid the additional maintenance cost to Guix. As +upstreams cannot accept snippets, writing a patch can be a more +efficient use of time. Secondly, if the fix of a technical issue embeds +a store file name, then it has to be a phase. Otherwise, if the store +file name were embedded in the source, the result of @command{guix build +--source} would be unusable on non-Guix systems and also likely unusable +on Guix systems of another architecture. + +@subsubsection Adding new functionality +To add new functionality, a patch is almost always the most convenient +choice of the three -- patches are usually multi-line changes, which are +convenient to do with patches and inconvenient to do with phases or +snippets. This choice is usually also compatible with all the guiding +principles. As such, patches are preferred. However, as with bug +fixes, upstreaming the new functionality is desired. + +@subsubsection How to add patches +Refer to the @code{origin} record documentation (particularly the fields +@code{patches}, @code{patch-inputs}, and @code{patch-flags}) for +information on how to use patches (@pxref{origin Reference}). When +adding a patch, do not forget to also list it in @code{dist_patch_DATA} +of @file{gnu/local.mk}. + +@subsubsection How to add files +New source files can be added in phases or snippets, by using +@b{auxiliarry files}. Auxiliary files are stored in the +@file{gnu/packages/aux-files} directory and can be retrieved (in a +snippet or a phase) with @code{search-auxiliary-file}. When adding an +auxiliary file, do not forget to also list it in @code{AUX_FILES} of +@file{Makefile.am}. + +Another option for adding new files, is to use procedures such as +@code{display}, @code{format} and @code{call-with-output-file}. As a +matter of principle, auxiliary files ought to be preferred over an +equivalent @code{call-with-output-file} when creating non-trivil files, +as that makes them easier to edit. The exact threshold for a +non-trivial file might be subjective, though it should lie somewhere +between 10--20 lines. + +Currently, there is no policy on deciding between phase and snippets for +adding new files, except for the guiding principles. @node Emacs Packages @subsection Emacs Packages diff --git a/doc/guix.texi b/doc/guix.texi index 7bce8a567c..042ab3bd8a 100644 --- a/doc/guix.texi +++ b/doc/guix.texi @@ -70,7 +70,7 @@ Copyright @copyright{} 2019 Jakob L. Kreuze@* Copyright @copyright{} 2019 Kyle Andrews@* Copyright @copyright{} 2019 Alex Griffin@* Copyright @copyright{} 2019, 2020, 2021, 2022 Guillaume Le Vaillant@* -Copyright @copyright{} 2020 Liliana Marie Prikler@* +Copyright @copyright{} 2020, 2022 Liliana Marie Prikler@* Copyright @copyright{} 2019, 2020, 2021, 2022 Simon Tournier@* Copyright @copyright{} 2020 Wiktor Żelazny@* Copyright @copyright{} 2020 Damien Cassou@*
* doc/contributing.texi (Modifying Sources): Replaced with ... ("Modifying Sources"): ... this. List more use cases and some principles. This patch incorporates some text written by Liliana Marie Prikler. The structure is based on a proposal by Julien Lepiller. The text has been revised based on reviews by David Larsson and blake. (! remove following text before committing !) Unless someone finds some more typo's or such, this is the final version for me, as it has become a time sink with diminishing returns. Changes since previous version: - typofix: tree -> three - third and fourth principle are merged - corrected @subsection -> @subsubsection Not changed: - no additional information on bugfixes even though initially proposed, I'm done with the patch, if you'd like such information please write a follow-up patch. - I didn't add 'no naming the file names of non-free things in the snippets’, as I don't think this is currently policy and it seems rather inconvenient to work with (e.g.: then we wouldn't be able to keep records of what parts still need to be replaced with something free in the comments) - going from ‘problem -> solution’ to ‘solution -> problem’ -- would partially defeats the point of the patch. - going through the two different patches and take the most concise phrasing (other people in 57598@debbugs.gnu.org didn't agree to my proposal, also, I'm about done with the patch - ‘subsubsections -> item / table’: I haven't read a convincing explanation on why item / table is better here. --- doc/contributing.texi | 140 +++++++++++++++++++++++++++++++++++++----- doc/guix.texi | 2 +- 2 files changed, 126 insertions(+), 16 deletions(-) base-commit: a44e08337d15b3f254a35d0311663c2bbd501852 prerequisite-patch-id: 0caac311875ee39cb48573657ebb960e90da6dfb prerequisite-patch-id: 418285493d89ebf102175902d9b09a0174e88190 prerequisite-patch-id: 3c39eb839d9d3ff3fca6cd98621a5d5c411b7af4 prerequisite-patch-id: 8d5662e874c469f5ee496ef5181cf2d0a30ad1d8 prerequisite-patch-id: 26513c3b3b86963df718ee41d14a25d1cc6a8f3f prerequisite-patch-id: 2b2497e2edec0afc48ebadd6f09f0c661c466127 prerequisite-patch-id: 2712efb97bf33985fd0658e4dd8e936dc08be5fe prerequisite-patch-id: 9d2409b480a8bff0fef029b4b095922d4957e06f