Message ID | 28d93ea0e4bda4dbbc9a653cfefb7e569b827ada.1693140822.git.ngraves@ngraves.fr |
---|---|
State | New |
Headers | show |
Series | [bug#60571,v2,1/4] gnu: Add spirv-headers-for-skia. | expand |
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.
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. + (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)) + (arguments + (list + #:make-flags #~`(,(string-append "DESTDIR=" #$output)) + #:configure-flags #~'(;;"--enable-shared=no" "--enable-static=yes" + "--prefix=" "--exec-prefix=") + #: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) + (let ((bash (search-input-file inputs "/bin/sh"))) + ;; Replace bash executable. + (setenv "CONFIG_SHELL" bash) + (substitute* "./configure" + (("`/bin/sh") + (string-append "`" bash))) + ;; Make the static library position-independent. + ;; (substitute* "./icudefs.mk.in" + ;; (("^CFLAGS = ") + ;; "CFLAGS = -fPIC ") + ;; (("^CXXFLAGS = ") + ;; "CXXFLAGS = -fPIC ")) + ;; Update runpath. + (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. + (with-directory-excursion "data" + `(,invoke "make" "clean" ,@make-flags)) + ;; Set prefix and exec-prefix. + (substitute* "./runConfigureICU" + (("^OPTS=") + (string-append + "OPTS=\"" (string-join configure-flags " ") "\""))) + ;; Configure for common data. + (setenv "ICU_DATA_FILTER_FILE" + (string-append (getcwd) "/../filters/common.json")) + (invoke "./runConfigureICU" "Linux/gcc" + "--disable-layout" "--disable-tests"))) + (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) + (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") + ""))))))))))