bug#30351: [PATCH] gnu: mcron2: Correct and enhance the wrap phase.

[Maxim Cournoyer]

Hello!

address@hidden (Ludovic Courtès) writes:

> Hi Maxim,
>
> Maxim Cournoyer <address@hidden> skribis:
>
>> As a follow up to my previous patch to mcron2, this completes the wrap
>> phases and fix a small issue where the mcron modules were installed to
>> share/guile/site/2.0 instead of 2.2.
>
> Good catch.  I’ve committed that change separately.
>
> BTW, I encourage you to submit changes to address@hidden  Mcron is
> now maintained by Mathieu Lirzin, who is familiar with current Guile and
> certainly interested in receiving improvements like this.
>
> (Another thing that should be done would be install .go files in
> $libdir/guile/2.2/site-ccache.)

It's already all fixed upstream! I verified myself. I guess we could
poke Mathieu for a new release ;).

>> For those using dyndns services, a job like the following should now
>> work without having to propagate Guile or GnuTLS in your user/system
>> profile[0]:
>
> [...]
>
>>>From 385343b1370d87e6104ebe2ef473bf2d1e31f2f2 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <address@hidden>
>> Date: Sun, 4 Feb 2018 14:05:40 -0500
>> Subject: [PATCH] gnu: mcron2: Correct and enhance the wrap phase.
>>
>> * gnu/packages/guile.scm (mcron2)[inputs]: Rename "guile-2.2" to just 
>> "guile".
>> [phases]: Install mcron2 modules to guile/site/2.2 instead of guile/site/2.0.
>> Add Guile 2.2 and GnuTLS modules to the wrap phase.
>
> [...]
>
>>             (add-after 'install 'wrap-mcron
>> -             (lambda* (#:key outputs #:allow-other-keys)
>> -               ;; Wrap the 'mcron' command to refer to the right
>> -               ;; modules.
>> -               (let* ((out  (assoc-ref outputs "out"))
>> -                      (bin  (string-append out "/bin"))
>> -                      (site (string-append
>> -                             out "/share/guile/site")))
>> -                 (match (scandir site)
>> -                   (("." ".." version)
>> -                    (let ((modules (string-append site "/" version)))
>> -                      (wrap-program (string-append bin "/mcron")
>> -                        `("GUILE_LOAD_PATH" ":" prefix
>> -                          (,modules))
>> -                        `("GUILE_LOAD_COMPILED_PATH" ":" prefix
>> -                          (,modules)))
>> -                      #t))))))))))))
>> +             (lambda* (#:key inputs outputs #:allow-other-keys)
>> +               ;; Wrap the 'mcron' command to refer to the right modules. We
>> +               ;; also include Guile 2.2 modules and GnuTLS, so that Guile
>> +               ;; libraries can be used in mcron jobs without having to
>> +               ;; propagate those in a user profile.
>> +               (let* ((site-dir "/share/guile/site/2.2")
>> +                      (ccache-dir "/lib/guile/2.2/ccache")
>> +                      (mcron  (assoc-ref outputs "out"))
>> +                      (mcron-bin (string-append mcron "/bin/mcron"))
>> +                      (mcron-modules (string-append mcron site-dir))
>> +                      (guile (assoc-ref inputs "guile"))
>> +                      (guile-modules (string-append guile site-dir))
>> +                      (guile-ccache (string-append guile ccache-dir))
>> +                      (gnutls (assoc-ref inputs "gnutls"))
>> +                      (gnutls-modules (string-append gnutls site-dir))
>> +                      (gnutls-ccache (string-append gnutls ccache-dir)))
>> +                 (wrap-program mcron-bin
>> +                   `("GUILE_LOAD_PATH" ":" prefix
>> +                     (,mcron-modules ,guile-modules ,gnutls-modules))
>> +                   `("GUILE_LOAD_COMPILED_PATH" ":" prefix
>> +                     (,mcron-modules ,guile-ccache ,gnutls-ccache)))
>> +                 #t)))))))))
>
> A couple of issues here: the ‘scandir’ trick above allowed us to not
> hard-code “2.2”.  I think it’d be nice to try to preserve such things;
> it’ll be less pain down the road.
>
> Second issue is about adding GnuTLS to the search path: it’s not the
> right place for that.  What if someone wants Guile-JSON?  And Guile-Git?
> And…  You get the idea.  :-)
>
> Instead I’d suggest writing your mcron job along these lines:
>
>   #~(begin
>       (add-to-load-path (string-append #+gnutls "/share/guile/site/"
>                                        (effective-version)))
>       (use-modules (web client))
>       …)
>
> (See (guix download) for an example of this hack.)

Thanks for sharing this idea; it makes sense. I hadn't thought about
manipulating the load-path at run time.

> The extra boilerplate is admittedly not great, so I’d like to add a
> ‘with-extensions’ or ‘with-imported-packages’ form that would be like
> ’with-imported-modules’ but for Guile “extensions” like GnuTLS.

I don't think it's that bad as it is. Any enhancements welcome, of
course :).

> Last thing: it’s not necessary to put Guile’s own module directories in
> the search path.  They’re already there by default.

Tried, and indeed it works without it. Thanks. I must have gotten
confused somewhere.

> So overall I think I’m arguing for the status quo.  Would that work for
> you?

I'm happy with status quo as well. Thanks for lending your sharp eyes to
this review!

Maxim