Hello Carl, Some feedback on the patch, since it did not go through the usual channels :-)
guix-comm...@gnu.org writes: > carl pushed a commit to branch master > in repository guix. > > commit e214a22007450187c3db5d9fdef6ee021be6e953 > Author: Carl Dong <cont...@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?
signature.asc
Description: PGP signature
