Subject: Re: [PATCH v2 1/2] gnu: Add llvm-for-mesa.

From: Ludovic Courtès <[email-masked]>
Date: Mon, 23 Jan 2023 11:35:44 +0100

Hello!

Efraim Flashner <[email-masked]> skribis:

> * gnu/packages/llvm.scm (llvm-for-mesa): New variable.

Yay for reduced closures!!

> +(define-public llvm-for-mesa

Maybe add a line saying that this is a slim variant specifically
tailored for Mesa, that takes X% of the size of the default LLVM.

> +  ;; Note: update the 'clang' input of mesa-opencl when bumping this.
> +  (let ((base-llvm llvm-15))
> +    (package
> +      (inherit base-llvm)

Add (name "llvm-for-mesa") ?

> +      (arguments
> +       (substitute-keyword-arguments (package-arguments base-llvm)
> +         ((#:modules modules '((guix build cmake-build-system)
> +                               (guix build utils)))
> +          `((ice-9 regex)
> +            (srfi srfi-1)
> +            (srfi srfi-26)
> +            ,@modules))
> +         ((#:configure-flags cf ''())
> +          #~(cons*
> +              ;; AMDGPU is needed by the vulkan drivers.
> +              #$(string-append "-DLLVM_TARGETS_TO_BUILD="
> +                               (system->llvm-target) ";AMDGPU")

So the result is two build only two backends, for example x86_64 and
AMDGPU, right?

> +         ((#:phases phases '%standard-phases)
> +          #~(modify-phases #$phases
> +              (add-after 'install 'delete-static-libraries
> +                ;; If these are just relocated then llvm-config can't find them.
> +                (lambda* (#:key outputs #:allow-other-keys)
> +                  (for-each delete-file
> +                            (find-files (string-append
> +                                          (assoc-ref outputs "out") "/lib")
> +                                        "\\.a$"))))

Should we pass -DDISABLE_STATIC=ON or whatever it’s called instead?

> +              (add-after 'install 'build-and-install-llvm-config
> +                (lambda* (#:key outputs #:allow-other-keys)

Why do we need this extra phase compared to ‘llvm’?  Please add a
comment.  :-)

> +                  (let ((out (assoc-ref outputs "out")))
> +                    (substitute*
> +                      "tools/llvm-config/CMakeFiles/llvm-config.dir/link.txt"
> +                      (((string-append "/tmp/guix-build-llvm-"
> +                                       #$(package-version base-llvm)
> +                                       ".drv-0/build/lib"))
> +                       (string-append out "/lib")))

The non-literal pattern here, and that it explicitly refers to the build
directory, is not great.

I believe you can use (string-append (getcwd) "/lib") as the pattern,
fixing the second problem.  Not sure about the first one.

Thank you!

Ludo’.