Re: Graft hooks

[Timothy Sample]

Hi Ludo,

address@hidden (Ludovic Courtès) writes:

> Hello Timothy,
>
> Timothy Sample <address@hidden> skribis:
>
>> address@hidden (Ludovic Courtès) writes:
>>
>>> Hello Timothy,
>>>
>>> Timothy Sample <address@hidden> skribis:
>>>
>>>> The basic idea would be to add a field (or use a property) to the
>>>> package record.  Let’s call it “graft-hook”.  It would be Scheme code
>>>> that gets run after grafting takes place, giving us a chance to patch
>>>> special things like checksums.  The hook would be passed the list of
>>>> files that were been modified during grafting.  Then, in the Racket
>>>> package for example, I could write a graft-hook that updates the SHA-1
>>>> hash of each of the modified source files.
>>>>
>>>> Since grafting is done at the derivation level, the hook code would have
>>>> to be propagated down from the package level.  I haven’t looked at all
>>>> the details yet, because maybe this is a bad idea and I shouldn’t waste
>>>> my time!  :)  My first impression is that it is not too tricky.
>>>>
>>>> Are these problems too specialized to deserve a general mechanism like
>>>> this?  Let me know what you think!
>>>
>>> I agree that this would be the right thing to do!  (I’d really like to
>>> do it for GDB as discussed in <https://bugs.gnu.org/19973>.)
>>>
>>> Package properties would be the right way to make it extensible, but
>>> there are complications (notably we’d need to use gexps, but build
>>> systems don’t use gexps yet.)
>>
>> But soon, right?  ;)
>
> Well, it’s complicated.  :-)
>
> Also, I realized that some things, like the .gnu_debuglink and build-id
> hooks, don’t really fit in any package; they’re transverse.

You’re right.  Packages are the wrong place.  What about build systems?
Maybe it makes sense (theoretically) to define graft hooks in build
systems, and then modify them (if necessary) using the “arguments” field
of a package.  Isn’t it the GNU or Go build systems that are ultimately
responsible for these checksums?  Shouldn’t it be their job to tell the
grafting code how to fix them?

>> Here’s a draft patch (it’s mercifully small).  I have a few questions
>> about it, but if it looks like the right approach, I will clean it up
>> and submit it.
>>
>> Basically, it checks if we are grafting Racket, and then adds some code
>> to the build expression to run the hook.
>
> OK.  In theory, should it be just for Racket, or should it also be for
> Racket libraries (we don’t have any currently AFAIK)?

I could imagine attaching the hook to our (theoretical) Racket build
system.  ;)  In the meantime, we can check if “share/racket” exists in
the output and run the code then.  Of course, if it were a Racket
library (and not Racket itself), I wouldn’t be able to use “raco setup”,
because I wouldn’t be able to find it.

>> Also, is there a preference for patching the files using Guile or using
>> an external tool?  This patch uses Racket’s “raco setup” command to
>> recompile the files and fix the checksums.  Unfortunately, it also
>> updates timestamps.  I’m pretty sure our Racket package is not
>> reproducible at the moment, so I didn’t worry about it too much.  The
>> timestamps could be patched out, though.  The reason I shied away from
>> writing my own code is that Racket also hashes all the dependencies for
>> a bytecode file.  This means that the custom code would have to traverse
>> the Racket dependency graph to get the checksums right.  It is not too
>> hard to do so, but it would be a couple hundred lines of code (compared
>> to the five or so it took to invoke “raco setup”).
>
> Regarding whether or not to write our own code: let’s do whichever is
> more convenient.  In this case, using ‘raco setup’ looks like the right
> thing to do, given that raco is available in the build environment
> anyway (see below); for .gnu_debuglink, I found it nicer (and more fun
> :-)) to write a Guile module.
>
> Regarding timestamps: I guess there’s no problem since timestamps are
> reset in the store.

Whoops!  I didn’t mean the file metadata, I meant in the bytecode files
themselves.  Also, I only saw the changes in diffoscope and assumed they
were timestamps.  I looked more closely and realized they were more
checksums that I didn’t notice before (it should have been obvious
because they are 20 bytes...).  They are supposed to be updated.
Nothing to see here.  :)

> Some comments:
>
>> diff --git a/guix/grafts.scm b/guix/grafts.scm
>> index d6b0e93e8..88a99312d 100644
>> --- a/guix/grafts.scm
>> +++ b/guix/grafts.scm
>> @@ -75,6 +75,36 @@
>>      (($ <graft> (? string? item))
>>       item)))
>>  
>> +(define (fix-racket-checksums store drv system)
>> +  (define racket-drv
>> +    (let ((package-derivation (module-ref (resolve-interface '(guix 
>> packages))
>> +                                          'package-derivation))
>> +          (racket (module-ref (resolve-interface '(gnu packages scheme))
>> +                              'racket)))
>> +      (package-derivation store racket system #:graft? #f)))
>> +
>> +  (define hook-exp
>> +    `(lambda (input output mapping)
>> +       (let ((raco (string-append output "/bin/raco")))
>> +         ;; Setting PLT_COMPILED_FILE_CHECK to "exists" tells Racket to
>> +         ;; ignore timestamps when checking if a compiled file is valid.
>> +         ;; Without it, Racket attempts a complete rebuild of
>> +         ;; everything.
>> +         (setenv "PLT_COMPILED_FILE_CHECK" "exists")
>> +         ;; All of the --no-* flags below keep Racket from making
>> +         ;; unecessary and unhelpful changes (like rewriting scripts and
>> +         ;; reverting their shebangs in the process).
>> +         (invoke raco "setup" "--no-launcher" "--no-install"
>> +                 "--no-post-install" "--no-info-domain" "--no-docs"))))
>
> Since this is used when grafting Racket, I would suggest moving this
> graft to the “build side” entirely, similar to what I did in
> <https://debbugs.gnu.org/cgi/bugreport.cgi?bug=19973#25>.  Probably
> you’d just add a single procedure to (guix build graft) and add it to
> %graft-hooks.
>
> That procedure could be the same as what you have above, except that
> it’d run OUT/bin/raco, if it exists, and do nothing if OUT/bin/raco does
> not exist.
>
> WDYT?

This makes sense.

Following my note above, I think I will try and finish my update code in
Guile, and then use the existence of “share/racket” as the heuristic to
determine if it should run.

Maybe I will package a Racket library, too, so I can test it.

> Thanks,
> Ludo’.

Thanks for the feedback!


-- Tim