Re: branch master updated: gnu: Add passff.

Re: branch master updated: gnu: Add passff.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]


guix-commits@gnu.org writes:

> This is an automated email from the git hooks/post-receive script.
>
> snape pushed a commit to branch master
> in repository guix.
>
> The following commit(s) were added to refs/heads/master by this push:
>      new 6c894b7a1a gnu: Add passff.
> 6c894b7a1a is described below
>
> commit 6c894b7a1a6a7a0f7c51a44136bba00dc0b5250c
> Author: Clément Lassieur <clement@lassieur.org>
> AuthorDate: Tue Oct 17 16:53:57 2023 +0200
>
>     gnu: Add passff.
>
>     * gnu/packages/browser-extensions.scm (passff-host): New variable.
>     (passff): New variable.
>
>     Change-Id: I0f6f4b0c319e5cffd0940421a4d8bdc73d8d806b
> ---
>  gnu/packages/browser-extensions.scm | 76 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)

This passff-host package looks a bit odd to me, one thing to mention is
that guix show says it has no dependencies, but I don't think that's
correct:

  ./pre-inst-env guix show passff-host
  name: passff-host
  version: 1.2.3
  outputs:
  + out: everything
  systems: x86_64-linux mips64el-linux aarch64-linux powerpc64le-linux riscv64-linux
  + i686-linux armhf-linux i586-gnu powerpc-linux
  dependencies: 

Was this change sent as a patch to guix-patches?

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

Re: branch master updated: gnu: Add passff.

[Clément Lassieur]

On Sat, Oct 28 2023, Christopher Baines wrote:

> This passff-host package looks a bit odd to me, one thing to mention is
> that guix show says it has no dependencies, but I don't think that's
> correct:
>
>   ./pre-inst-env guix show passff-host
>   name: passff-host
>   version: 1.2.3
>   outputs:
>   + out: everything
>   systems: x86_64-linux mips64el-linux aarch64-linux powerpc64le-linux riscv64-linux
>   + i686-linux armhf-linux i586-gnu powerpc-linux
>   dependencies: 

I imagine it's a bug in `guix show`?  As doc says:

   • Gexps carry information about the packages or derivations they
     refer to, and these dependencies are automatically added as inputs
     to the build processes that use them.

> Was this change sent as a patch to guix-patches?

No it wasn't.

Re: branch master updated: gnu: Add passff.

[Kaelyn]

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]






------- Original Message -------
On Saturday, October 28th, 2023 at 8:05 AM, Clément Lassieur <clement@lassieur.org> wrote:


> 
> 
> On Sat, Oct 28 2023, Christopher Baines wrote:
> 
> > This passff-host package looks a bit odd to me, one thing to mention is
> > that guix show says it has no dependencies, but I don't think that's
> > correct:

I agree about this. When I packaged passff-host locally some time ago, I saw it has a runtime dependency on python and also needs to be able to find the pass binary. I've attached the bare (unfinished/unpolished) package definition extracted from my local channel and attached it, for if it is of assistance to folks. My definition tries to embed a sane path for finding pass with a default of the path to the password-store package it was built against, and also tries to copy the passff.json into the correct browser folder for Chromium, Firefox, and Icecat based on the packaged install_host_app.sh script (note: I have only used it with Icecat).

Cheers,
Kaelyn

> > 
> > ./pre-inst-env guix show passff-host
> > name: passff-host
> > version: 1.2.3
> > outputs:
> > + out: everything
> > systems: x86_64-linux mips64el-linux aarch64-linux powerpc64le-linux riscv64-linux
> > + i686-linux armhf-linux i586-gnu powerpc-linux
> > dependencies:
> 
> 
> I imagine it's a bug in `guix show`? As doc says:
> 
> • Gexps carry information about the packages or derivations they
> refer to, and these dependencies are automatically added as inputs
> to the build processes that use them.
> 
> > Was this change sent as a patch to guix-patches?
> 
> 
> No it wasn't.

I

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: passff-host.scm --]
[-- Type: text/x-scheme; name=passff-host.scm, Size: 3656 bytes --]

