[bug#48630] adds `ack' package and it's perl module dependency File::Next

modified:   gnu/packages/perl.scm
 - adds package `perl-file-next' (module File::Next)

modified:   gnu/packages/textutils.scm
 - adds package `ack' with disabled tests
---
 gnu/packages/perl.scm      | 20 ++++++++++++++++++++
 gnu/packages/textutils.scm | 26 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

Gabriel Wicki schreef op ma 24-05-2021 om 21:10 [+0200]:
> [...]
> +(define-public ack
> +  (package
> +   (name "ack")
> +   (version "3.5.0")
> +   (source (origin
> +            (method git-fetch)
> +            (uri
> +             (git-reference
> +              (url "https://github.com/beyondgrep/ack3")
> +              (commit (format #nil "v~a" version))))

#f is an elisp compatibility hack in Guile.
Use (format #f "v~a" version) instead, or simpler
(string-append "v" version).

Looking at the patch only and (I didn't test it, didn't do
licensing checks, etc.) I only see one other problem:

> +   (arguments '(#:tests? #f))

Why are the tests disabled? (Are there none, do they need
additional packages not in Guix, ...) Maybe add a line like

> +   ;; There is no test suite.
> +   (arguments '(#:tests? #f))

Greetings,
Maxime.

On Mon, May 24 2021, Gabriel Wicki wrote:

> modified:   gnu/packages/perl.scm
>  - adds package `perl-file-next' (module File::Next)
>
> modified:   gnu/packages/textutils.scm
>  - adds package `ack' with disabled tests

Commit messages should be written in ChangeLog format[1], you can look
at previous commits for examples.

This commit should also be split into two separate commits, one adding
‘perl-file-next’, and the other adding ‘ack’.

> +(define-public perl-file-next
> +  (package
> +   (name "perl-file-next")
> +   (version "1.18")
> +   (source (origin
> +            (method git-fetch)
> +            (uri (git-reference
> +                  (url "https://github.com/petdance/file-next")
> +                  (commit version)))
> +            (file-name "perl-next-file")

Use ‘(file-name (git-file-name name version))’.

> +            (sha256
> +             (base32
> +              "0zdrxk409qxkbbv4fl4wi285kfzyrpaja9wfl00vrxc078rs4afm"))))
> +   (build-system perl-build-system)
> +   (synopsis "File::Next is a lightweight, taint-safe file-finding Perl module")

Try to make the synopsis as concise as possible, just “Lightweight,
taint-safe file-finding Perl module” should do.  See the ‘Synopses and
Descriptions’ section of the manual.

> +   (description "A Perl CPAN module for finding files.  It has no non-core
> +prerequisites")

The description should contain one or more complete sentences.

> +(define-public ack
> +  (package
> +   (name "ack")
> +   (version "3.5.0")
> +   (source (origin
> +            (method git-fetch)
> +            (uri
> +             (git-reference
> +              (url "https://github.com/beyondgrep/ack3")
> +              (commit (format #nil "v~a" version))))

We usually use ‘(string-append "v" version)’ instead of ‘format’.

> +            (file-name "ack")

Use ‘(file-name (git-file-name name version))’ (same as above).

> +            (sha256
> +             (base32 "00131vqjbzcn6w22m0h3j6x9kp59dimfnnqhpmi78vbcj0jws1dv"))))
> +   (build-system perl-build-system)
> +   (arguments '(#:tests? #f))

What’s the reason for disabling the tests?

> +   (propagated-inputs `(("perl-file-next" ,perl-file-next)))
> +   (synopsis "Code-searching tool optimized for programmers with large trees
> +of source code")
> +   (description "ack is a tool for finding text inside files. it is designed for
                                                                ^^
Please use double spacing, and capitalize.

> +hackers and programmers by being fast, ignoring VCS directories, letting a user
> +easily specify file types, match highlighting, Perl-Compatible Regular
> +Expressions, and being faster to type than `grep '")
                                                   ^^
Unecessary whitespace, and missing period.

Could you send an updated series?

[1]: https://www.gnu.org/prep/standards/html_node/Change-Logs.html#Change-Logs

thanks for the advice, i'll update and split the patch into a small
series soon!


the tests are deactivated because 3 of 841 fail. they seem to check
some kind of shebang #! paths. see this excerpt from the build log:

#   Failed test 'Util::sets_match( TEST_TYPE with -t perl )'
#   at t/ack-type.t line 43.

	# actual[
	#   't/swamp/0:1:#!/gnu/store/8zvc5mvk0xm3ygrxsgpyy5ilxb5rzjry-perl-5.30.2/bin/perl -w',
...
	# ]
	# expected[
	#   't/swamp/0:1:#!/usr/bin/perl -w',
...
	# ]

these lines are being ignored by the patch-generated-file-shebangs
phase, maybe because the #! aren't at the beginning of the line?

i'm a total novice in Perl, so i'm not sure what exactly is happening
there or why this is even tested.

should i try and add my own version of a patch-shebang after the
patch-generated-file-shebangs phase?

On Tue, May 25 2021, Gabriel Wicki wrote:

> thanks for the advice, i'll update and split the patch into a small
> series soon!

Great! :)

> the tests are deactivated because 3 of 841 fail. they seem to check
> some kind of shebang #! paths. see this excerpt from the build log:
>
> #   Failed test 'Util::sets_match( TEST_TYPE with -t perl )'
> #   at t/ack-type.t line 43.
>
> 	# actual[
> 	#   't/swamp/0:1:#!/gnu/store/8zvc5mvk0xm3ygrxsgpyy5ilxb5rzjry-perl-5.30.2/bin/perl -w',
> ...
> 	# ]
> 	# expected[
> 	#   't/swamp/0:1:#!/usr/bin/perl -w',
> ...
> 	# ]
>
> these lines are being ignored by the patch-generated-file-shebangs
> phase, maybe because the #! aren't at the beginning of the line?
>
> i'm a total novice in Perl, so i'm not sure what exactly is happening
> there or why this is even tested.
>
> should i try and add my own version of a patch-shebang after the
> patch-generated-file-shebangs phase?

Yes, you could add add a phase after the ‘patch-generated-file-shebangs’
phase that replaces #!/usr/bin/perl with #!/gnu/store/...-perl/bin/perl.
Something like this should work (untested):

#+begin_src scheme
(add-after 'patch-generated-file-shebangs 'patch-more-shebangs
  (lambda (#:key inputs #:allow-other-keys)
    (substitute* '("t/ack-type.t" ...)
      (let ((perl (assoc-ref inputs "perl")))
        (("/usr/bin/perl")
         (string-append perl "/bin/perl"))))))
#+end_src

Commit 1051db25267a9bbc0ad38be2f3ec92af40f18e18 adds ack.  I’ve
reformatted your patch a little, but nothing fundamental has been
changed.

Thank you for your patience!  Let’s hope your next patch gets noticed
sooner.  Feel free to ping us on IRC if you think we may have forgotten
about your contributions.

Thanks again!