[bug#59355] [PATCH] gnu: Add dataparksearch.

[Adam Faiz via Guix-patches via <guix-patches@gnu.org>]

On 2/18/23 08:13, Tobias Geerinckx-Rice wrote:
> Hi Adam,
> 
> Adam Faiz via Guix-patches via 写道：
>> * gnu/packages/search.scm (dataparksearch): New variable.
> 
> Thanks!  I've applied the patch locally but stopped short of pushing.  I 
> have several questions and [notes]:
> 
>> +(define-public dataparksearch
>> +  (package
>> +    (name "dataparksearch")
>> +    (version "4.54-2016-12-03")
> 
> I don't think this tag is anything special compared to other commits.  
> Is it?
> 
> There are many more recent commits, up to 2022.  If the argument is 
> ‘4.53 is ancient’ (it is!), should we just package the latest commit?
> Would the latest release (4.53) instead still be useful today?
Not really, I think it's best to package the latest commit.

>> +    (source (origin
>> +              (method git-fetch)
>> +              (uri (git-reference
>> +                    (url "https://github.com/Maxime2/dataparksearch")
>> +                    (commit version)))
>> +              (sha256
>> +               (base32
>> "1g5kxw2d8jbc1h9yyy0xpnd3gkscj5a32k6hk3brvdwcbsnjbgyg"))
>> +              (modules '((guix build utils)))
>> +              (snippet
>> +               #~(begin
>> +                   (for-each delete-file-recursively '("config.sub"
>> + "config.guess"
>> + "configure"
> 
> [All checked into Git.  Nice.  This snippet is ‘ugly’ because it 
> addresses an ugly problem.  I think I'll keep it but add an apologetic 
> comment.]
Sure.

>> + "Makefile.in"
>> + "missing"
>> + "depcomp"
>> + "ltmain.sh"
>> + "compile"
>> +                                                       ))))
> 
> [We don't dangle brackets like this but keep them on the previous line.]
I dangled the brackets because I can't keep track of the brackets when I 
was working on the package. It's about time I try out paredit or some 
solution that colours each bracket.

>> +              (file-name (git-file-name name version))))
>> +    (build-system gnu-build-system)
>> +    (native-inputs
>> +     (list pkg-config automake autoconf libtool openjade))
>> +    (inputs
>> +     (list mbedtls-apache zlib postgresql aspell c-ares libextractor

> [For consistency with other packages, I've moved the …inputs below the 
> arguments field.]
Alright, my logic was to specify the inputs used before running the 
build commands since that's the normal flow for a packager.

> 
>> +     (list #:configure-flags
>> +           #~(list "--disable-syslog"
> 
> Is this not useful?  If not, could you provide a short comment 
> explaining why?
It's useful for people who use syslog, but syslog itself is suboptimal 
for its purpose. More info can be found here:
http://skarnet.org/software/s6/s6-log.html#diesyslogdiedie
Since the Shepherd (and other init systems) can keep the log output of 
the services it supervises, I was hoping that could be used instead.

>> +                   "--with-gnu-ld"
> 
> And here?
That's unnecessary, I just wanted to try it out for fun.

> 
>> +               (add-before 'bootstrap 'fix-configure
>> +                 (lambda _
>> +                   (substitute* "configure.ac"
>> +                     (("MY_DIRS=\"/usr/local/include")
>> +                      (string-append "MY_DIRS=\""
>> #$(this-package-input "aspell") "/include"))
>> +                     (("MY_DIRS=\"/usr/lib")
>> +                      (string-append "MY_DIRS=\""
>> #$(this-package-input "aspell") "/lib"))))))))
> 
> Why not use --with-aspell=… as you did for Postgres?
It's not available in 4.53, which is another reason why the latest 
commit should be used.

>> +    (license license:gpl3+)))
> 
> Why GPL3+?
It was a mistake.

> [I was unable to get this package to build reproducibly, although I 
> tried only disabling #:parallel-build?.]
I'll send an updated patch which fixes the issues you mentioned.

> Kind regards,
> 
> T G-R