Package: guix-patches;
Reported by: Nicolas Graves <ngraves <at> ngraves.fr>
Date: Thu, 5 Jan 2023 10:04:01 UTC
Severity: normal
Tags: moreinfo
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Nicolas Graves <ngraves <at> ngraves.fr> Cc: 60571 <at> debbugs.gnu.org Subject: [bug#60571] [PATCH v2 3/4] gnu: Add icu4c-for-skia. Date: Mon, 28 Aug 2023 11:48:14 -0400
Hello!
Nicolas Graves <ngraves <at> ngraves.fr> writes:
> * gnu/packages/icu4c.scm (icu4c-for-skia): New variable.
> ---
> gnu/packages/icu4c.scm | 101 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 101 insertions(+)
>
> diff --git a/gnu/packages/icu4c.scm b/gnu/packages/icu4c.scm
> index ba8b4915f2..eef8bd2cd3 100644
> --- a/gnu/packages/icu4c.scm
> +++ b/gnu/packages/icu4c.scm
> @@ -27,14 +27,17 @@
>
> (define-module (gnu packages icu4c)
> #:use-module (gnu packages)
> + #:use-module (gnu packages cpio)
> #:use-module (gnu packages java)
> #:use-module (gnu packages perl)
> + #:use-module (gnu packages pkg-config)
> #:use-module (gnu packages python)
> #:use-module (guix gexp)
> #:use-module (guix licenses)
> #:use-module (guix packages)
> #:use-module (guix utils)
> #:use-module (guix download)
> + #:use-module (guix git-download)
> #:use-module (guix build-system ant)
> #:use-module (guix build-system gnu))
>
> @@ -240,3 +243,101 @@ (define-public java-icu4j
> globalisation support for software applications. This package contains the
> Java part.")
> (license x11)))
> +
> +(define-public icu4c-for-skia
> + ;; The current version of skia needs commit-precise icu4c
> + ;; version for its test-dependencies.
I'd reword to use "needs this exact commit". Also,
s/test-dependencies/test dependencies/ (no hyphen).
> + (let ((commit "a0718d4f121727e30b8d52c7a189ebf5ab52421f")
> + (revision "0"))
> + (package
> + (inherit icu4c)
> + (name "icu4c-for-skia")
> + (version "skia")
> + (source
> + (origin
> + (method git-fetch)
> + (uri (git-reference
> + (url "https://chromium.googlesource.com/chromium/deps/icu.git")
> + (commit commit)))
> + (file-name (git-file-name name version))
> + (sha256
> + (base32 "1qxws2p91f6dmhy7d3967r5ygz06r88pkmpm97px067x0zzdz384"))))
> + (native-inputs (list python cpio pkg-config))
nitpick: Conventionally, it's more common to put the inputs after the
arguments fields.
> + (arguments
> + (list
> + #:make-flags #~`(,(string-append "DESTDIR=" #$output))
It'd be more readable to use the simpler #~(list (string-append ...)).
> + #:configure-flags #~'(;;"--enable-shared=no" "--enable-static=yes"
> + "--prefix=" "--exec-prefix=")
Please drop the dead code (;; ...).
> + #:phases
> + #~(modify-phases %standard-phases
> + (add-after 'unpack 'chdir-to-source
> + (lambda _ (chdir "source")))
> + (replace 'configure
> + (lambda* (#:key inputs parallel-build? configure-flags #:allow-other-keys)
Too long of a line; you can but #:allow-other-keys on its own line.
> + (let ((bash (search-input-file inputs "/bin/sh")))
> + ;; Replace bash executable.
> + (setenv "CONFIG_SHELL" bash)
> + (substitute* "./configure"
> + (("`/bin/sh")
> + (string-append "`" bash)))
Is just setting CONFIG_SHELL not enough? Since '/bin/sh' here is used
strictly at build time (in the configure script), you can use simply
(which "sh") in your substitution. Otherwise you'd have to
(search-input-file (or native-inputs inputs) ...) to not break
cross-compilation.
> + ;; Make the static library position-independent.
> + ;; (substitute* "./icudefs.mk.in"
> + ;; (("^CFLAGS = ")
> + ;; "CFLAGS = -fPIC ")
> + ;; (("^CXXFLAGS = ")
> + ;; "CXXFLAGS = -fPIC "))
All commented out? Drop unless needed.
> + ;; Update runpath.
The comments are a bit terse; I'd expound them to complete sentences
with a tad more context.
> + (substitute* "./icudefs.mk.in"
> + (("= -L\\$\\(LIBDIR\\)")
> + (string-append "= -L$(LIBDIR)"
> + " -Wl,-rpath=\"" #$output "/lib\"")))
> + ;; Set prefix and exec-prefix.
> + (substitute* "./runConfigureICU"
> + (("^OPTS=")
> + (string-append "OPTS=\""
> + (string-join configure-flags " ")
> + "\"")))
> + ;; Configure.
> + (invoke "./runConfigureICU" "Linux/gcc"
> + "--disable-layout" "--disable-tests"))))
> + (add-after 'install 'configure-filtered-data
> + (lambda* (#:key make-flags configure-flags #:allow-other-keys)
> + ;; Cleanup.
Ditto.
> + (with-directory-excursion "data"
> + `(,invoke "make" "clean" ,@make-flags))
It'd be cleaner to use 'apply' here, e.g.
(apply invoke "make" "clean" make-flags)
> + ;; Set prefix and exec-prefix.
> + (substitute* "./runConfigureICU"
> + (("^OPTS=")
> + (string-append
> + "OPTS=\"" (string-join configure-flags " ") "\"")))
Mentioning in passage, you could also use (format #f "OPTS=\"~a\""
(string-join ...)), or even the more advanced (ice-9 format).
> + ;; Configure for common data.
> + (setenv "ICU_DATA_FILTER_FILE"
> + (string-append (getcwd) "/../filters/common.json"))
> + (invoke "./runConfigureICU" "Linux/gcc"
> + "--disable-layout" "--disable-tests")))
I'm confused here; we re-run the configure script post installation?
How does that work? If that's really needed it needs a proper
explanatory comment.
> + (add-after 'configure-filtered-data 'build-filtered-data
> + (lambda* (#:key parallel-build? make-flags #:allow-other-keys)
> + (let ((job-count (if parallel-build?
> + (number->string (parallel-job-count))
> + "1")))
> + `(,invoke "make" "-j" ,job-count ,@make-flags)
Please use 'apply', as explained above.
> + (setenv "DESTDIR" #$output)
> + (invoke "bash" "../scripts/copy_data.sh" "common"))))
> + (add-after 'build-filtered-data 'install-scripts-and-data
> + (lambda _
> + (let* ((share (string-append #$output "/share"))
> + (scripts (string-append share "/scripts"))
> + (data (string-append share "/data/common")))
> + ;; Install scripts.
> + (mkdir-p scripts)
> + (copy-recursively "../scripts/" scripts)
> + ;; Install data.
> + (mkdir-p data)
> + (copy-recursively "./dataout/common/data/out/tmp" data)
> + (symlink (string-append data "/icudt69l.dat")
> + (string-append data "/icudtl.dat")))))
> + (add-before 'check 'disable-failing-uconv-test
> + (lambda _
> + (substitute* "extra/uconv/Makefile.in"
> + (("check: check-local")
> + ""))))))))))
The rest looks good! It's seems a tricky package, well done! Could you
please send a v2 with the above suggestions/comments taken into account?
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.