From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
To: Nicolas Graves <ngraves@ngraves.fr>
Cc: 60571@debbugs.gnu.org
Subject: [bug#60571] [PATCH v2 3/4] gnu: Add icu4c-for-skia.
Date: Mon, 28 Aug 2023 11:48:14 -0400 [thread overview]
Message-ID: <874jkjgoch.fsf@gmail.com> (raw)
In-Reply-To: <28d93ea0e4bda4dbbc9a653cfefb7e569b827ada.1693140822.git.ngraves@ngraves.fr> (Nicolas Graves's message of "Sun, 27 Aug 2023 14:53:41 +0200")
Hello!
Nicolas Graves <ngraves@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
next prev parent reply other threads:[~2023-08-28 15:49 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-05 10:02 [bug#60571] activating tests for skia 2D graphics library Nicolas Graves via Guix-patches via
2023-01-05 12:18 ` [bug#60571] [PATCH 1/4] gnu: Add spirv-headers-for-skia Nicolas Graves via Guix-patches via
2023-01-05 12:18 ` [bug#60571] [PATCH 2/4] gnu: Add spirv-tools-for-skia Nicolas Graves via Guix-patches via
2023-03-24 3:20 ` [bug#60571] activating tests for skia 2D graphics library Maxim Cournoyer
2023-01-05 12:18 ` [bug#60571] [PATCH 3/4] gnu: Add icu4c-for-skia Nicolas Graves via Guix-patches via
2023-01-05 12:18 ` [bug#60571] [PATCH 4/4] gnu: skia: Activate tests Nicolas Graves via Guix-patches via
2023-03-24 3:15 ` [bug#60571] activating tests for skia 2D graphics library Maxim Cournoyer
2023-08-27 12:53 ` [bug#60571] [PATCH v2 1/4] gnu: Add spirv-headers-for-skia Nicolas Graves via Guix-patches via
2023-08-27 12:53 ` [bug#60571] [PATCH v2 2/4] gnu: Add spirv-tools-for-skia Nicolas Graves via Guix-patches via
2023-08-28 15:29 ` Maxim Cournoyer
2023-08-27 12:53 ` [bug#60571] [PATCH v2 3/4] gnu: Add icu4c-for-skia Nicolas Graves via Guix-patches via
2023-08-28 15:48 ` Maxim Cournoyer [this message]
2023-08-29 13:37 ` Nicolas Graves via Guix-patches via
2023-08-29 14:46 ` Maxim Cournoyer
2023-08-27 12:53 ` [bug#60571] [PATCH v2 4/4] gnu: skia: Activate tests Nicolas Graves via Guix-patches via
2023-08-28 15:58 ` Maxim Cournoyer
2023-08-28 15:27 ` [bug#60571] [PATCH v2 1/4] gnu: Add spirv-headers-for-skia Maxim Cournoyer
2023-08-29 21:11 ` [bug#60571] [PATCH v3 1/3] gnu: Add icu4c-for-skia Nicolas Graves via Guix-patches via
2023-08-29 21:11 ` [bug#60571] [PATCH v3 2/3] gnu: skia: Activate tests Nicolas Graves via Guix-patches via
2023-08-29 21:11 ` [bug#60571] [PATCH v3 3/3] gnu: skia: Update to 110.0.0f3fb7a Nicolas Graves via Guix-patches via
2023-09-02 13:33 ` [bug#60571] activating tests for skia 2D graphics library Maxim Cournoyer
2023-09-02 16:17 ` Maxim Cournoyer
2023-09-05 15:07 ` [bug#60571] [PATCH v4 1/3] gnu: skia: Update to 110.0.0f3fb7a Nicolas Graves via Guix-patches via
2023-09-05 15:07 ` [bug#60571] [PATCH v4 2/3] gnu: skia: Update to 112.0.6d0b938 Nicolas Graves via Guix-patches via
2023-09-05 15:07 ` [bug#60571] [PATCH v4 3/3] gnu: python-skia-pathops: Update to 0.8.0 Nicolas Graves via Guix-patches via
2023-09-06 1:08 ` bug#60571: " Maxim Cournoyer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874jkjgoch.fsf@gmail.com \
--to=maxim.cournoyer@gmail.com \
--cc=60571@debbugs.gnu.org \
--cc=ngraves@ngraves.fr \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).