Re: 03/03: build: go-build-system: Ensure uniform unpacking directory.

[Maxim Cournoyer]

Hi again :-)

Mark H Weaver <address@hidden> writes:

> Hello again,
>
> address@hidden writes:
>
>> apteryx pushed a commit to branch master
>> in repository guix.
>>
>> commit f42e4ebb56fe4f16991ca6c6e060c8f3535865cb
>> Author: Maxim Cournoyer <address@hidden>
>> Date:   Fri Apr 5 00:00:08 2019 -0400
>>
>>     build: go-build-system: Ensure uniform unpacking directory.
>>     
>>     Depending on whether the source is a directory or an archive, we strip 
>> the
>>     source directory or preserve it, respectively.  This change makes it so 
>> that
>>     whether the type of the source, it is unpacked at the expected location 
>> given
>>     by the IMPORT-PATH of the Go build system.
>>     
>>     * guix/build/go-build-system.scm: Add the (ice-9 ftw) module.
>>     (unpack): Add inner procedure to maybe strip the top level directory of 
>> an
>>     archive, document it and use it.
>> ---
>>  guix/build/go-build-system.scm | 36 +++++++++++++++++++++++++++---------
>>  1 file changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/guix/build/go-build-system.scm b/guix/build/go-build-system.scm
>> index 92a5c86..d106e70 100644
>> --- a/guix/build/go-build-system.scm
>> +++ b/guix/build/go-build-system.scm
>> @@ -1,6 +1,7 @@
>>  ;;; GNU Guix --- Functional package management for GNU
>>  ;;; Copyright © 2016 Petter <address@hidden>
>>  ;;; Copyright © 2017, 2019 Leo Famulari <address@hidden>
>> +;;; Copyright © 2019 Maxim Cournoyer <address@hidden>
>>  ;;;
>>  ;;; This file is part of GNU Guix.
>>  ;;;
>> @@ -22,6 +23,7 @@
>>    #:use-module (guix build union)
>>    #:use-module (guix build utils)
>>    #:use-module (ice-9 match)
>> +  #:use-module (ice-9 ftw)
>>    #:use-module (srfi srfi-1)
>>    #:use-module (rnrs io ports)
>>    #:use-module (rnrs bytevectors)
>> @@ -151,13 +153,31 @@ dependencies, so it should be self-contained."
>>    #t)
>>  
>>  (define* (unpack #:key source import-path unpack-path #:allow-other-keys)
>> -  "Relative to $GOPATH, unpack SOURCE in the UNPACK-PATH, or the 
>> IMPORT-PATH is
>> -the UNPACK-PATH is unset.  When SOURCE is a directory, copy it instead of
>> +  "Relative to $GOPATH, unpack SOURCE in UNPACK-PATH, or IMPORT-PATH when
>> +UNPACK-PATH is unset.  If the SOURCE archive has a single top level 
>> directory,
>> +it is stripped so that the sources appear directly under UNPACK-PATH.  When
>> +SOURCE is a directory, copy its content into UNPACK-PATH instead of
>>  unpacking."
>> -  (if (string-null? import-path)
>> -      ((display "WARNING: The Go import path is unset.\n")))
>> -  (if (string-null? unpack-path)
>> -      (set! unpack-path import-path))
>
> I see this should have been in the earlier commit, but okay.
> However, I have some other comments:
>
>> +  (define (unpack-maybe-strip source dest)
>> +    (let* ((scratch-dir (string-append (or (getenv "TMPDIR") "/tmp")
>> +                                       "/scratch-dir"))
>> +           (out (mkdir-p scratch-dir)))
>> +      (with-directory-excursion scratch-dir
>> +        (if (string-suffix? ".zip" source)
>> +            (invoke "unzip" source)
>> +            (invoke "tar" "-xvf" source))
>> +        (let ((top-level-files (remove (lambda (x)
>> +                                         (member x '("." "..")))
>> +                                       (scandir "."))))
>> +          (match top-level-files
>> +            ((top-level-file)
>> +             (when (file-is-directory? top-level-file)
>> +               (copy-recursively top-level-file dest #:keep-mtime? #t)))
>> +            (_
>> +             (copy-recursively "." dest #:keep-mtime? #t)))
>> +          #t))
>> +      (delete-file-recursively scratch-dir)))
>> +
>>    (when (string-null? import-path)
>>      ((display "WARNING: The Go import path is unset.\n")))
>>    (when (string-null? unpack-path)
>> @@ -168,9 +188,7 @@ unpacking."
>>          (begin
>>            (copy-recursively source dest #:keep-mtime? #t)
>>            #t)
>> -        (if (string-suffix? ".zip" source)
>> -            (invoke "unzip" "-d" dest source)
>> -            (invoke "tar" "-C" dest "-xvf" source)))))
>> +        (unpack-maybe-strip source dest))))
>
> Please make sure to return #t from this phase procedure.
> It did so before these changes.
>
> You might as well remove the #t from 'unpack-maybe-strip', which will be
> ignored anyway.

Another sharp observation! I've pushed a321312e3a which implements these 
corrections.

Thank you!

Maxim