On Tue, Mar 30, 2021 at 08:23:43AM +0200, Brendan Tildesley wrote:
> Me adding python-pyqt-builder here looks like a mistake. I left it
> there while experimenting.  When I was Upgrading SIP, I experimented
> with switching to the new 'sip-build' tool introduced in SIP 5. SIP
> 6's main change is to delete obsoleted code, so the old 'python
> configure.py' won't work. I never got it working so I just went back
> to the old way with SIP 5.  python-pyqt-builder is still needed by
> Calibre however.
> 
> The only other functional change in this patch is to remove the patch
> pyqt-public-sip.patch.  The introduction of python-pyqt5-sip as a
> separate module means means that with this patch, the module is not
> found at all. Removing it fixes it. I don't understand it in any great
> depth but it seems the patch may only really relevant to SIP 4.

Okay. I adjusted the commit message to match your revision.

> > On 03/30/2021 1:05 AM Leo Famulari <leo@famulari.name> wrote:
> > Also, there is a comment "Linking here means the sip module can be found
> > without python-pyqt5-sip needing to be added as an input". But,
> > python-pyqt5-sip is an input to this package. Can you explain what you
> > mean?
> 
> Upstream for whatever reason chose to move the sip module part out in
> to a separate source package python-pyqt5-sip. It's broken without
> removing the pyqt-public-sip.
> 
> The linking can be removed, but it would mean for every input that
> requires python-pyqt5 as an input, you also need to add
> python-pyqt5-sip so the sip module can be found. 

If I understand correctly, the issue that any package that uses
python-pyqt5 also needs to be able to find python-pyqt5-sip. Is that
right?

If so, it sounds like a case for propagated-inputs [0]. Concretely, I made
python-pyqt5-sip a propagated-input of python-pyqt and removed the
'pyqt5-sip' phase, and Calibre built successfully.

Does that seem like the right approach?

> The reason I added qtsvg was to try fix the Qt test. If you remove the
> line (setenv "SKIP_QT_BUILD_TEST" "true"), this test fails for
> multiple reasons.  One of them was qtsvg missing. Another was the
> get_exe_path bit. But a third reason I that its call to printtopdf in
> pyqtwebegine returns an empty string instaed of b'Skia/PDF'. I had no
> idea how to proceed with fixing that so I left it for now. But at
> least fixed the other errors. I assume some SVG related functionality
> will fail without it...

That's a good point. However, I checked if the built Calibre refers to
qtsvg, and it doesn't [1]. So, it's unlikely that Calibre will be able to
find and use qtsvg, regardless of whether or not it's an input. So, I'd
prefer to leave it out until we understand what it's for and how to make
sure that Calibre can use it.

> All good I think. My descriptions were much worse than I realised.

No worries. Writing the synopses and descriptions is a completely
different type of work from packaging or programming. I often "finish"
some packages, but need to go back later to write the descriptions. I'm
happy to finish these tasks as part of the code review process.

> python-cchardet differs from in python-chardet in that its not written
> /in/ python, but links to a fast C library to do it, but your
> description/synopsis changes make it look like its all in
> Python. Maybe make the description:
>  
> "cChardet is a character encoding detector, binding to the C
> library uchardet for speed." ?

Thanks, that helps. I amended the synopses and description based on
this.

I pushed my revisions of your updated branch, rebased on the current
master branch, to Savannah:

https://git.savannah.gnu.org/cgit/guix.git/log/?h=wip-update-calibre

[0]
https://guix.gnu.org/manual/en/html_node/package-Reference.html

[1] This command be used:
$ guix gc --references $(./pre-inst-env guix build --no-grafts calibre) | grep qtsvg