Message ID | 20210524191054.7wtb4wopstgx2c77@silver |
---|---|
State | Accepted |
Headers | show |
Series | [bug#48630] adds `ack' package and it's perl module dependency File::Next | 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 |
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!
diff --git a/gnu/packages/perl.scm b/gnu/packages/perl.scm index d35508112f..87d541783e 100644 --- a/gnu/packages/perl.scm +++ b/gnu/packages/perl.scm @@ -4578,6 +4578,26 @@ that arise trying to find them consistently across a wide variety of platforms.") (license (package-license perl)))) +(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") + (sha256 + (base32 + "0zdrxk409qxkbbv4fl4wi285kfzyrpaja9wfl00vrxc078rs4afm")))) + (build-system perl-build-system) + (synopsis "File::Next is a lightweight, taint-safe file-finding Perl module") + (description "A Perl CPAN module for finding files. It has no non-core +prerequisites") + (home-page "https://metacpan.org/pod/File::Next") + (license license:artistic2.0))) + (define-public perl-file-path (package (name "perl-file-path") diff --git a/gnu/packages/textutils.scm b/gnu/packages/textutils.scm index ab34373705..7dd7ee80e2 100644 --- a/gnu/packages/textutils.scm +++ b/gnu/packages/textutils.scm @@ -45,6 +45,7 @@ #:use-module (guix build-system gnu) #:use-module (guix build-system go) #:use-module (guix build-system cmake) + #:use-module (guix build-system perl) #:use-module (guix build-system python) #:use-module (guix utils) #:use-module (gnu packages) @@ -1304,3 +1305,28 @@ languages such as HTML, Markdown, Asciidoc, and reStructuredText. The community around it also has a list of style guides implemented with Vale in @url{https://github.com/errata-ai/styles, their styles repo}.") (license license:expat))) + +(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)))) + (file-name "ack") + (sha256 + (base32 "00131vqjbzcn6w22m0h3j6x9kp59dimfnnqhpmi78vbcj0jws1dv")))) + (build-system perl-build-system) + (arguments '(#:tests? #f)) + (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 +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 '") + (home-page "https://beyondgrep.com/") + (license license:artistic2.0)))