On Wed, Apr 29, 2020 at 05:05:04PM +0200, Ricardo Wurmus wrote:
> 
> Thank you for this patch.
> 
> > +    (arguments
> > +     `(#:tests? #f                      ; no check target
> > +       ;; XXX: There are RUNPATH issues such as
> > +       ;; $prefix/lib/icecat-31.6.0/plugin-container NEEDing libmozalloc.so,
> > +       ;; which is not in its RUNPATH, but they appear to be harmless in
> > +       ;; practice somehow.  See <http://hydra.gnu.org/build/378133>.
> > +       #:validate-runpath? #f
> 
> The comment seems to have been copied from somewhere.  I don’t think
> this package produces $prefix/lib/icecat-31.6.0/plugin-container
> 
> The URL is also no longer reachable.
> 
> > +         (add-after 'unpack 'remove-bundled-libraries
> > +           (lambda _
> > +             ;; Remove bundled libraries that we don't use.
> > +             (for-each (lambda (file)
> > +                         (format #t "deleting '~a'...~%" file)
> > +                         (delete-file-recursively file))
> > +                       '(;; UNBUNDLE-ME! icu
> > +                         ;; UNBUNDLE-ME! nss
> > +                         "modules/zlib"))
> > +             #t))
> 
> This should be done in a snippet.
> 
> > +         (add-after 'patch-source-shebangs 'patch-cargo-checksums
> > +           (lambda _
> > +             (use-modules (guix build cargo-utils))
> > +             (let ((null-hash
> > "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"))
> 
> What is this null-hash?
> 

This is the "null-hash" that is also used in (gnu packages gnuzilla)

> > +               (substitute* '("Cargo.lock" "gfx/wr/Cargo.lock")
> > +                 (("(\"checksum .* = )\".*\"" all name)
> > +                  (string-append name "\"" null-hash "\"")))
> > +               (for-each
> > +                (lambda (filename)
> > +                  (delete-file filename)
> > +                  (let ((dir (dirname filename)))
> > +                    (display (string-append
> > +                              "patch-cargo-checksums: generate-checksums for "
> > +                              dir "\n"))
> > +                    (generate-checksums dir)))
> > +                (find-files "third_party/rust" ".cargo-checksum.json")))
> > +             #t))
> 
> Oh, there are a lot of things in third_party/rust…  Can this be unbundled?
> 

We don't yet unbundle the crates used in icecat so I wouldn't consider
it a blocker yet. On the other hand we do have a function for this in
(guix build cargo-utils) which should be used.

> > +         ; Fixes issue where each installation directory generates its own profile.
> > +         ; See e.g. https://trac.torproject.org/projects/tor/ticket/31457
> > +         (add-after 'patch-source-shebangs 'fix-profile-setting
> > +           (lambda _
> > +             (let ((mozconfigure "comm/mail/moz.configure"))
> > +               (substitute* mozconfigure
> > +                 (("'MOZ_DEDICATED_PROFILES', True")
> > +                  "'MOZ_DEDICATED_PROFILES', False")))))
> 
> This phase needs to end on #t.
> 
> > +         (replace 'configure
> > +           (lambda* (#:key outputs configure-flags #:allow-other-keys)
> > +             (let* ((out (assoc-ref outputs "out"))
> > +                    (bash (which "bash"))
> > +                    (abs-srcdir (getcwd))
> > +                    (srcdir (string-append "../" (basename abs-srcdir)))
> > +                    (flags `(,(string-append "--prefix=" out)
> > +                             ,@configure-flags))
> > +                    (mozconfig (string-append (getcwd) "/.mozconfig")))
> > +               (setenv "SHELL" bash)
> > +               (setenv "AUTOCONF"
> > +                       (string-append (assoc-ref %build-inputs
> > +                                                 "autoconf")
> > +                                      "/bin/autoconf"))
> > +               (setenv "CONFIG_SHELL" bash)
> > +               (setenv "QA_CONFIGURE_OPTIONS" ".*")
> > +               (setenv "MOZBUILD_STATE_PATH"
> > +                       (string-append (getcwd) "/mach_state"))
> > +               (setenv "MOZCONFIG"
> > +                       (string-append (getcwd) "/.mozconfig"))
> > +               (setenv "CC" "gcc")
> > +               (setenv "MOZ_NOSPAM" "1")
> > +               (setenv "PYTHON"
> > +                       (string-append (assoc-ref %build-inputs
> > +                                                 "python2")
> > +                                      "/bin/python"))
> 
> Looks like Python 3 could be used instead.  Have you tried that?
> 
> > +               (delete-file-recursively "obj-x86_64-pc-linux-gnu")
> 
> Can this be deleted in a snippet or is this generated in some earlier
> phase?  Is the same directory generated on other architectures?
> 

I noticed this too. It should probably be generated per-architecture.

> > +               (zero? (system* "./mach" "configure")))))
> 
> Please use (invoke …) instead of (zero? (system* …)).
> 
> > +         (replace 'build
> > +           (lambda _
> > +             (zero? (system* "./mach" "build"))))
> 
> Same here.
> 
> > +         (replace 'install
> > +           (lambda _
> > +             (zero? (system* "./mach" "install"))))
> 
> …and here.
> 
> > +         (add-after 'install 'wrap-program
> > +           (lambda* (#:key inputs outputs #:allow-other-keys)
> > +             (let* ((out (assoc-ref outputs "out"))
> > +                    (lib (string-append out "/lib"))
> > +                    (gtk (assoc-ref inputs "gtk+"))
> > +                    (gtk-share (string-append gtk "/share"))
> > +                    (pulseaudio (assoc-ref inputs "pulseaudio"))
> > +                    (pulseaudio-lib (string-append pulseaudio "/lib")))
> > +               (wrap-program (car (find-files lib "^thunderbird$"))
> > +                 `("XDG_DATA_DIRS" prefix (,gtk-share))
> > +                 `("LD_LIBRARY_PATH" prefix (,pulseaudio-lib)))
> > +               #t))))))
> 
> Why does thunderbird need wrapping with pulseaudio?
> 
> > +       ("python" ,python)
> > +       ("python2" ,python-2.7)
> 
> See above.  We may be able to use Python 3 here.
> 
> > +    (home-page "https://www.thunderbird.net")
> > +    (synopsis "Trademarkless version of Mozilla Thunderbird")
> 
> Is the use of the name “Thunderbird” a violation of Mozilla’s trademark
> policies?
> 
> --
> Ricardo
> 
> 
> 

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted