[bug#60571,v2,3/4] gnu: Add icu4c-for-skia.

* gnu/packages/icu4c.scm (icu4c-for-skia): New variable.
---
 gnu/packages/icu4c.scm | 101 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)
  

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?
  

On 2023-08-28 11:48, Maxim Cournoyer wrote:

> Hello!
>
> Nicolas Graves <ngraves@ngraves.fr> writes:
>
>
> The comments are a bit terse; I'd expound them to complete sentences
> with a tad more context.

Will try to expand a bit, but that will be hard, the commits are very
old and my memory of this mostly faded.

>
> 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.

IIRC, since we use this version of icu4c to run tests, we needed to
also build some "data". It's rather a reconfiguration to build data
rather than a re-run of the configuration script post-installation.

>
> 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?

Doing so 

--
Best regards,
Nicolas Graves
  

Hi Nicolas,

Nicolas Graves <ngraves@ngraves.fr> writes:

> On 2023-08-28 11:48, Maxim Cournoyer wrote:
>
>> Hello!
>>
>> Nicolas Graves <ngraves@ngraves.fr> writes:
>>
>>
>> The comments are a bit terse; I'd expound them to complete sentences
>> with a tad more context.
>
> Will try to expand a bit, but that will be hard, the commits are very
> old and my memory of this mostly faded.
>
>>
>> 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.
>
> IIRC, since we use this version of icu4c to run tests, we needed to
> also build some "data". It's rather a reconfiguration to build data
> rather than a re-run of the configuration script post-installation.

That would perfect in an explanatory comment :-).

>>
>> 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?
>
> Doing so 

Super!  Thanks for seeing it through.