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

[Adam Faiz]

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