On Mon, 2019-08-26 at 11:03 +0200, Nicolas Goaziou wrote:
> Hello,
> 
> Jesse Gibbons <jgibbons2357@gmail.com> writes:
> 
> > Patch is attached.
> 
> It looks like this patch felt through the cracks. Do not hesitate to
> send a friendly ping after one or two weeks if no one responds.
> 
> > As explained in the comment I had to correct a few problems:
> > 1. Tests always fail, but (in my experience) the final application
> > is
> > stable.
> > 2. rednotebook  uses the GI_TYPELIB_PATH and LD_LIBRARY_PATH to
> > find
> > webkitgtk.
> > - I made gnu/packages/journal.scm to add other similar
> > journal/diary
> >   applications. Because I do not like naming sources for the only
> >   package defined within, especially when the package is simple, I
> >   recommend moving the jrnl package definition to it. Say the word
> > and
> >   I will produce the patch to do this.
> 
> It sounds good.
> 
> > Perhaps we should discuss the idea of organizing packages by
> > function
> > on the devel mailing list?
> 
> Feel free to start a discussion about it.

I had the discussion soon after I sent the patch. Ricardo convinced me
there is at least one good reason for having sources declaring a single
package.

See https://lists.gnu.org/archive/html/guix-devel/2019-07/msg00240.html
 for the explanation and the thread.

I should have sent a new patch when I decided to rename journal.scm to
rednotebook.scm. My mistake.

> 
> >   - I also plan to define a package for other journal applications
> >     (like those listed between rednotebook and jrnl at
> >     <https://www.maketecheasier.com/best-journal-apps-for-linux/>
> > and
> >     any other recommendations).
> 
> Great!
> 
> > Knowing I am likely to err, I expect you (whoever reviews this
> > patch)
> 
> Some comments follow.
> 
> > +    (source
> > +     (origin
> > +       (method url-fetch)
> > +       (uri (string-append
> > +             "https://github.com/jendrikseipp/rednotebook/archive/
> > v"
> > +             version
> > +             ".tar.gz"))
> 
> We do not use Github releases, because they are not stable. You
> should
> use `git-fetch` here:
> 
>        (method git-fetch)
>        (uri (git-reference
>              (url "https://github.com/jendrikseipp/rednotebook.git")
>              (commit (string-append "v" version))))
>        (file-name (git-file-name name version))
> 
> with an appropriate hash.
> 
fixed.
> > +       (sha256
> > +        (base32
> > +         "15n1ziypfj3lzpvhha7r637zrb259l9yrcsvkic9cg5mndiaivs3")))
> > )
> > +    (build-system python-build-system)
> > +    (arguments
> > +     ;;Tests fail to find the "_" function.
> > +     ;;It should be defined in rednotebook/info.py if '_' is not a
> > member of 'builtins'.
> > +     ;;It is either not defined or not exported during the check
> > phase.
> > +     ;;The program does not have this problem after it is
> > installed.
> > +     ;;TODO: Fix tests.
> 
> Nitpick: missing space between ";;" and "Make".
> 
> > +     `(#:tests? #f
> > +       #:imported-modules ((guix build glib-or-gtk-build-system)
> > +                           ,@%python-build-system-modules)
> > +       #:modules ((ice-9 match)
> > +                  (guix build python-build-system)
> > +                  ((guix build glib-or-gtk-build-system) #:prefix
> > glib-or-gtk:)
> > +                  (guix build utils))
> > +       #:phases
> > +       (modify-phases %standard-phases
> > +         ;;Make sure rednotebook can find the typelibs and
> > webkitgtk shared libraries.
> 
> Ditto.
> 
both fixed.
> > +         (add-before 'wrap 'wrap-with-library-paths
> > +           (lambda* (#:key inputs outputs #:allow-other-keys)
> > +             (let ((out (assoc-ref outputs "out"))
> > +                   (gi-typelib-path (getenv "GI_TYPELIB_PATH"))
> > +                   (webkitgtk-path (string-append
> > +                                    (assoc-ref inputs "webkitgtk")
> > +                                    "/lib")))
> > +               (wrap-program (string-append out
> > "/bin/rednotebook")
> > +                 `("GI_TYPELIB_PATH" ":" prefix (,gi-typelib-
> > path))
> > +                 `("LD_LIBRARY_PATH" ":" prefix (,webkitgtk-
> > path)))
> > +               #t))))))
> > +    (inputs
> > +     `(("python" ,python-3)
> 
> I don't think this input is necessary. `python-build-system' should
> provide it already, shouldn't it?
fixed
> 
> > +       ("gtk+"  ,gtk+)
> 
> Nitpick: spurious spaces.
fixed
> > 
> > +       ("webkitgtk" ,webkitgtk)
> > +       ("python-pyyaml" ,python-pyyaml)))
> > +    (home-page "https://www.rednotebook.app")
> > +    (synopsis "journal")
> 
> What about this one, borrowed to Debian:
> 
>   "Daily journal with calendar, templates and keyword searching"
good idea
> 
> > +    (description
> > +     "RedNotebook is a modern desktop journal. It lets you format,
> > tag and search
> > +your entries. You can also add pictures, links and customizable
> > templates, spell
> > +check your notes, and export to plain text, HTML, Latex or PDF.")
> 
> You need two spaces after full stops in Texinfo.
> 
fixed
> Also, would it make sense to package pyenchant and add it as
> a dependency? It's not a blocker though.
Added this in a TODO. I also made a list of packages that should be
added. It looks like there are four python modules in the
rednotebook/external directory that should be packaged externally if
possible.
> 
> Could you send an updated patch?
attached
> 
> Thank you!
> 
> Regards,
> 
-- 
-Jesse