(define-public passff-host
  (package
    (name "passff-host")
    (version "1.2.3")
    (home-page "https://github.com/passff/passff-host/")
    (source
     (origin
       (method git-fetch)
       (uri (git-reference (url home-page)
                           (commit version)))
       (file-name (git-file-name name version))
       (sha256
        (base32
         "1p18l1jh20x4v8dj64z9qjlp96fxsl5h069iynxfpbkzj6hd74yl"))
       ))
    ;; NOTE: python-build-system is used instead of copy-build-system to
    ;; automatically pick up the Python 3 dependency and to wrap the installed
    ;; Python script.
    ;; FIXME: The passff.json in etc/ needs to go into a browser-dependent
    ;; location to work with that specific browser. How to install it to the
    ;; right location needs to be figured out and documented.
    (build-system python-build-system)
    (arguments
     (list #:tests? #f ; There are no tests
           #:phases
           #~(modify-phases %standard-phases
               (replace 'build
                 (lambda _
                   ;; TODO? Add password-store as an input and embed the store
                   ;; path to the "pass" executable.
                   (substitute* "src/passff.py"
                     (("_VERSIONHOLDER_") #$(package-version this-package))
                     ;; (("\"pass\"") (string-append
                     ;;                "\""
                     ;;                #$(this-package-input "password-store")
                     ;;                "/bin/pass\""))
                     (("\"PATH\": .*")
                      (string-join (list
                                    "\"PATH\""
                                    " \"/run/current-system/profile/bin"
                                    "/run/booted-system/profile/bin"
                                    (string-append
                                     #$(this-package-input "password-store")
                                     "/bin\",\n"))
                                   ":"))
                     )))
               (replace 'install
                 (lambda _
                   (let ((etc (string-append #$output "/etc"))
                         (libexec (string-append #$output "/libexec")))
                     ;; Install the host script in libexec
                     (install-file "src/passff.py" libexec)
                     ;; Insert the script path and install the (example)
                     ;; native host manifest to etc
                     (substitute* "src/passff.json"
                       (("PLACEHOLDER") (string-append libexec "/passff.py")))
                     (for-each
                      (lambda (dir)
                        (install-file "src/passff.json" dir))
                      (list etc ;; Generic location for easier access
                            ;; Chromium location based on src/install_host_app.sh
                            (string-append etc "/chromium/native-messaging-hosts")
                            ;; Firefox location based on src/install_host_app.sh
                            (string-append #$output "/lib/mozilla/native-messaging-hosts")
                            ;; Icecat location derived from the above Firefox location
                            (string-append #$output "/lib/icecat/native-messaging-hosts")))
                     )))
                 )))
    (inputs (list password-store))
    (synopsis "Host app for the WebExtension PassFF")
    (description
     "This piece of software wraps around the zx2c4 pass shell command. It has
to be installed for the PassFF browser extension to work properly.")
    (license li:gpl2)))

Re: branch master updated: gnu: Add passff.

[Clément Lassieur]

Hi Kaelyn,

On Sat, Oct 28 2023, Kaelyn wrote:

> I agree about this. When I packaged passff-host locally some time ago, I saw

I'm happy I'm not alone packaging Icecat extensions :)

> it has a runtime dependency on python and also needs to be able to find the
> pass binary.

My patch also has dependencies on python and password-store.  You can
have a look at documentation about G-Expressions to understand how.

> I've attached the bare (unfinished/unpolished) package definition
> extracted from my local channel and attached it, for if it is of assistance to
> folks.

It's a bit late for passff-host because it's already there (and it works
well), but if you need help for packaging other Icecat extensions, I'd
be glad to help.

> My definition tries to embed a sane path for finding pass with a
> default of the path to the password-store package it was built against, and
> also tries to copy the passff.json into the correct browser folder for

[...]

>     ;; FIXME: The passff.json in etc/ needs to go into a browser-dependent
>     ;; location to work with that specific browser. How to install it to the
>     ;; right location needs to be figured out and documented.

The Icecat browser-dependent location did not exist before
https://git.savannah.gnu.org/cgit/guix.git/commit/?id=25043e01b6cb6696ffdc6cdedd9cdf8235bb695c.

Now it is in /lib/icecat, e.g. /lib/icecat/native-messaging-hosts

[...]

>     ;; NOTE: python-build-system is used instead of copy-build-system to
>     ;; automatically pick up the Python 3 dependency and to wrap the installed
>     ;; Python script.

`trivial-build-system` is enough, most of the work is done by the
Makefile and install_host_app.sh.

Thanks,
Clément

Re: branch master updated: gnu: Add passff.

[Clément Lassieur]

On Mon, Oct 30 2023, Clément Lassieur wrote:

> Hi Kaelyn,
>
> On Sat, Oct 28 2023, Kaelyn wrote:
>
>> I agree about this. When I packaged passff-host locally some time ago, I saw

> I'm happy I'm not alone packaging Icecat extensions :)
>
>> it has a runtime dependency on python and also needs to be able to find the
>> pass binary.
>
> My patch also has dependencies on python and password-store.  You can
> have a look at documentation about G-Expressions to understand how.

It's indeed better if the inputs are explicit (see conversation with
John) so I amended my patch again to change a few things.  Now it has
fewer dependencies (password-store and python, as you rightfully said
above), and they all are explicit.

>> I've attached the bare (unfinished/unpolished) package definition
>> extracted from my local channel and attached it, for if it is of assistance to
>> folks.
>
> It's a bit late for passff-host because it's already there (and it works
> well), but if you need help for packaging other Icecat extensions, I'd
> be glad to help.
>
>> My definition tries to embed a sane path for finding pass with a
>> default of the path to the password-store package it was built against, and
>> also tries to copy the passff.json into the correct browser folder for
>
> [...]
>
>>     ;; FIXME: The passff.json in etc/ needs to go into a browser-dependent
>>     ;; location to work with that specific browser. How to install it to the
>>     ;; right location needs to be figured out and documented.
>
> The Icecat browser-dependent location did not exist before
> https://git.savannah.gnu.org/cgit/guix.git/commit/?id=25043e01b6cb6696ffdc6cdedd9cdf8235bb695c.
>
> Now it is in /lib/icecat, e.g. /lib/icecat/native-messaging-hosts
>
> [...]
>
>>     ;; NOTE: python-build-system is used instead of copy-build-system to
>>     ;; automatically pick up the Python 3 dependency and to wrap the installed
>>     ;; Python script.
>
> `trivial-build-system` is enough, most of the work is done by the
> Makefile and install_host_app.sh.

Well this was wrong :) See my updated patch where I use
copy-build-system and don't use Makefile and install_host_app.sh
anymore...

Thanks,
Clément

Re: branch master updated: gnu: Add passff.

[John Kehayias]

Hi Clément,

On Sat, Oct 28, 2023 at 05:05 PM, Clément Lassieur wrote:

> On Sat, Oct 28 2023, Christopher Baines wrote:
>
>> This passff-host package looks a bit odd to me, one thing to mention is
>> that guix show says it has no dependencies, but I don't think that's
>> correct:
>>
>>   ./pre-inst-env guix show passff-host
>>   name: passff-host
>>   version: 1.2.3
>>   outputs:
>>   + out: everything
>>   systems: x86_64-linux mips64el-linux aarch64-linux powerpc64le-linux riscv64-linux
>>   + i686-linux armhf-linux i586-gnu powerpc-linux
>>   dependencies:
>
> I imagine it's a bug in `guix show`?  As doc says:
>
>    • Gexps carry information about the packages or derivations they
>      refer to, and these dependencies are automatically added as inputs
>      to the build processes that use them.
>

Right, it uses gexps but I think here the better and more explicit
style would be to use inputs/native-inputs. Then instead of
referencing directly like #$<package-variable-name> use
#$(this-package-input "package-name") to get the store path. This I
think is clearer and I believe better allows for inheritance,
input-rewriting, and so on.

Feel free for anyone else to chime in on this point, I'm always
looking to learn to improve my own packaging and review, but this is
what I understand is preferred when possible.

>> Was this change sent as a patch to guix-patches?
>
> No it wasn't.

The mantra I've heard, and agree with, is that the
trivial-build-system is anything but trivial. Not saying it wasn't the
best choice here, or has anything to do with the above points, but
thought it worth mentioning for anyone else.

But this is also why I think it would have been better to have it go
through review. I see there's been several followup commits to improve
the style and fix things which also could have been avoided. Not a
huge deal perhaps, but I would err on the side of review for something
like this.

Of course, thanks for the contribution!

John

Re: branch master updated: gnu: Add passff.

[Clément Lassieur]

Hi John,

Thanks for taking the time to write this message.

On Tue, Oct 31 2023, John Kehayias wrote:

> Right, it uses gexps but I think here the better and more explicit
> style would be to use inputs/native-inputs. Then instead of
> referencing directly like #$<package-variable-name> use
> #$(this-package-input "package-name") to get the store path. This I
> think is clearer and I believe better allows for inheritance,
> input-rewriting, and so on.

You are right, I'll amend my patch, taking this into account.

I believe `guix lint` should say something about it though, maybe warn
when using an input that is not explicitly added in the `input` field.
Those errors are hard to catch and are in our code base at several
places already.

I've also noticed there is some subtle in-between style in our code
base: people adding x as input but still using #$x in the package
definition instead of using #$(this-package-input "x").  It's wrong
because in this case input rewriting does not work.

G-Expressions allow for weird things.  Here's another weird pattern from
our code base: going from (inputs `("foo", ,(origin ...))) to #$(origin
...)  without inputs.  This removes input rewriting too.

> Feel free for anyone else to chime in on this point, I'm always
> looking to learn to improve my own packaging and review, but this is
> what I understand is preferred when possible.
>
>>> Was this change sent as a patch to guix-patches?
>>
>> No it wasn't.
>
> The mantra I've heard, and agree with, is that the
> trivial-build-system is anything but trivial. Not saying it wasn't the
> best choice here, or has anything to do with the above points, but
> thought it worth mentioning for anyone else.

Agreed.

> But this is also why I think it would have been better to have it go
> through review. I see there's been several followup commits to improve
> the style and fix things which also could have been avoided. Not a
> huge deal perhaps, but I would err on the side of review for something
> like this.

I thought my patch was trivial, but I was obviously wrong since I
(indeed) amended it twice, and now a third time.

> Of course, thanks for the contribution!

Thanks for the kind words :)

Clément

Re: branch master updated: gnu: Add passff.

[Clément Lassieur]

See here for the patches.

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=66868

Thanks,
Clément

G-expressions and chroot environment? (was Re: branch master updated: gnu: Add passff.)

[Simon Tournier]

Hi,

On Sat, 28 Oct 2023 at 17:05, Clément Lassieur <clement@lassieur.org> wrote:

>>   ./pre-inst-env guix show passff-host
>>   name: passff-host
>>   version: 1.2.3
>>   outputs:
>>   + out: everything
>>   systems: x86_64-linux mips64el-linux aarch64-linux powerpc64le-linux riscv64-linux
>>   + i686-linux armhf-linux i586-gnu powerpc-linux
>>   dependencies: 
>
> I imagine it's a bug in `guix show`?

It is not a bug of ’guix show’ because ’guix show’ accesses to the
fields of the package record.  And Clément’s patch is an “abuse” of the
G-expressions machinery. :-)

>                                       As doc says:
>
>    • Gexps carry information about the packages or derivations they
>      refer to, and these dependencies are automatically added as inputs
>      to the build processes that use them.

Well, this is correct from my understanding. ;-)

However, there is still something that I am missing.  The derivations
tracks all and that’s expected; thanks G-expression machinery. :-)
However, I miss how the builder works with the chrooted environment if
nothing is passed to it.

The derivation reads,

--8<---------------cut here---------------start------------->8---
Derive
([("out","/gnu/store/0amanwyzx3jylyw7bz5nmszpybxll8ww-passff-host-1.2.3","","")]
 ,[("/gnu/store/070vbkzbs0dn6w9mhz0xw8fi5hfp92rg-make-4.3.drv",["out"])
   ,("/gnu/store/2i4781y3mmnm2jlx3awa4mwbqam2ar80-python-3.10.7.drv",["out"])
   ,("/gnu/store/ax7wdlbxhcz7w8nfyrxkb1pqai80niw6-sed-4.8.drv",["out"])
   ,("/gnu/store/gb247cil5nlnx175dhqmgg67q7ng7n2h-which-2.21.drv",["out"])
   ,("/gnu/store/ghwl0z5ci5sssbrzixxji8l0x3j9i3dv-bash-minimal-5.1.16.drv",["out"])
   ,("/gnu/store/mqmnsly3nm0a7hj46apf2hfm7j8wk56h-module-import-compiled.drv",["out"])
   ,("/gnu/store/raay3plnbzadwqc0yv8yw8pjr929pkqd-coreutils-9.1.drv",["out"])
   ,("/gnu/store/rnphhzpwkz82zf1il1cg52041myvp3d4-password-store-1.7.4.drv",["out"])
   ,("/gnu/store/y6871hl8lklcslvw57wj4bnyysxlv2np-passff-host-1.2.3-checkout.drv",["out"])
   ,("/gnu/store/y9l0jnyxssx1glbyg3cav78js2fm7j50-grep-3.8.drv",["out"])
   ,("/gnu/store/zraigp7miin3vzr5dcbr4i9rvds0i07r-guile-3.0.9.drv",["out"])]
 ,["/gnu/store/8nam67byqnpvbfn4anpgg5pb2qrqhs3v-passff-host-1.2.3-builder","/gnu/store/pj751v3199vmv6i6sf0szp185ryzcfdg-module-import"]
 ,"x86_64-linux","/gnu/store/g8p09w6r78hhkl2rv1747pcp9zbk6fxv-guile-3.0.9/bin/guile",["--no-auto-compile","-L","/gnu/store/pj751v3199vmv6i6sf0szp185ryzcfdg-module-import","-C","/gnu/store/2gbsk55kwag577skxwsxrfy3l4cl03xh-module-import-compiled","/gnu/store/8nam67byqnpvbfn4anpgg5pb2qrqhs3v-passff-host-1.2.3-builder"]
 ,[("out","/gnu/store/0amanwyzx3jylyw7bz5nmszpybxll8ww-passff-host-1.2.3")])
--8<---------------cut here---------------end--------------->8---

However the builder reads,

--8<---------------cut here---------------start------------->8---
(begin
  (define %build-inputs
    (quote
     (("source" . "/gnu/store/fjnkcv14qb61623lm16kq1mgb4bsxivl-passff-host-1.2.3-checkout"))))
  (define %outputs
    (list
     (cons "out"
           ((@
             (guile)
             getenv)
            "out"))))
  (define %output
    (assoc-ref %outputs "out"))
  (begin
    (use-modules
     (guix build utils))
    (setenv "PATH"
            (string-join
             (list "/gnu/store/yr39rh6wihd1wv6gzf7w4w687dwzf3vb-coreutils-9.1/bin" "/gnu/store/ixr7c3jadiqg640b8pz3njqhhm5zzmvj-grep-3.8/bin" "/gnu/store/sj794a2709pxsi4mgvi619qdpi1g32aa-password-store-1.7.4/bin" "/gnu/store/dy3xh053ahkhrp2jamggq8cpsyvp8mg0-python-3.10.7/bin" "/gnu/store/fyy3wkjkix16sb1ginqw2kbji74cwl2b-sed-4.8/bin" "/gnu/store/6vxk0i5j9w8mik4l6gx3cbw33f9x4l24-which-2.21/bin")
             ":"))
    (copy-recursively "/gnu/store/fjnkcv14qb61623lm16kq1mgb4bsxivl-passff-host-1.2.3-checkout" ".")
    (substitute* "src/install_host_app.sh"
      (("#!/usr/bin/env sh")
       "/gnu/store/9vw5slrffp27rzy2i2plnw7xfqjyk7m4-bash-minimal-5.1.16/bin/sh")
      (("(TARGET_DIR_FIREFOX=).*" all var)
       (string-append var
                      ((@
                        (guile)
                        getenv)
                       "out")
                      "/lib/icecat/native-messaging-hosts")))
    (invoke "/gnu/store/vq4g8390wbz2434m678v010mkpnkjb2w-make-4.3/bin/make"
            (string-append "VERSION=" "1.2.3")
            "install-unix")))
--8<---------------cut here---------------end--------------->8---

and I would have expect that it fails because of the isolated
environment.

How is it possible that the builder script is able to run that?

For another example using the gnu-build system.

--8<---------------cut here---------------start------------->8---
(define-module (appendix)
  #:use-module (guix packages)
  #:use-module (gnu packages base)
  #:use-module (guix utils)
  #:use-module (guix gexp)
  #:use-module (gnu packages emacs))

(define-public bye
  (package
    (inherit hello)
    (name "bye")
    (arguments
     (list
      #:phases
      #~(modify-phases %standard-phases
          (add-after 'install 'do-something-with-emacs
            (lambda _
              (invoke #$(file-append emacs-minimal
                                     "/bin/emacs") "--version"))))))))
--8<---------------cut here---------------end--------------->8---

Considering the builder of this, I miss how, in an isolated environment,
’gnu-build’ access to the store item ’emacs-minimal’ if it is not listed
in ’%build-inputs’.

Is it a bug?  Or someone could help me to understand what I miss with
the chrooted environment.

Cheers,
simon

Re: G-expressions and chroot environment? (was Re: branch master updated: gnu: Add passff.)

[Simon Tournier]

Hi,

On Fri, 03 Nov 2023 at 19:46, Simon Tournier <zimon.toutoune@gmail.com> wrote:

> However, there is still something that I am missing.  The derivations
> tracks all and that’s expected; thanks G-expression machinery. :-)
> However, I miss how the builder works with the chrooted environment if
> nothing is passed to it.

Thanks to chat with civodul, now all is clear. :-)

The isolated environment is populated using the list provided by the
derivation.

Somehow, the G-expression machinery tracks all for helping in
constructing the derivation, then the inputs of that derivation are put
inside the isolated environment where the builder Guile script runs.

Cool!

Cheers,
simon