Subject: Re: [bug#29625] [PATCH core-updates] Vulkan patch series.
From: Marius Bakke <[email-masked]>
To: Rutger Helling <[email-masked]>
Cc: [email-masked]
Date: Mon, 18 Dec 2017 01:26:21 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Rutger Helling <[email-masked]> writes:

> Hey Marius, 
>
> thanks for the feedback!
> I've changed the patches and made sure they work on the latest
> core-updates commit. 

Thank you!  One of the commits was not rebased, but I ended up doing a
lot of adjustments anyway (including re-indent), so I fixed it up.

Changes detailed below.  Most of the issues were cosmetic and should
have been caught in the first round, but I was in a hurry :-)

> From 9d13265ac579adfd7d18c7710e793fa78b4aa0f6 Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Thu, 14 Dec 2017 20:16:37 +0100
> Subject: [PATCH] gnu: mesa: Enable Vulkan drivers for Intel and Radeon.

[...]

> +         ;; Enable Vulkan on x86-64.
> +         ,@(match (%current-system)
> +             ((or "x86_64-linux")
> +                '("--with-vulkan-drivers=intel,radeon"))
> +             (_
> +              '("")))

The "or" here was not doing anything useful.  Is there any particular
reason i686 is not supported?

> From 35b07f1e24c8597bdd504ae9f986abed486cb8df Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Fri, 8 Dec 2017 13:39:16 +0100
> Subject: [PATCH] gnu: local.mk: Add vulkan.scm.
>
> * gnu/local.mk: Add vulkan.scm.

I squashed this into the next patch.

> From 9c0b6c5e7729d94651b54c9597496f284cac5dbe Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Thu, 14 Dec 2017 22:37:45 +0100
> Subject: [PATCH] gnu: vulkan: Add spirv-headers.
>
> * gnu/packages/vulkan.scm: Create file. (spirv-headers): New variable.

[...]

> +     (license license:non-copyleft)))) ;; Custom license. See
> +     ;; https://github.com/KhronosGroup/SPIRV-Headers/blob/master/LICENSE for
> +     ;; details.

Note: "non-copyleft" and "x11-style" are procedures and takes a URI as
argument.  I changed this back to x11-style which was indeed more
appropriate with a link to the upstream license:

      (license (license:x11-style
                (string-append "https://github.com/KhronosGroup/SPIRV-Headers/blob/"
                               commit "/LICENSE")))

> From 80e82f1f92823e04893e8400dc82b69e890c7276 Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Fri, 8 Dec 2017 14:56:36 +0100
> Subject: [PATCH] gnu: vulkan: Add spirv-tools.
>
> * gnu/packages/vulkan.scm (spirv-tools): New variable.

[...]

> +   (inputs `(("python" ,python)
> +             ("spirv-headers" ,spirv-headers)))
> +   (native-inputs `(("pkg-config", pkg-config)))

I moved python to native-inputs, since it is only needed for building.

> From e45701483f559eccac56e087fb40e075afe2ffd3 Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Thu, 14 Dec 2017 23:03:02 +0100
> Subject: [PATCH] gnu: vulkan: Add glslang.
>
> * gnu/packages/vulkan.scm (glslang): New variable.

[...]

> +(define-public glslang
> +  ;; Version 3.0 is too old for vulkan-icd-loader. Use a recent git commit
> +  ;; until the next stable version.
> +  (let ((commit "471bfed0621162a7513fc24a51e8a1ccc2e640ff")
> +        (revision "1"))
> +    (package
> +     (name "glslang")
> +     (version (string-append "0.0-" revision "." (string-take commit 9)))

Since the last proper tag in the upstream repo was "3.0", I changed this
to (string-append "3.0-" ...).

> +   (inputs `(("bison" ,bison)))

Bison is only required for building, so I made it a native-input.

> +   (native-inputs `(("pkg-config" ,pkg-config)))
> +   (home-page "https://github.com/KhronosGroup/glslang")
> +   (synopsis "OpenGL and OpenGL ES shader front end and validator")
> +   (description "glslang is a OpenGL and OpenGL ES shader front end and
> +validator.")
> +   ;; Modified BSD license. See "copyright" section of
> +   ;; https://www.khronos.org/opengles/sdk/tools/Reference-Compiler/
> +   (license license:bsd-3))))

I changed the description to be the first paragraph of that URL :)
Also mentioned two Apache 2.0 header files that gets installed here.

> From 0172d3cb7cbdae03f143a4c7966c4694e69eea15 Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Fri, 8 Dec 2017 16:06:05 +0100
> Subject: [PATCH] gnu: vulkan: Add vulkan-icd-loader.
>
> * gnu/packages/vulkan.scm (vulkan-icd-loader): New variable.

[...]

> +   (inputs `(("glslang" ,glslang)
> +             ("libxcb" ,libxcb)
> +             ("libx11" ,libx11)
> +             ("libxrandr" ,libxrandr)
> +             ("mesa" ,mesa)
> +             ("python" ,python)
> +             ("spirv-tools" ,spirv-tools)
> +             ("wayland" ,wayland)))
> +   (native-inputs `(("pkg-config", pkg-config)))

Python was not referenced by the output, so I moved it to a native-input.

> +   (home-page (string-append "https://github.com/"
> +              "KhronosGroup/Vulkan-LoaderAndValidationLayers"))
> +   (synopsis "Khronos official ICD loader for Vulkan")
> +   (description "Vulkan-ICD-Loader provides Khronos official ICD loader and
> +validation layers for Vulkan developers on GNU/Linux.")

I mentioned that ICD is and expanded a little based on my limited
understanding of Vulkan.  Improvements welcome!

> +   (license license:asl2.0)))

And added a mention of the licenses listed in COPYRIGHT.txt.  While
snooping around I also found tests and tried adding this phase:

                  (replace 'check
                    (lambda _
                      (zero? (system* "bash" "tests/run_all_tests.sh")))))

But got 23/39 failures.  I suspect most can be resolved by pointing
$VK_LAYER_PATH to the right place in the build directory.  Added a FIXME
for now.

> From 3b45c0ea6d35a0fa7895344fe53758fb4b64d00f Mon Sep 17 00:00:00 2001
> From: Rutger Helling <[email-masked]>
> Date: Sun, 10 Dec 2017 11:15:03 +0100
> Subject: [PATCH] gnu: retroarch: Enable Vulkan support.
>
> * gnu/packages/games.scm (retroarch)[arguments]: Hard-code the path to
> libvulkan.so. [native-inputs]: Add vulkan-icd-loader.

Since libvulkan.so is used at runtime, it should be a normal input.

[...]

> @@ -1446,6 +1447,10 @@ either by Infocom or created using the Inform compiler.")
>             (lambda* (#:key outputs #:allow-other-keys)
>               (let* ((out (assoc-ref outputs "out"))
>                      (etc (string-append out "/etc")))
> +               ;; Hard-code the path to libvulkan.so.
> +               (substitute* "gfx/common/vulkan_common.c"
> +                 (("libvulkan.so") (string-append (assoc-ref %build-inputs
> +                  "vulkan-icd-loader") "/lib/libvulkan.so")))

I made this a little more idiomatic by dereferencing vulkan-icd-loader
in the let binding instead of the global %build-inputs.

Do you know if RetroArch (or rather the dependencies) will still work on
platform for which we don't have Vulkan support?

I've pushed this series as e1454e0e..01564e5a.