diff mbox series

[bug#71109] gnu: vulkan-tools: Wrap binaries with LD_LIBRARY_PATH.

Message ID 0f2a806ee6259147a098e8d672d03ed768fde5fa.1716370579.git.sughosha@disroot.org
State New
Headers
Series [bug#71109] gnu: vulkan-tools: Wrap binaries with LD_LIBRARY_PATH. |

Commit Message

Sughosha May 22, 2024, 9:36 a.m. UTC 
This fixes not finding vulkan-loader.

* gnu/packages/vulkan.scm (vulkan-tools)[arguments]<#:phases>:
Wrap-binaries with LD_LIBRARY_PATH.

Change-Id: I9aaf1cf04f70f1da976fa84d2189ca2c01b9520f
---
 gnu/packages/vulkan.scm | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)


base-commit: e9b25a6c6c626a560d28a1f732e6e5d362d584a4

Comments

Kaelyn Takata May 26, 2024, 6:51 p.m. UTC | #1 
Hi,

This patch looks good to me, and I've confirmed it fixes the issue with vulkaninfo not running.

I am curious though as to what caused vulkaninfo to need LD_LIBRARY_PATH, as before the the vulkan updates a couple of months ago (at least as of gitish 4d79a9c from early April), vulkaninfo worked fine without needing LD_LIBRARY_PATH set. The new need does have me a bit concerned that other vulkan apps which used to work may need a similar workaround to keep functioning as expected.

Cheers,
Kaelyn
Ludovic Courtès May 31, 2024, 10:38 a.m. UTC | #2 
Hi,

Sughosha <sughosha@disroot.org> skribis:

> This fixes not finding vulkan-loader.

Could you show how to test it?

> +                          (wrap-program file
> +                            `("LD_LIBRARY_PATH" ":" =
> +                               (,(getenv "LIBRARY_PATH")))))

This is probably a bit too broad because LIBRARY_PATH includes
build-time-only dependencies (python, gawk, binutils, bzip2, etc.).

Could we explicitly list what needs to be there instead?

Also maybe change ‘=’ to ‘suffix’ so users can still override
LD_LIBRARY_PATH.

Thanks,
Ludo’.
Sughosha May 31, 2024, 3:21 p.m. UTC | #3 
On Friday, May 31, 2024 4:08:53 PM IST Ludovic Courtès wrote:
> Hi,
> 
> Sughosha <sughosha@disroot.org> skribis:
> > This fixes not finding vulkan-loader.
> 
> Could you show how to test it?
> 
> > +                          (wrap-program file
> > +                            `("LD_LIBRARY_PATH" ":" =
> > +                               (,(getenv "LIBRARY_PATH")))))
> 
> This is probably a bit too broad because LIBRARY_PATH includes
> build-time-only dependencies (python, gawk, binutils, bzip2, etc.).
> 
> Could we explicitly list what needs to be there instead?
> 
> Also maybe change ‘=’ to ‘suffix’ so users can still override
> LD_LIBRARY_PATH.
> 
> Thanks,
> Ludo’.
Hi,
Thanks for your review!

> Could you show how to test it?
Currently if I just run `vulkaninfo`, this error appears:
```
ERROR at /tmp/guix-build-vulkan-tools-1.3.280.0.drv-0/source/vulkaninfo/./
vulkaninfo.h:412: Failed to initialize: Vulkan loader is not installed, not 
found, or failed to load.
```
I don't know if something should be patched before building itself.
But if I run: `LD_LIBRARY_PATH=$(guix build vulkan-loader)/lib vulkaninfo`, 
the program will be executed without any error.

> This is probably a bit too broad because LIBRARY_PATH includes
> build-time-only dependencies (python, gawk, binutils, bzip2, etc.).
If I wrap only with vulkan-loader's library path also, it works. But would it 
be a "right" way of wrapping a program.

> Also maybe change ‘=’ to ‘suffix’ so users can still override
Yes, you are right. I will change it.
Kaelyn Takata May 31, 2024, 4:47 p.m. UTC | #4 
Hi,

On Friday, May 31st, 2024 at 3:38 AM, Ludovic Courtès <ludo@gnu.org> wrote:

> 
> 
> Hi,
> 
> Sughosha sughosha@disroot.org skribis:
> 
> > This fixes not finding vulkan-loader.
> 
> 
> Could you show how to test it?

At least for me, the problem shows up simply with vulkan-tools installed (or using "guix shell vulkan-tools") using a guix commit after the latest (1.3.280) vulkan updates. Simply running "vulkaninfo" results in the error:

ERROR at /tmp/guix-build-vulkan-tools-1.3.280.0.drv-0/source/vulkaninfo/./vulkaninfo.h:412: Failed to initialize: Vulkan loader is not installed, not found, or failed to load.

> 
> > + (wrap-program file
> > + `("LD_LIBRARY_PATH" ":" =
> > + (,(getenv "LIBRARY_PATH")))))
> 
> 
> This is probably a bit too broad because LIBRARY_PATH includes
> build-time-only dependencies (python, gawk, binutils, bzip2, etc.).
> 
> Could we explicitly list what needs to be there instead?

A bit of local testing just now suggests setting LD_LIBRARY_PATH to the vulkan-loader package's lib dir should be enough to resolve the error.

Cheers,
Kaelyn

> Also maybe change ‘=’ to ‘suffix’ so users can still override
> LD_LIBRARY_PATH.
> 
> Thanks,
> Ludo’.
diff mbox series

Patch

diff --git a/gnu/packages/vulkan.scm b/gnu/packages/vulkan.scm
index 4c53a19aba..dcf97e98dd 100644
--- a/gnu/packages/vulkan.scm
+++ b/gnu/packages/vulkan.scm
@@ -368,7 +368,16 @@  (define-public vulkan-tools
                   (replace 'check
                     (lambda* (#:key tests? #:allow-other-keys)
                       (when tests?
-                        (invoke "./tests/vulkan_tools_tests")))))))
+                        (invoke "./tests/vulkan_tools_tests"))))
+                  (add-after 'install 'wrap-binaries
+                    (lambda* (#:key outputs #:allow-other-keys)
+                      (for-each
+                        (lambda (file)
+                          (wrap-program file
+                            `("LD_LIBRARY_PATH" ":" =
+                               (,(getenv "LIBRARY_PATH")))))
+                        (find-files (string-append (assoc-ref outputs "out")
+                                                   "/bin"))))))))
     (home-page
      "https://github.com/KhronosGroup/Vulkan-Tools")
     (synopsis "Tools and utilities for Vulkan")