diff mbox series

[bug#52075,3/4] gnu: Add python-notmuch2.

Message ID 163775787053.3163.3334445524894222634@localhost
State Accepted
Headers show
Series None | expand

Commit Message

Tanguy LE CARROUR Nov. 24, 2021, 12:44 p.m. UTC
Hi Nicolas,

Thanks for reviewing!

Quoting Nicolas Goaziou (2021-11-24 11:29:02)
> Tanguy Le Carrour <tanguy@bioneland.org> writes:
> 
> > * gnu/packages/mail.scm (python-notmuch2): New variable.
> 
> Thanks.
> 
> Would it be possible (or appropriate) to inherit from python-notmuch
> instead?

Possible? Yes! Thanks for suggesting.

```
Appropriate? I have no idea!

If this version is OK with you, I can submit a V2 of the patch set.

Regards,

Comments

Nicolas Goaziou Nov. 24, 2021, 1:21 p.m. UTC | #1
Tanguy LE CARROUR <tanguy@bioneland.org> writes:

>  (define-public python-notmuch2
>    (package
> +    (inherit python-notmuch)
>      (name "python-notmuch2")
> -    (version (package-version notmuch))
> -    ;; The bindings are distributed via the notmuch release tarball.
> -    (source (package-source notmuch))
> -    (build-system python-build-system)
> -    (inputs `(("notmuch" ,notmuch)))
>      (propagated-inputs `(("python-cffi" ,python-cffi)))
>      (arguments
>       `(#:phases
> @@ -1528,11 +1524,7 @@ (define-public python-notmuch2
>           ;; tree, so chdir into it before building.
>           (add-after 'unpack 'enter-python-dir
>             (lambda _ (chdir "bindings/python-cffi") #t)))))

You can drop the trailing #t above.

> -    (home-page (package-home-page notmuch))
>      (synopsis "Pythonic bindings for the notmuch mail database using CFFI")
> -    (description
> -     "This package provides Python bindings to use the Notmuch mail indexing
> -and search library.")
>      (license license:gpl3+)))
> ```
>
> Appropriate? I have no idea!
>
> If this version is OK with you, I can submit a V2 of the patch set.

It looks good, thanks. Please submit a v2 and I'll apply the patch set.

Regards,
Jack Hill Nov. 24, 2021, 4:53 p.m. UTC | #2
On Wed, 24 Nov 2021, Tanguy LE CARROUR wrote:

> Hi Nicolas,
>
> Thanks for reviewing!
>
> Quoting Nicolas Goaziou (2021-11-24 11:29:02)
>> Tanguy Le Carrour <tanguy@bioneland.org> writes:
>>
>>> * gnu/packages/mail.scm (python-notmuch2): New variable.
>>
>> Thanks.
>>
>> Would it be possible (or appropriate) to inherit from python-notmuch
>> instead?
>
> Possible? Yes! Thanks for suggesting.
>
> Appropriate? I have no idea!

As far as appropriate, do you know more about the origin of these 
"notmuch2" bindings? I know that at some point notmuch upstream re-wrote 
the binding using CFFI. Is that the origin of this package? If so, then I 
would be inclined to make this package independent of the existing 
python-notmuch package. If not, then please ignore this concern.

Thoughts?
Jack
Nicolas Goaziou Nov. 24, 2021, 5:11 p.m. UTC | #3
Hello,

Jack Hill <jackhill@jackhill.us> writes:

> As far as appropriate, do you know more about the origin of these
> "notmuch2" bindings? I know that at some point notmuch upstream
> re-wrote the binding using CFFI. Is that the origin of this package?
> If so, then I would be inclined to make this package independent of
> the existing python-notmuch package. If not, then please ignore this
> concern.

I do not grasp your concern. python-notmuch and python-notmuch2 share
most fields. Inheritance prevents duplication, that's all.

Why are you inclined to make this new package independant (in the sense
of Guix inheritance) of python-notmuch?

Regards,
Jack Hill Nov. 24, 2021, 5:41 p.m. UTC | #4
On Wed, 24 Nov 2021, Nicolas Goaziou wrote:

> Hello,
>
> Jack Hill <jackhill@jackhill.us> writes:
>
>> As far as appropriate, do you know more about the origin of these
>> "notmuch2" bindings? I know that at some point notmuch upstream
>> re-wrote the binding using CFFI. Is that the origin of this package?
>> If so, then I would be inclined to make this package independent of
>> the existing python-notmuch package. If not, then please ignore this
>> concern.
>
> I do not grasp your concern. python-notmuch and python-notmuch2 share
> most fields. Inheritance prevents duplication, that's all.
>
> Why are you inclined to make this new package independant (in the sense
> of Guix inheritance) of python-notmuch?

It is possible that my concern was misplaced. My thinking was that if the 
two bindings were logically separate projects, rather than being 
variations on the same code, we should keep them logically separate in 
Guix even if they are superficially similar. However, it might not be a 
logically separate project, and that might not be the right way to think 
about it anyway.

I don't have an objection to the version using inheritance. I am still 
curious in learning about the differences between the two packages if only 
for my own edification.

Hopefully that clarifies, and sorry for adding noise to this review.

Best,
Jack
Nicolas Goaziou Nov. 27, 2021, 9:15 a.m. UTC | #5
Hello,

Jack Hill <jackhill@jackhill.us> writes:

> It is possible that my concern was misplaced. My thinking was that if
> the two bindings were logically separate projects, rather than being 
> variations on the same code, we should keep them logically separate in
> Guix even if they are superficially similar. However, it might not be
> a logically separate project, and that might not be the right way to
> think about it anyway.

They are probably internally separate, but they do share superficial
parts. The Guix shortcut is only concerned about the latter, IMO.

> I don't have an objection to the version using inheritance.

I applied the patch set. Thanks to Tanguy for it, and to you for the
feedback!

> I am still curious in learning about the differences between the two
> packages if only for my own edification.

I cannot help here, unfortunately.

> Hopefully that clarifies, and sorry for adding noise to this review.

No problem! I think you raise an interesting question: does inheritance
in Guix imply strong internal dependency between the project inheriting
and the one being inherited?

Regards,
Tanguy LE CARROUR Nov. 28, 2021, 5:32 p.m. UTC | #6
Hi,


Quoting Nicolas Goaziou (2021-11-27 10:15:06)
> Jack Hill <jackhill@jackhill.us> writes:
> […]
> > I don't have an objection to the version using inheritance.
> 
> I applied the patch set. Thanks to Tanguy for it, and to you for the
> feedback!

Thanks!


> > I am still curious in learning about the differences between the two
> > packages if only for my own edification.
> 
> I cannot help here, unfortunately.

`python-notmuch2` contains the new binding that uses CFFI.
`python-notmuch` contains the "old" binding which, I guess, will be
deprecated at some point.

There's not much in the documentation or the mailing list about why they
decided to work on the new binding, though.


> > Hopefully that clarifies, and sorry for adding noise to this review.
> 
> No problem! I think you raise an interesting question: does inheritance
> in Guix imply strong internal dependency between the project inheriting
> and the one being inherited?

Actually, I have to admit that I have the same reserve about using
inheritance. But it's totally fine with me to use it as a mechanism
to reduce code duplication.

Regards,
diff mbox series

Patch

diff --git a/gnu/packages/mail.scm b/gnu/packages/mail.scm
index 1069aac16c..e1e199c656 100644
--- a/gnu/packages/mail.scm
+++ b/gnu/packages/mail.scm
@@ -1514,12 +1514,8 @@  (define-public python2-notmuch

 (define-public python-notmuch2
   (package
+    (inherit python-notmuch)
     (name "python-notmuch2")
-    (version (package-version notmuch))
-    ;; The bindings are distributed via the notmuch release tarball.
-    (source (package-source notmuch))
-    (build-system python-build-system)
-    (inputs `(("notmuch" ,notmuch)))
     (propagated-inputs `(("python-cffi" ,python-cffi)))
     (arguments
      `(#:phases
@@ -1528,11 +1524,7 @@  (define-public python-notmuch2
          ;; tree, so chdir into it before building.
          (add-after 'unpack 'enter-python-dir
            (lambda _ (chdir "bindings/python-cffi") #t)))))
-    (home-page (package-home-page notmuch))
     (synopsis "Pythonic bindings for the notmuch mail database using CFFI")
-    (description
-     "This package provides Python bindings to use the Notmuch mail indexing
-and search library.")
     (license license:gpl3+)))
```