[bug#48325] julia-1.6 guix

[zimoun]

Hi Jean-Baptiste,

Thanks for the patch.  Here some minor comments.

On Mon, 10 May 2021 at 11:29, Jean-Baptiste Volatier <jbv@pm.me> wrote:

> From e610dacab669ce84fe8f263a01aefff1fe49b6aa Mon Sep 17 00:00:00 2001
> From: Jean-Baptiste Volatier <jbv@pm.me>
> Date: Mon, 10 May 2021 09:57:23 +0200
> Subject: [PATCH] gnu: julia: update to 1.6.1
>
> gnu: openlibm: update to 0.7.4
> gnu: pcre2: update to 10.56
> gnu: utf8proc: update to 2.6.1
> gnu: julia-benchmarktools: update to 0.7.0

Please, split this patch.  One per update, i.e., 5 patches I guess.

> diff --git a/gnu/packages/julia.scm b/gnu/packages/julia.scm
> index 13c9f7baf1..39627eeed0 100644
> --- a/gnu/packages/julia.scm
> +++ b/gnu/packages/julia.scm
> @@ -1,9 +1,10 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2015, 2016, 2017 Ricardo Wurmus <rekado@elephly.net>
>  ;;; Copyright © 2016, 2020 Efraim Flashner <efraim@flashner.co.il>
> -;;; Copyright © 2020 Nicolò Balzarotti <nicolo@nixo.xyz>
> +;;; Copyright © 2020, 2021 Nicolò Balzarotti <nicolo@nixo.xyz>

Just to be sure, if Nicoló is co-author, it should be worth to add them
in the commit message, something like:

Co-Authored-By: Nicolò Balzarotti <nicolo@nixo.xyz>.

> -    (source (origin
> -              (inherit (package-source llvm-9))
> -              ;; Those patches are inside the Julia source repo.
> -              ;; They are _not_ Julia specific 
> (https://github.com/julialang/julia#llvm)
> -              ;; but they are required to build Julia.
> -              ;; Discussion: 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=919628
> -              (patches
> -               (map (match-lambda
> -                      ((name hash)
> -                       (julia-patch name hash)))
> -                    (list
> -                     '("llvm-D27629-AArch64-large_model_6.0.1"
> -                       
> "1qrshmlqvnasdyc158vfn3hnbigqph3lsq7acb9w8lwkpnnm2j4z")
> -                     '("llvm8-D34078-vectorize-fdiv"
> -                       
> "19spqc3xsazn1xs9gpcgv9ldadfkv49rmc5khl7sf1dlmhgi4602")
> -                     '("llvm-7.0-D44650"
> -                       
> "1h55kkmkiisfj6sk956if2bcj9s0v6n5czn8dxb870vp5nccj3ir")
> -                     '("llvm9-D50010-VNCoercion-ni"
> -                       
> "1s1d3sjsiq4vxg7ncy5cz56zgy5vcq6ls3iqaiqkvr23wyryqmdx")
> -                     '("llvm-exegesis-mingw"
> -                       
> "0ph1cj1j7arvf1xq2xcr7qf9g0cpdl14fincgr67vpi520zvd3vp")
> -                     '("llvm-test-plugin-mingw"
> -                       
> "12z738cnahbf6n381im7i0hxp1m6k9hrnfjlmq9sac46nxly9gnj")
> -                     '("llvm7-revert-D44485"
> -                       
> "0f59kq3p3mpwsbmskypbi4zn01l6ig0x7v2rjp08k2r8z8m6fa8n")
> -                     '("llvm-8.0-D66657-codegen-degenerate"
> -                       
> "1n1ddx19h90bbpimdyd9dh8fsm6gb93xxyqm4ljkxa1k3cx2vm72")
> -                     '("llvm-8.0-D71495-vectorize-freduce"
> -                       
> "1zff08wvji9lnpskk4b3p5zyjsy5hhy23ynxjqlj9dw7jvvfrf0p")
> -                     '("llvm-D75072-SCEV-add-type"
> -                       
> "029a3fywsm233vf48mscina24idd50dc75wr70lmimrhwnw27p0z")
> -                     '("llvm-9.0-D65174-limit-merge-stores"
> -                       
> "04bff1mnblfj9mxfdwr1qdnw3i3szmp60gnhxwas5y68qg33z6j0")
> -                     '("llvm9-D71443-PPC-MC-redef-symbol"
> -                       
> "1c93nv7rgc9jg5mqrnvv08xib1789qvlql94fwggh18mp3b9hbgy")
> -                     '("llvm-9.0-D78196"
> -                       
> "08a43hyg7yyqjq2vmfsmppf34xcz60wq6y9zw5fdyhw2h1mcnmns")
> -                     '("llvm-julia-tsan-custom-as"
> -                       
> "0awh40kf6lm4wn1nsjd1bmhfwq7rqj811szanp2xkpspykw9hg9s")
> -                     '("llvm-9.0-D85499"
> -                       
> "0vxlr35srvbvihlgrxq15v6dylp90vgi0qahj22j01jgqmdasjkm"))))
> -              (patch-flags '("-p1"))))
>      (arguments
> -     (substitute-keyword-arguments (package-arguments llvm-9)
> +     (substitute-keyword-arguments (package-arguments llvm-11)
>         ((#:configure-flags flags)
>          `(list ;; Taken from NixOS. Only way I could get libLLVM-6.0.so
>             "-DCMAKE_BUILD_TYPE=Release"
> @@ -177,7 +140,61 @@
>             ;; "-DLLVM_DEFAULT_TARGET_TRIPLE=${stdenv.hostPlatform.config}"
>             ;; "-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD=WebAssembly"
>             "-DLLVM_ENABLE_DUMP=ON"
> -           "-DLLVM_LINK_LLVM_DYLIB=ON"))))))
> +           "-DLLVM_LINK_LLVM_DYLIB=ON"))
> +       ((#:phases phases)
> +        `(modify-phases ,phases
> +           ;; applying patches from julia
> +           ;; list of patches can be found in deps/llvm.mk in julia source
> +           (add-after 'unpack 'julia-patches
> +             (lambda* (#:key inputs outputs #:allow-other-keys)
> +               (let ((patch
> +                      (lambda (patchname flag)
> +                        (invoke "patch" flag "-i"
> +                                (string-append
> +                                 "julia-src/deps/patches/"
> +                                 patchname
> +                                 ".patch")))))
> +                 (mkdir-p "julia-src")
> +                 (invoke "tar" "xf"
> +                         (assoc-ref inputs "julia-source")
> +                         "-C" "julia-src" "--strip-components=1")
> +                 (map (lambda (patchname)
> +                        (patch patchname "-p1"))
> +                      (list "llvm-D27629-AArch64-large_model_6.0.1"
> +                            "llvm8-D34078-vectorize-fdiv"
> +                            "llvm-7.0-D44650"
> +                            "llvm-6.0-DISABLE_ABI_CHECKS"
> +                            "llvm9-D50010-VNCoercion-ni"
> +                            "llvm7-revert-D44485"
> +                            "llvm-11-D75072-SCEV-add-type"
> +                            "llvm-julia-tsan-custom-as"
> +                            "llvm-D80101"
> +                            "llvm-D84031"
> +                            "llvm-10-D85553"
> +                            "llvm-10-unique_function_clang-sa"
> +                            "llvm-11-D85313-debuginfo-empty-arange"
> +                            "llvm-11-D90722-rtdyld-absolute-relocs"
> +                            "llvm-invalid-addrspacecast-sink"
> +                            "llvm-11-D92906-ppc-setjmp"
> +                            "llvm-11-PR48458-X86ISelDAGToDAG"
> +                            "llvm-11-D93092-ppc-knownbits"
> +                            "llvm-11-D93154-globalisel-as"
> +                            "llvm-11-ppc-half-ctr"
> +                            "llvm-11-ppc-sp-from-bp"
> +                            "llvm-rGb498303066a6-gcc11-header-fix"
> +                            "llvm-11-D94813-mergeicmps"
> +                            "llvm-11-D94980-CTR-half"
> +                            "llvm-11-D94058-sext-atomic-ops"
> +                            "llvm-11-D96283-dagcombine-half"))
> +                 (map (lambda (patchname)
> +                        (patch patchname "-p2"))
> +                      (list "llvm-11-AArch64-FastIsel-bug"
> +                            "llvm-11-D97435-AArch64-movaddrreg"
> +                            "llvm-11-D97571-AArch64-loh"
> +                            "llvm-11-aarch64-addrspace")))))))))

I am not convinced by this move of patches from ’source’ to ’phases’.
My understanding about the usual way is to let the patch in the source
field.  Is this move motivated by something special?

> -                                       '("arpack-ng" "curl" "dsfmt"

I have not read the Julia ChangeLog.  Do they remove Arpack?  This
should be mentioned in the commit message.

> +                                       '("curl" "dsfmt"
>                                           "gmp" "lapack"
> -                                         "libssh2" "libgit2"
> +                                         "libssh2" "libnghttp2" "libgit2"

Idem for libnghttp2.

>                                           "mbedtls" "mpfr"
>                                           "openblas" "openlibm" "pcre2"
> -                                         "suitesparse"))
> -                                  ":"))
> -             #t))
> +                                         "suitesparse" "libfortran"))

