Jorge Acereda schreef op vr 07-01-2022 om 19:37 [+0100]:
Toggle quote (5 lines)
> 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'.
Toggle quote (10 lines)
> * 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.
I would put it in hardware.scm instead.
Toggle quote (23 lines)
> @@ -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.
Toggle quote (25 lines)
>
> (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.
Toggle quote (7 lines)
> + (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) ...) ...)
Toggle quote (2 lines)
> + (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.
Toggle quote (4 lines)
> + (bin (string-append out "/bin"))
> + (out-gpu-switch (string-append bin "/gpu-switch"))
> + (readme (search-input-file %build-inputs "README.md")))
Toggle quote (2 lines)
> + (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.
Toggle quote (4 lines)
> + (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.
Toggle quote (3 lines)
> + ((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)))
Toggle quote (2 lines)
> + #t))))
Returning #true in phases is not required anymore, presumably the same
holds for #:builder.
Toggle quote (6 lines)
> + (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?
Toggle quote (3 lines)
> +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.
