all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Adam Faiz via Guix-patches via <guix-patches@gnu.org>
To: Tobias Geerinckx-Rice <me@tobias.gr>
Cc: 59355@debbugs.gnu.org
Subject: [bug#59355] [PATCH] gnu: Add dataparksearch.
Date: Sat, 18 Feb 2023 19:37:27 +0800	[thread overview]
Message-ID: <036888fe-5347-755f-1ae3-121197d828c2@disroot.org> (raw)
In-Reply-To: <87y1ovbvfd.fsf@nckx>

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





  reply	other threads:[~2023-02-18 11:38 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-18 11:17 [bug#59355] [PATCH] gnu: Add dataparksearch Adam Faiz via Guix-patches via
2023-02-18  0:13 ` Tobias Geerinckx-Rice via Guix-patches via
2023-02-18 11:37   ` Adam Faiz via Guix-patches via [this message]
2023-02-19  0:29   ` [bug#59355] [PATCH v1] " Adam Faiz via Guix-patches via
2023-04-22 10:03     ` Nicolas Goaziou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=036888fe-5347-755f-1ae3-121197d828c2@disroot.org \
    --to=guix-patches@gnu.org \
    --cc=59355@debbugs.gnu.org \
    --cc=adam.faiz@disroot.org \
    --cc=me@tobias.gr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.