On Mon, Jan 23, 2023 at 11:35:44AM +0100, Ludovic Courtès wrote:
> Hello!
> 
> Efraim Flashner <efraim@flashner.co.il> 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.

Will do.

> > +  ;; 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") ?

I don't have a strong preference. By leaving it as 'llvm' we make it
more easily substitutable by any other build of llvm since they'd have
the same name length. Having it as 'llvm-for-mesa' makes it really clear
what it is.

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

That's right. I tried with just (system->llvm-target) but AMDGPU was
needed for the vulkan drivers.

> > +         ((#: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?

It turns out that static is the default, and we override it in most of
the llvm packages to build shared libraries instead. If we disable the
static ones too then it errors out and says it has nothing to build.

> > +              (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.  :-)

Fair enough :) It turns out that *everyone* expects llvm-config to
exist, and doing it this way allows us to only build llvm-config, not
all the tools/utilities.

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

I suppose it would be (string-append (getcwd) "/bin/lib") to fix both. I
wasn't able to find a good way to get llvm-config to be configured
correctly by the build system without also having it install all the
utilities. When I looked in the build directory after a RUNPATH failure
it had that directory as the one to link to for the libraries.

> Thank you!

This one's been bothering everyone for years :)

-- 
Efraim Flashner   <efraim@flashner.co.il>   אפרים פלשנר
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted