Subject: Re: [bug#55874] [PATCH] Add timewarrior 1.4.3

From: Zac Berkowitz <[email-masked]>
Date: Fri, 10 Jun 2022 10:04:02 -0400

Maxime,

Ended up having a bit of time today to look through what you brought
up.  Thanks again for it!

>> +    (native-inputs
>> +     `(("ruby-asciidoctor" ,ruby-asciidoctor)
>> +       ("python" ,python)))
>
> According to Debian, additional (native-)inputs are required:
>
> https://salsa.debian.org/tasktools-team/timew/-/blob/master/debian/tests/control
>
> Are the tests actually run and can the man page be read?

According to the source
(https://github.com/GothenburgBitFactory/timewarrior/blob/develop/INSTALL)
asciidoctor is required to build the docs, but python is not a
requirement at runtime.
I'm able to run the timew executable in a pure shell without moving
python to the input list, so this seems to be a debian issue?

The python scripts in ext/ are completely optional according to the
README in that same directory.  Like taskwarrior, timewarrior can
execute python scripts as hooks in response to various commands, but
they're not necessary for base use.
Maybe this is why debian adds the python requirement?  So hooks just
work out of the box?  How does guix deal with optional dependencies
like this?

Tests are run and pass, but that doesn't rule out determinism issues,
I'll see what debian did to address that.

man pages are not available, I'll correct that.

>> +    (native-inputs
>> +     `(("ruby-asciidoctor" ,ruby-asciidoctor)
>> +       ("python" ,python)))
>> +    (inputs
>> +     `(("gnutls" ,gnutls)
>> +       ("util-linux:lib" ,util-linux "lib")))
>
> Nowadays, these can be simplified to.
>
>  (native-inputs (list ruby-asciidoctor python))
>
> Where did you learn about the old form?

Wouldn't say "learn" - I'm very new to lisps! The code here is mostly
adapted from taskwarrior (same developer) and other snippets from
gnu/packages.  I'm sure I grep'd for another package that required
asciidoctor and adapted their portion of the code.

>>+         (delete 'install-license-files) ; Already installed by
>> package
>
> Not a problem I think -- at worst, the license file will appear twice,
> but identical files are automatically deduplicated, and even if not, they
> are relatively small files anyway.

This is from taskwarrior from the same developer and has a similar
build setup.  Seems like both packages should be consistent, but I'm
happy to remove this directive if needed.

>> +             (substitute* "src/commands/CMakeLists.txt"
>> +               (("/bin/sh") (which "sh"))))))))
>
> In this context, (which "sh") can maybe be simplified to just "sh", not
> that it really matters.

Ok.

On Fri, Jun 10, 2022 at 3:49 AM Maxime Devos <[email-masked]> wrote:
>
> Zac Berkowitz schreef op do 09-06-2022 om 21:36 [-0400]:
> > Maxime,
> >
> > Thanks for all of the great feedback -- seems like "works for me"
> > wasn't nearly good enough in this case!  I'll chip away at these
> > issues next week when I've got some time.  Is submitting a new patch
> > on this bug # the best way forward?  Or should I close this and open a
> > new patch issue when it's ready?
>
> Send it as a v2 to 55874@, then all the relevant information is kept
> together at <https://issues.guix.gnu.org/55874>.
>
> Greetings,
> Maxime.