Jorge Acereda schreef op vr 07-01-2022 om 19:37 [+0100]:
> The package version is the same one used in nixpkgs (current tip).
> Should I add some "unstable" string somewhere?  Also, I'm pretty sure
> I overcomplicated things, there must be some easier way to patch the
> executable paths.

Ok, but anyone looking at the package definition of gpu-switch would
be having a hard time figuring out these reasons.  Also, the version
used by nixpkgs isn't very relevant; nixpkgs might be out-of-date.

An "-unstable" version suffix isn't very informative, and doesn't
seem correct here: there haven't been any changes in gpu-switch for
about five years, which seems rather stable to me.

I suggest having a look at ‘17.4.3 Version Numbers’ in the manual,
in particular the text about VCS vs formal releases.

Because upstream isn't formally releasing anything, using a revision
from git seems appropriate to me, but the reasons needs to be
documented with a comment.  E.g., see 'emacs-graphql-mode'.

> * gnu/packages/graphics.scm (gpu-switch): New variable.
> ---
>  gnu/packages/graphics.scm | 58 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/gnu/packages/graphics.scm b/gnu/packages/graphics.scm
> index fe35aaad2d..d425a18c18 100644
> --- a/gnu/packages/graphics.scm
> +++ b/gnu/packages/graphics.scm

GPUs aren't only used for graphics, see e.g.
<https://boinc.berkeley.edu/wiki/GPU_computing>.
I would put it in hardware.scm instead.

> @@ -27,6 +27,7 @@
>  ;;; Copyright © 2021 Andy Tai <atai@atai.org>
>  ;;; Copyright © 2021 Ekaitz Zarraga <ekaitz@elenq.tech>
>  ;;; Copyright © 2021 Vinicius Monego <monego@posteo.net>
> +;;; Copyright © 2022 Jorge Acereda <jacereda@gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -113,12 +114,14 @@ (define-module (gnu packages graphics)
>    #:use-module (guix build-system meson)
>    #:use-module (guix build-system python)
>    #:use-module (guix build-system qt)
> +  #:use-module (guix build-system trivial)
>    #:use-module (guix download)
>    #:use-module (guix git-download)
>    #:use-module (guix hg-download)
>    #:use-module ((guix licenses) #:prefix license:)
>    #:use-module (guix packages)
> -  #:use-module (guix utils))
> +  #:use-module (guix utils)
> +  #:use-module (ice-9 match))


Not needed, the use of 'match' is only at the build side.

>  
>  (define-public mmm
>    (package
> @@ -2011,4 +2014,56 @@ (define-public monado
>  such as VR and AR on mobile, PC/desktop, and any other device.  Monado aims to be
>  a complete and conforming implementation of the OpenXR API made by Khronos.")
>      (license license:boost1.0)))
>  +
> +(define-public gpu-switch
> +  (package
> +    (name "gpu-switch")
> +    (version "2017-04-28")
> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri (git-reference
> +             (url "https://github.com/0xbb/gpu-switch")
> +             (commit "a365f56d435c8ef84c4dd2ab935ede4992359e31")))
> +       (file-name (git-file-name name version))
> +       (sha256
> +        (base32 "1jnh43nijkqd83h7piq7225ixziggyzaalabgissyxdyz6szcn0r"))))
> +    (build-system trivial-build-system)
> +    (inputs
> +     (list bash e2fsprogs util-linux grep coreutils which))

I suggest bash-minimal and coreutils-minimal to reduce the closure.

> +    (arguments
> +     `(#:modules ((guix build utils))
> +       #:builder
> +       (begin
> +         (use-modules (guix build utils) (ice-9 match))
> +         (let* ((out (assoc-ref %outputs "out"))

%outputs is sort-of deprecated, it is recommended to use G-exps
instead: (let* ((out #$output) ...) ...)

> +                (gpu-switch (search-input-file %build-inputs "gpu-switch"))

Likewise, %build-inputs is deprecated, and it doesn't do the right
thing when cross-compiling, because (implicit) native inputs go before
native inputs. In this particular case, it would work, but I'd avoid
this fragility, by doing something like (let (... (inputs #$inputs)
(gpu-switch (search-input-file inputs "gpu-switch"))) ...) instead.

Personally, I'd do #$(file-append source "/gpu-switch") instead though.

> +                (bin (string-append out "/bin"))
> +                (out-gpu-switch (string-append bin "/gpu-switch"))
> +                (readme (search-input-file %build-inputs "README.md")))

Likewise.

> +           (install-file gpu-switch bin)

The shebang starts with

#!/usr/bin/env /gnu/store/[a hash]-bash-5.1.8/bin/bash

so the script depends on the system's /usr/bin/env. Can this dependency
be removed, e.g. using patch-shebang?

The script has a line

  $(/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename $0) --integrated   # Switch to the integrated GPU

but this is fragile, what if I create a symlink named "switch the gpu"
pointing to gpu-switch (without the quotes, and with the spaces)?

Then I get

$ ./switch\ the\ gpu 
/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename: extra operand 'gpu'
Try '/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename --help' for more information.
/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename: extra operand 'gpu'
Try '/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename --help' for more information.
/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename: extra operand 'gpu'
Try '/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/basename --help' for more information.

Can this be fixed (upstream)?  Putting "" around the $0 would probably
be enough.  Also, I thought it might be required to place -- before the
"$0" (in case the symlink is named "--help"), but it seems to work
without in my tests.  I would still recommend an -- argument though,
to make things less fragile.

> +           (for-each
> +            (match-lambda
> +              ((pkg . nm) (substitute* out-gpu-switch

I see the following line in the output of "gpu-switch"

  gpu-switch --dedi/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-
coreutils-8.32/bin/cated    # Switch to the
dedi/gnu/store/y5jxkx484x7s2c2n7dc8wprh5sbps7pl-coreutils-
8.32/bin/cated GPU   

Likewise:

      printf "Fatal: Couldn't
/gnu/store/64d0mxsjqifrpashlhyd3rf7zm2r709x-util-linux-2.37.1/bin/mount
'${sysfs_efi_vars}'.\n" 1>&2

Seems like the substitutions are not sufficiently specific.

> +                            ((nm)
> +                             (string-append (assoc-ref %build-inputs pkg)

%build-inputs -> #$inputs? Or maybe even use search-input-file.

Also, you can substitute multiple things with a single substitute*.
E.g., 

          ;; From the manual, see (guix)Build Utilities
          (substitute* file
            (("hello")
             "good morning\n")
            (("foo([a-z]+)bar(.*)$" all letters end)
             (string-append "baz" letter end)))

> +           #t))))

Returning #true in phases is not required anymore, presumably the same
holds for #:builder.

> +    (home-page "https://github.com/0xbb/gpu-switch")
> +    (synopsis "GPU switcher for dual-GPU MacBook Pro models")
> +    (description
> +     "Switch between the integrated and dedicated GPU of dual-GPU MacBook Pro
> +models for the next reboot.

Is this specific for ‘MacBook Pro models’, or does it work for any
computer that has a certain combination of ‘integrated’ and ‘dedicated’
GPU?

> +It aims to remove the need of booting into OS X and running gfxCardStatus
> +v2.2.1 to switch to the integrated card.")

Is this v2.2.1 important?

Greetings,
Maxime.