Idem for libfortran.

> -         (add-before 'build 'fix-precompile
> -           (lambda _
> -             (substitute* "base/loading.jl"
> -               (("something(Base.active_project(), \"\")") "\"\""))
> +         (add-before 'build 'shared-objects-paths
> +           (lambda* (#:key inputs #:allow-other-keys)

[...]

> +               ;; FAILING: OpenBLAS

What does it mean?


> +         (add-before 'install 'symlink-libraries

[...]

> +               (link "zlib" "usr/lib/julia/" "libz\\.so")

Does this fix

   <http://issues.guix.gnu.org/48238>

?  If yes, cool and thank you! :-)  So it should be mentioned in the
commit message, something like:

--8<---------------cut here---------------start------------->8---
* gnu: julia: Update to 1.6.1.

Fixes <https://bug.gnu.org/48238>.

* gnu/packages/julia.scm (julia): Update to 1.6.1.
[arguments]: …stuff that changed…
[inputs]: Add foo, Remove bar.

Co-Authored-By: Nicolò Balzarotti <nicolo@nixo.xyz>.
--8<---------------cut here---------------end--------------->8---

Does it make sense?

> -         "USE_SYSTEM_ARPACK=1"

What is the motivation for removing Arpack?  Sorry if my question is naive.

>           "USE_SYSTEM_LIBGIT2=1"
>           "USE_SYSTEM_ZLIB=1")))
>      (inputs
>       `(("llvm" ,llvm-julia)
>         ("p7zip" ,p7zip)
> -       ;; The bundled version is 3.3.0 so stick to that version.  With other
> -       ;; versions, we get test failures in 'linalg/arnoldi' as described in
> -       ;; <https://bugs.gnu.org/30282>.
> -       ("arpack-ng" ,arpack-ng-3.3.0)
> -
> -       ("coreutils" ,coreutils) ;for bindings to "mkdir" and the like
> +       ("coreutils" ,coreutils)         ;for bindings to "mkdir" and the like

This is not a change.  Even if the new indentation is correct, please
let avoid cosmetic change in the same commit updating a complex package.
Because then digging in the history becomes more complex. :-)

>         ("lapack" ,lapack)
> -       ("openblas" ,openblas) ;Julia does not build with Atlas
> +       ("openblas" ,openblas)           ;Julia does not build with Atlas

Idem.

>         ("libunwind" ,libunwind-julia)
>         ("openlibm" ,openlibm)
>         ("mbedtls" ,mbedtls-apache)
>         ("curl" ,curl)
> -       ("libgit2" ,libgit2-0.28)
> +       ("libnghttp2" ,nghttp2 "lib")
> +       ("libgit2" ,libgit2)
>         ("libssh2" ,libssh2)
>         ("fortran" ,gfortran)
> +       ;; required for libgcc_s.so
> +       ("libfortran" ,gfortran "lib")
>         ("libuv" ,libuv-julia)
>         ("pcre2" ,pcre2)
>         ("utf8proc" ,utf8proc)
>         ("mpfr" ,mpfr)
> +       ("nss-certs" ,nss-certs)         ; required to precompile

Hum?  Is it really necessary?

> +       ("glibc-locales" ,glibc-locales)

Idem.  Is it really necessary?  Because it is a “big“ packages which
drastically increases the closure size of the Julia package.



Thanks again for the patch.


Cheers,
simon