Re: 01/01: gnu: Add nsis-x86_64 and nsis-i686.

[Marius Bakke <mbakke@fastmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7095 bytes --]

Hello Carl,

Some feedback on the patch, since it did not go through the usual
channels :-)

guix-commits@gnu.org writes:

> carl pushed a commit to branch master
> in repository guix.
>
> commit e214a22007450187c3db5d9fdef6ee021be6e953
> Author: Carl Dong <contact@carldong.me>
> Date:   Wed Sep 11 15:43:19 2019 -0400
>
>     gnu: Add nsis-x86_64 and nsis-i686.
>     
>     * guix/build-system/scons.scm (scons-build): Add build-targets and
>     install-targets parameters.
>     * guix/build/scons-build-system.scm (build, install): Adjust
>     accordingly.
>     * doc/guix.texi (Build Systems): Document it.

I would prefer if this was a separate commit, since it is unrelated to
NSIS, even though it is a prerequisite.  If the NSIS patch had to be
reverted for some reason, we would still have this.

(there is nothing we can do about that now, of course, but its's
something to consider in the future)

>     * gnu/packages/installers.scm: New file,
>     (make-nsis): New procedure,
>     (nsis-x86_64, nsis-i686): New variables.
>     * gnu/packages/patches/nsis-env-passthru.patch: New file.
>     * gnu/local.mk (dist_patch_DATA, GNU_SYSTEM_MODULES): Adjust
>     accordingly.

[...]

> +(define(make-nsis machine target-arch nsis-target-type)
> +  (let ((triplet (string-append machine "-" "w64-mingw32")))
> +    (package
> +      (name (string-append "nsis-" machine))
> +      (version "3.04")
> +      (source (origin
> +                (method url-fetch)
> +                (uri (string-append "http://prdownloads.sourceforge.net/nsis/nsis-"
> +                                    version "-src.tar.bz2"))
> +                (sha256
> +                 (base32
> +                  "1xgllk2mk36ll2509hd31mfq6blgncmdzmwxj3ymrwshdh23d5b0"))
> +                (patches (search-patches "nsis-env-passthru.patch"))))
> +      (build-system scons-build-system)
> +      (native-inputs `(("xgcc" ,(cross-gcc triplet #:libc (cross-libc triplet)))
> +                       ("xbinutils" ,(cross-binutils triplet))
> +                       ("mingw-w64" ,(cross-libc triplet))))

Nit-pick: we usually don't indent like this.

> +      (inputs `(("zlib" ,zlib)))
> +      (arguments
> +       `(#:scons ,scons-python2
> +         #:modules ((srfi srfi-1)
> +                    (guix build utils)
> +                    (guix build scons-build-system))
> +         #:tests? #f

Could you leave a comment explaining why tests are disabled?

> +         #:scons-flags `("UNICODE=yes"
> +                         "SKIPUTILS=MakeLangId,Makensisw,NSIS Menu,SubStart,zip2exe"
> +                         "SKIPDOC=COPYING"
> +                         "STRIP_CP=no"
> +                         ,(string-append "PREFIX=" %output)
> +                         ,(string-append "TARGET_ARCH=" ,target-arch)
> +                         ,(string-append "XGCC_W32_PREFIX=" ,triplet "-")
> +                         ,(string-append "PREFIX_PLUGINAPI_INC=" (assoc-ref %build-inputs "mingw-w64") "/include/")
> +                         ,(string-append "PREFIX_PLUGINAPI_LIB=" (assoc-ref %build-inputs "mingw-w64") "/lib/"))
> +         #:build-targets '("makensis"
> +                           "stubs"
> +                           "plugins"
> +                           "utils")
> +         #:install-targets '("install-stubs"
> +                             "install-plugins"
> +                             "install-data"
> +                             "install-utils"
> +                             "install-compiler"
> +                             "install-conf")
> +         #:phases (modify-phases %standard-phases
> +                    (add-before 'build 'fix-env
> +                      (lambda _
> +                        (define* (filter-delimited-string delimited-string predicate #:optional (delimiter #\:))
> +                          ;; Given a DELIMITED-STRING delimited by DELIMITER,
> +                          ;; only keep items that satisfy PREDICATE

Please use punctuations in code comments for consistency.

> +                          (string-join
> +                           (filter predicate (string-split delimited-string delimiter))
> +                           (string delimiter)))
> +                        (define (mingw-path? path)
> +                          (string-prefix? (assoc-ref %build-inputs "mingw-w64") path))
> +                        (for-each
> +                         (lambda (env-name)
> +                           (let ((env-val (getenv env-name)))
> +                             ;; Remove all mingw-w64 paths from env vars meant
> +                             ;; for native toolchain
> +                             (setenv env-name
> +                                     (filter-delimited-string env-val (negate mingw-path?)))
> +                             ;; Add the removed paths back into
> +                             ;; CROSS_-prefixed version of env vars
> +                             (setenv (string-append "CROSS_" env-name)
> +                                     (filter-delimited-string env-val mingw-path?))))
> +                         '("CPLUS_INCLUDE_PATH" "LIBRARY_PATH" "C_INCLUDE_PATH"))))
> +                    (add-before 'build 'fix-target-detection
> +                      (lambda _
> +                        ;; NSIS target detection is screwed up, manually
> +                        ;; change it ourselves
> +                        (substitute* "Source/build.cpp" (("m_target_type=TARGET_X86ANSI")
> +                                                         (string-append "m_target_type=" ,nsis-target-type))))))))

substitute* and setenv have unspecified return values, so these phases
should be modified to end on #t.

> +      (home-page "http://nsis.sourceforge.net/")
> +      (synopsis "A professional open source system to create Windows installers")

Everything in Guix is free software, so there is no need to mention
"open source" (which is a term we also avoid using, but that's another
issue).  Synopses also should not start with an article: `guix lint`
usually warns about that.

Maybe it can be shortened to "Create Windows Installers"?

> +      (description
> +       "NSIS (Nullsoft Scriptable Install System) is a professional open
> +source system to create Windows installers. It is designed to be as small and
> +flexible as possible and is therefore very suitable for internet
> +distribution.")

"open source" is redundant here too.  Also, use two spaces after
punctuations to avoid another warning from `guix lint`.

> +      (license (license:non-copyleft "file://COPYING"
> +                                     "See COPYING in the distribution.")))))

According to COPYING, NSIS is largely the zlib license, with some
portions under bzip2 or CPL 1.0.  So I think it would be better to have
a list of those, with an explaining comment mentioning that COPYING has
the gory details.

Could you send an updated patch?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]