From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 08 10:38:37 2022 Received: (at 53059) by debbugs.gnu.org; 8 Jan 2022 15:38:37 +0000 Received: from localhost ([127.0.0.1]:48637 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1n6Dnc-0005DA-TX for submit@debbugs.gnu.org; Sat, 08 Jan 2022 10:38:37 -0500 Received: from baptiste.telenet-ops.be ([195.130.132.51]:39386) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1n6DnY-0005Cv-Ux for 53059@debbugs.gnu.org; Sat, 08 Jan 2022 10:38:32 -0500 Received: from [172.20.10.5] ([213.132.158.202]) by baptiste.telenet-ops.be with bizsmtp id gFeR260074NHtyl01FeStz; Sat, 08 Jan 2022 16:38:27 +0100 Message-ID: <7a15b34a40868bcbc7a63c3aea787b0f176e6077.camel@telenet.be> Subject: Re: [PATCH v2] gnu: Add gpu-switch. From: Maxime Devos To: Jorge Acereda , 53059@debbugs.gnu.org Date: Sat, 08 Jan 2022 15:38:20 +0000 In-Reply-To: <6a263efbca6f379b056e06887de50e6cccac9929.1641580314.git.jacereda@gmail.com> References: <6a263efbca6f379b056e06887de50e6cccac9929.1641580314.git.jacereda@gmail.com> Content-Type: multipart/signed; micalg="pgp-sha512"; protocol="application/pgp-signature"; boundary="=-a+QYe9mEPpn1ckpVoN+b" User-Agent: Evolution 3.38.3-1 MIME-Version: 1.0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=telenet.be; s=r22; t=1641656307; bh=God1taptmzmTmr+NgGX6NwMQZTnHwbjmOZDvOKKjKo8=; h=Subject:From:To:Date:In-Reply-To:References; b=Tn2r9SsIyfjKRXt8Er1w5dEi99/gu4NPrmXXA8Xs3VRqf8kgvZ673jOCQwsuYyozE p3+njTDna4DgMygd3UH6AoI7Y0ihC3CP6Pf8kWwCOGHOcerNWTWecpaShFng7BerYa xTrQZ7h7VQEqFYUMaqGrpRX3OwgQycXvh2R9gThP88u8L/56QYxxkOe9oFrjA7gLdv ayNdB+NI071CPsYFYgOjg0zxXcU8pXcIO6kNcWux6yvGHGfR22CSUeLIHOnctTfuP4 nQKPEWKFze+65/9ujTWdeXQm12uNo1Pssmwf+ZcLXakiE3LijXbm2T+9ft1WeW1tqS x1yAse+S/atyg== X-Spam-Score: 0.7 (/) X-Debbugs-Envelope-To: 53059 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) --=-a+QYe9mEPpn1ckpVoN+b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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?=C2=A0 Also, I'm pretty sur= e > 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 =E2=80=9817.4.3 Version Numbers=E2=80=99 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. > --- > =C2=A0gnu/packages/graphics.scm | 58 ++++++++++++++++++++++++++++++++++++= ++- > =C2=A01 file changed, 57 insertions(+), 1 deletion(-) >=20 > 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. > @@ -27,6 +27,7 @@ > =C2=A0;;; Copyright =C2=A9 2021 Andy Tai > =C2=A0;;; Copyright =C2=A9 2021 Ekaitz Zarraga > =C2=A0;;; Copyright =C2=A9 2021 Vinicius Monego > +;;; Copyright =C2=A9 2022 Jorge Acereda > =C2=A0;;; > =C2=A0;;; This file is part of GNU Guix. > =C2=A0;;; > @@ -113,12 +114,14 @@ (define-module (gnu packages graphics) > =C2=A0=C2=A0 #:use-module (guix build-system meson) > =C2=A0=C2=A0 #:use-module (guix build-system python) > =C2=A0=C2=A0 #:use-module (guix build-system qt) > +=C2=A0 #:use-module (guix build-system trivial) > =C2=A0=C2=A0 #:use-module (guix download) > =C2=A0=C2=A0 #:use-module (guix git-download) > =C2=A0=C2=A0 #:use-module (guix hg-download) > =C2=A0=C2=A0 #:use-module ((guix licenses) #:prefix license:) > =C2=A0=C2=A0 #:use-module (guix packages) > -=C2=A0 #:use-module (guix utils)) > +=C2=A0 #:use-module (guix utils) > +=C2=A0 #:use-module (ice-9 match)) Not needed, the use of 'match' is only at the build side. > =C2=A0 > =C2=A0(define-public mmm > =C2=A0=C2=A0 (package > @@ -2011,4 +2014,56 @@ (define-public monado > =C2=A0such as VR and AR on mobile, PC/desktop, and any other device.=C2= =A0 Monado aims to be > =C2=A0a complete and conforming implementation of the OpenXR API made by = Khronos.") > =C2=A0=C2=A0=C2=A0=C2=A0 (license license:boost1.0))) > =C2=A0+ > +(define-public gpu-switch > +=C2=A0 (package > +=C2=A0=C2=A0=C2=A0 (name "gpu-switch") > +=C2=A0=C2=A0=C2=A0 (version "2017-04-28") > +=C2=A0=C2=A0=C2=A0 (source > +=C2=A0=C2=A0=C2=A0=C2=A0 (origin > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (method git-fetch) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (uri (git-reference > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= (url "https://github.com/0xbb/gpu-switch") > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= (commit "a365f56d435c8ef84c4dd2ab935ede4992359e31"))) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (file-name (git-file-name name vers= ion)) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (sha256 > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (base32 "1jnh43nijkqd83h7piq7= 225ixziggyzaalabgissyxdyz6szcn0r")))) > +=C2=A0=C2=A0=C2=A0 (build-system trivial-build-system) > +=C2=A0=C2=A0=C2=A0 (inputs > +=C2=A0=C2=A0=C2=A0=C2=A0 (list bash e2fsprogs util-linux grep coreutils = which)) I suggest bash-minimal and coreutils-minimal to reduce the closure. > +=C2=A0=C2=A0=C2=A0 (arguments > +=C2=A0=C2=A0=C2=A0=C2=A0 `(#:modules ((guix build utils)) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #:builder > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (begin > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (use-modules (guix buil= d utils) (ice-9 match)) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (let* ((out (assoc-ref = %outputs "out")) %outputs is sort-of deprecated, it is recommended to use G-exps instead: (let* ((out #$output) ...) ...) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 (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. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 (bin (string-append out "/bin")) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 (out-gpu-switch (string-append bin "/gpu-switch")) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0 (readme (search-input-file %build-inputs "README.md"))) Likewise. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (install-fi= le 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=20 /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. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (for-each > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (matc= h-lambda > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0 ((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 =20 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. > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 ((nm) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 (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.,=20 ;; 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))) > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 #t)))) Returning #true in phases is not required anymore, presumably the same holds for #:builder. > +=C2=A0=C2=A0=C2=A0 (home-page "https://github.com/0xbb/gpu-switch") > +=C2=A0=C2=A0=C2=A0 (synopsis "GPU switcher for dual-GPU MacBook Pro mode= ls") > +=C2=A0=C2=A0=C2=A0 (description > +=C2=A0=C2=A0=C2=A0=C2=A0 "Switch between the integrated and dedicated GP= U of dual-GPU MacBook Pro > +models for the next reboot. Is this specific for =E2=80=98MacBook Pro models=E2=80=99, or does it work = for any computer that has a certain combination of =E2=80=98integrated=E2=80=99 and= =E2=80=98dedicated=E2=80=99 GPU? > +It aims to remove the need of booting into OS X and running gfxCardStatu= s > +v2.2.1 to switch to the integrated card.") Is this v2.2.1 important? Greetings, Maxime. --=-a+QYe9mEPpn1ckpVoN+b Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYdmv7BccbWF4aW1lZGV2 b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7lKlAQCGU7vkcgB8PqgN7Zg8KiwTMd4a TavQS8YHsXOtHFX93QD9G6NnyWSQvtvt1yVQSPJ0JF5CiEkFtzR59i1F3sUtxwQ= =DQxB -----END PGP SIGNATURE----- --=-a+QYe9mEPpn1ckpVoN+b--