| Message ID | 20220610150448.9513-1-remco@remworks.net |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#55891] gnu: Add iec16022 | 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 |
Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: > + (arguments > + '(#:phases (modify-phases %standard-phases > + (add-before 'bootstrap 'disable-autogen-configure > + (lambda _ > + (substitute* "autogen.sh" > + (("^\\./configure") "# nop"))))))) As per <https://logs.guix.gnu.org/guix/2022-06-09.log#194339>, I recommend doing eliminating the ' in favour of 'list': (arguments (list #:phases #~(modify-phases [...]))) Greetings, Maxime.
Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]:
> * gnu/packages/iec16022.scm: New file.
We usually don't do per-package modules in Guix, maybe it would fit
next to the barcode packages in (gnu packages aidc)?
Though aidc refers to a particular company, so maybe rename (gnu
packages aidc) to (gnu packages barcodes) first?
Greetings,
Maxime.
Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: > + "Iec16022 is a non-interactive software package for producing Data Matrix > +barcodes as specified by ISO/IEC 16022. It produces ASCII, UTF-8, EPS, Bin, > +Stamp and PNG images.") Can be improved by using the @acronym TeXinfo markup for ISO, IEC, ASCII, UTF-8, EPS and PNG. Or maybe that would be TMI here, dunno. Greetings, Maxime.
Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: > + (source (origin > + (method git-fetch) > + (uri (git-reference > + (url "https://github.com/rdoeffinger/iec16022") > + (commit "c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51"))) > + (file-name (git-file-name name version)) > + (sha256 Contains a bug: on line https://github.com/rdoeffinger/iec16022/blob/c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51/image.c#L284 , it allocates a buffer, which can fail (by returning NULL). It then passes the possibly NULL buffer to compress2. Also, it writes files without checking for errors (e.g. EIO, ENOSPC or EDQUOT). Greetings, Maxime.
2022/06/10 17:42, Maxime Devos: > Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: >> + (arguments >> + '(#:phases (modify-phases %standard-phases >> + (add-before 'bootstrap 'disable-autogen-configure >> + (lambda _ >> + (substitute* "autogen.sh" >> + (("^\\./configure") "# nop"))))))) > > As per <https://logs.guix.gnu.org/guix/2022-06-09.log#194339>, I > recommend doing eliminating the ' in favour of 'list': > > (arguments > (list #:phases > #~(modify-phases [...]))) done
2022/06/10 17:45, Maxime Devos: > Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: >> * gnu/packages/iec16022.scm: New file. > > We usually don't do per-package modules in Guix, maybe it would fit > next to the barcode packages in (gnu packages aidc)? > > Though aidc refers to a particular company, so maybe rename (gnu > packages aidc) to (gnu packages barcodes) first? > > Greetings, > Maxime. Done, minus rename, I don't feel comfortable doing that.
2022/06/10 17:47, Maxime Devos: > Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: >> + "Iec16022 is a non-interactive software package for producing Data Matrix >> +barcodes as specified by ISO/IEC 16022. It produces ASCII, UTF-8, EPS, Bin, >> +Stamp and PNG images.") > > Can be improved by using the @acronym TeXinfo markup for ISO, IEC, > ASCII, UTF-8, EPS and PNG. Or maybe that would be TMI here, dunno. TMI IMHO. No other package descriptions bother to use @acronym for thing like ASCII, PNG etc.
2022/06/10 17:55, Maxime Devos: > Remco van 't Veer schreef op vr 10-06-2022 om 17:04 [+0200]: >> + (source (origin >> + (method git-fetch) >> + (uri (git-reference >> + (url "https://github.com/rdoeffinger/iec16022") >> + (commit "c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51"))) >> + (file-name (git-file-name name version)) >> + (sha256 > > Contains a bug: on line > > https://github.com/rdoeffinger/iec16022/blob/c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51/image.c#L284 > > , it allocates a buffer, which can fail (by returning NULL). > It then passes the possibly NULL buffer to compress2. > Also, it writes files without checking for errors > (e.g. EIO, ENOSPC or EDQUOT). Good catch! I'll need to brush up on my C skills to fix these. Meanwhile I'd like to ship including bugs, WDYT?
Remco van 't Veer schreef op vr 10-06-2022 om 19:39 [+0200]: > > Contains a bug: on line > > > > https://github.com/rdoeffinger/iec16022/blob/c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51/image.c#L284 > > > > , it allocates a buffer, which can fail (by returning NULL). > > It then passes the possibly NULL buffer to compress2. > > Also, it writes files without checking for errors > > (e.g. EIO, ENOSPC or EDQUOT). > > Good catch! I'll need to brush up on my C skills to fix these. > Meanwhile I'd like to ship including bugs, WDYT? I think it's ok, _if_ the bugs are eventually at least reported upstream if we (i.e., you) cannot fix them timely (needs some contrived circumstances to trigger the bug). Greetings, Maxime.
2022/06/10 23:47, Maxime Devos: > Remco van 't Veer schreef op vr 10-06-2022 om 19:39 [+0200]: >> > Contains a bug: on line >> > >> > https://github.com/rdoeffinger/iec16022/blob/c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51/image.c#L284 >> > >> > , it allocates a buffer, which can fail (by returning NULL). >> > It then passes the possibly NULL buffer to compress2. >> > Also, it writes files without checking for errors >> > (e.g. EIO, ENOSPC or EDQUOT). >> >> Good catch! I'll need to brush up on my C skills to fix these. >> Meanwhile I'd like to ship including bugs, WDYT? > > I think it's ok, _if_ the bugs are eventually at least reported > upstream if we (i.e., you) cannot fix them timely (needs some contrived > circumstances to trigger the bug). https://github.com/rdoeffinger/iec16022/pull/17
2022/06/10 23:47, Maxime Devos: > I think it's ok, _if_ the bugs are eventually at least reported > upstream if we (i.e., you) cannot fix them timely (needs some > contrived circumstances to trigger the bug). The maintainer's stance on not checking malloc response (from https://github.com/rdoeffinger/iec16022/pull/17#issuecomment-1152892833): > > I'll open an issue for the unchecked [mc]alloc and write calls > > instead if you do and maybe have another stab at fixing these issues > > when my C-programming skills have improved. > > I agree on the unchecked writes. For the unchecked [mc]alloc I am more > tempted towards removing the existing checks, since I am not convinced > checking won't always do more harm than good for those. He elaborated on this at: https://github.com/rdoeffinger/iec16022/pull/17#issuecomment-1153103749 Boiling down to: when memory is that low exiting with a proper error messages and doing cleanup probably won't work anyway and the OOM killer will have swept in before the situation exists. Which makes sense, IMHO, but I am no expert. I've created an issue for the unchecked writes: https://github.com/rdoeffinger/iec16022/issues/18 HTH, Remco
Is there anything else I can do to keep this ticket moving? Bug about unchecked IO is reported and discussion about unchecked malloc has stalled. Also note, this package is also being shipped by debian, archlinux, fedora, and others. Not that it matters but just to point out it's a widely spread utility. Cheers, Remco
On 01-07-2022 08:58, Remco van 't Veer wrote: > Is there anything else I can do to keep this ticket moving? Bug about > unchecked IO is reported and discussion about unchecked malloc has > stalled. You could search for an alternative solution that is acceptable to upstream, or if upstream completely refuses bugfixes, add some patch to the Guix package definition. Also, I have not yet seen a v2 for resolving the other (non-upstream-code) issues I noted. Another method would be to help out with reviewing and fixing things (with other packages), this frees up time and is seen as a good act, hence making other people more inclined to help out with any remaining issues in your patches (including fixing issues such as those of the kind I have mentioned before, but also actually applying the patch and committing it). You could also perform _all_ the checks in (guix)Submitting Patches (e.g., the bundling check, actually building it for other systems as well with QEMU, reproducibility check). > Also note, this package is also being shipped by debian, archlinux, > fedora, and others. Not that it matters but just to point out it's a > widely spread utility. I am not convinced by your implied method of persuasion by social pressure / argumentum ad populum. If it doesn't matter, why use it as an argument? Greetings, Maxime.
Hi Maxime, Sorry for the late reply, I was out camping. 2022/07/31 01:48, Maxime Devos: > On 01-07-2022 08:58, Remco van 't Veer wrote: > >> Is there anything else I can do to keep this ticket moving? Bug about >> unchecked IO is reported and discussion about unchecked malloc has >> stalled. > > You could search for an alternative solution that is acceptable to > upstream, or if upstream completely refuses bugfixes, add some patch > to the Guix package definition. Also, I have not yet seen a v2 for > resolving the other (non-upstream-code) issues I noted. The patches I did submit upstream were not accepted and I agree with the arguments of the maintainer so I won't add these to this package definition. https://github.com/rdoeffinger/iec16022/pull/17 Also, I did make a v2 but forgot to cc you, sorry about that: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=55891#20 Also did a v3 because the copyright line was missing (and failed to cc you again): https://debbugs.gnu.org/cgi/bugreport.cgi?bug=55891#35 > Another method would be to help out with reviewing and fixing things > (with other packages), this frees up time and is seen as a good act, > hence making other people more inclined to help out with any remaining > issues in your patches (including fixing issues such as those of the > kind I have mentioned before, but also actually applying the patch and > committing it). That's a great tip and I will try and leap over my newby/shyness feelings to get into that. Getting feedback like yours has certainly thought me a lot about what to look out for. Thank you for your feedback so far, also on the other patches I've submitted! > You could also perform _all_ the checks in (guix)Submitting Patches > (e.g., the bundling check, actually building it for other systems as > well with QEMU, reproducibility check). Just ran through them again and all seems to be fine. I am not sure how to prove I did. >> Also note, this package is also being shipped by debian, archlinux, >> fedora, and others. Not that it matters but just to point out it's a >> widely spread utility. > > I am not convinced by your implied method of persuasion by social > pressure / argumentum ad populum. If it doesn't matter, why use it as > an argument? What I should have written is I am moving and application from ubuntu/debian to guix and am missing this package. We already use a custom channel to make development possible but in doing to I was tickled by (guix)Creating a Channel mentioning: Before publishing a channel, please consider contributing your package definitions to Guix proper (*note Contributing::). Guix as a project is open to free software of all sorts, and packages in Guix proper are readily available to all Guix users and benefit from the project’s quality assurance process. Cheers, Remco
diff --git a/gnu/local.mk b/gnu/local.mk index abd6a30d66..8b3ad229aa 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -313,6 +313,7 @@ GNU_SYSTEM_MODULES = \ %D%/packages/image-processing.scm \ %D%/packages/image-viewers.scm \ %D%/packages/imagemagick.scm \ + %D%/packages/iec16022.scm \ %D%/packages/inklingreader.scm \ %D%/packages/inkscape.scm \ %D%/packages/installers.scm \ diff --git a/gnu/packages/iec16022.scm b/gnu/packages/iec16022.scm new file mode 100644 index 0000000000..b8b0fe0176 --- /dev/null +++ b/gnu/packages/iec16022.scm @@ -0,0 +1,56 @@ +;;; GNU Guix --- Functional package management for GNU +;;; Copyright © 2022 Remco van 't Veer <remco@remworks.net> +;;; +;;; This file is part of GNU Guix. +;;; +;;; GNU Guix is free software; you can redistribute it and/or modify it +;;; under the terms of the GNU General Public License as published by +;;; the Free Software Foundation; either version 3 of the License, or (at +;;; your option) any later version. +;;; +;;; GNU Guix is distributed in the hope that it will be useful, but +;;; WITHOUT ANY WARRANTY; without even the implied warranty of +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +;;; GNU General Public License for more details. +;;; +;;; You should have received a copy of the GNU General Public License +;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>. + +(define-module (gnu packages iec16022) + #:use-module (guix packages) + #:use-module (guix git-download) + #:use-module (guix build-system gnu) + #:use-module (guix licenses) + #:use-module (guix gexp) + #:use-module (gnu packages autotools) + #:use-module (gnu packages popt)) + +(define-public iec16022 + (package + (name "iec16022") + (version "0.3.1") + (source (origin + (method git-fetch) + (uri (git-reference + (url "https://github.com/rdoeffinger/iec16022") + (commit "c9a9fd926fd1e6cca9782fe3f8c2bab66010ca51"))) + (file-name (git-file-name name version)) + (sha256 + (base32 + "1k9hfcjcm64w0n20979k2bx5lmxqhsh6df11qxyjdy4aq0l6i62z")))) + (build-system gnu-build-system) + (arguments + '(#:phases (modify-phases %standard-phases + (add-before 'bootstrap 'disable-autogen-configure + (lambda _ + (substitute* "autogen.sh" + (("^\\./configure") "# nop"))))))) + (inputs (list popt)) + (native-inputs (list autoconf automake libtool)) + (synopsis "Command-line tool for producing Data Matrix barcodes") + (description + "Iec16022 is a non-interactive software package for producing Data Matrix +barcodes as specified by ISO/IEC 16022. It produces ASCII, UTF-8, EPS, Bin, +Stamp and PNG images.") + (home-page "https://github.com/rdoeffinger/iec16022") + (license gpl2+)))