Bug 772362 - Review Request: sigil - Free, Open Source WYSIWYG ebook editor
Summary: Review Request: sigil - Free, Open Source WYSIWYG ebook editor
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dan Horák
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 773313 783151
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-01-06 23:26 UTC by Hans de Goede
Modified: 2012-11-26 10:00 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-11-26 10:00:40 UTC
dan: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
diff between sigil and upstream libtidy (16.08 KB, text/plain)
2012-04-09 19:47 UTC, Dan Horák
no flags Details

Description Hans de Goede 2012-01-06 23:26:36 UTC
Spec URL: http://people.fedoraproject.org/~jwrdegoede/sigil.spec
SRPM URL: http://people.fedoraproject.org/~jwrdegoede/sigil-0.4.2-2.fc15.src.rpm
Description:
Sigil is a free, open source WYSIWYG ebook editor.
It is designed to edit books in ePub format.

Comment 1 Brendan Jones 2012-01-08 14:51:52 UTC
Hi Hans,

I'm just wondering whether flightcrew should be packaged on its own?

I found this link http://www.mobileread.com/forums/showthread.php?t=140946

Comment 2 Dan Horák 2012-01-08 15:43:01 UTC
Please see for my work on unbundling stuff in sigil - http://fedora.danny.cz/sigil/

Comment 3 Dan Horák 2012-01-08 15:54:33 UTC
ZipArchive needs unbundling, see URL above, bzip2 is not needed at all in the GPLed version of ZipArchive.

And please also see Debian effort in packaging Sigil at http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=590180

Comment 4 Brendan Jones 2012-01-08 20:05:58 UTC
Thanks Dan. 

Hans, Unbundling is line with Fedora policy unless we can find good reason to make an exception. http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

As it looks like part of the work has already done, I would recommend against seeking an exception. I leave it for the two of you to discuss how you'd like to proceed.

Comment 5 Hans de Goede 2012-01-09 08:46:39 UTC
Dan, Brendan,

I was not aware that FlightCrew and ZipArchive were separate standalone libs. I agree that separating them seems to be the right thing. I'll look into this all as time permits and submit reviews for standalone FlightCrew and ZipArchive packages when I've them + put a link to a new sigil package using them here.

Dan, I assume it is ok with you if I base my work of your work? Also I would not mind you submitting them for review, me reviewing them and then co-maintaining them in the future?

Regards,

Hans

Comment 6 Dan Horák 2012-01-11 13:07:06 UTC
Hans, it's completely fine if you base the packages on my work. But we still need a plan, because the patches are still not complete, my sigil patchset needs to be updated for the external FlightCrew and all patches submitted upstream. There is a good chance he will accept the patches after reading the answer to the Debian guys. And the main remaining issue is the modified libtidy, but IMHO the changes are not very intrusive and should be upstreamable. Another issue is packaging FlightCrew in such way that it will also provide XercesExtensions as a library. Any ideas are welcome :-)

So let's start with me submitting ZipArchive as a standalone package. Then do FlightCrew and finally Sigil itself.

And the dependency on xerces-3.1 in XercesExtensions (IIRC) limits Sigil to F>=16.

Comment 7 Hans de Goede 2012-01-18 16:01:31 UTC
(In reply to comment #6)
> Hans, it's completely fine if you base the packages on my work. But we still
> need a plan

Agreed we need a plan :)

, because the patches are still not complete, my sigil patchset
> needs to be updated for the external FlightCrew and all patches submitted
> upstream. There is a good chance he will accept the patches after reading the
> answer to the Debian guys. And the main remaining issue is the modified
> libtidy, but IMHO the changes are not very intrusive and should be
> upstreamable. Another issue is packaging FlightCrew in such way that it will
> also provide XercesExtensions as a library. Any ideas are welcome :-)
> 
> So let's start with me submitting ZipArchive as a standalone package. Then do
> FlightCrew and finally Sigil itself.

I've just submitted a ZipArchive package based on your work for review, see bug 
782823.

Here are the changes from your version:

* Thu Jan 12 2012 Hans de Goede <hdegoede@redhat.com> - 4.1.1-4
- Make -devel package Requires on main package include isa
- Drop buildroot and defattr boilerplate (no longer needed with recent rpm)
- Fix building with gcc-4.7
- Fix various rpmlint warnings

I'm now working on making the FlightCrew package provide both XercesExtensions and zipios as libraries, since both are duplicated in Sigil itself.

Comment 8 Hans de Goede 2012-01-19 14:42:28 UTC
Good news, I've got the entire sigil dep chain building, running and ready for review now using system libraries for everything except for tidylib.

If I've understood things correctly the embedded tidylib copy has some modifications which sigil needs, but which we should be able to get upstream. So my suggestion is that we move forward with an embedded tidylib copy for now, while working with tidylib upstream to get the changes upstream. Then once these
are upstream we can move to using the system version.

Here are all the review bugs:
ZipArchive, bug 773313
FlightCrew, bug 783151
sigil, this bug :)

Here is the version of sigil using a system version of FlightCrew and XercesExtensions (part of FlightCrew):

Spec URL: http://people.fedoraproject.org/~jwrdegoede/sigil.spec
SRPM URL:
http://people.fedoraproject.org/~jwrdegoede/sigil-0.4.2-3.fc15.src.rpm

Dan, note that I've respun all your patches for both FlightCrew and sigil itself, they added new lines in unix format to DOS format textfiles, making the resulting format mixed-format wrt their newline use. In 2 cases I've also done some small other fixes, see the commit messages inside the patches.

Also noteworthy is that I ended up not building zipios as a shared library as it is only used by FlightCrew I've left it embedded in libFlightCrew.so .

Comment 9 Mihai Limbășan 2012-01-21 17:40:33 UTC
Ping: Sigil 0.5.0 stable was just released.

Comment 10 Hans de Goede 2012-01-23 22:09:10 UTC
Hi,

Here is an updated package using Sigil 0.5.0, patched to use system versions of pcre and hunspell.

Spec URL: http://people.fedoraproject.org/~jwrdegoede/sigil.spec
SRPM URL:
http://people.fedoraproject.org/~jwrdegoede/sigil-0.5.0-1.fc15.src.rpm

Regards,

Hans

Comment 11 Hans de Goede 2012-01-24 08:07:52 UTC
I've all send all the patches for both Sigil and FlightCrew upstream and I just got a mail from upstreaming stating that they will integrate them over the next couple of days.

Comment 12 Dan Horák 2012-01-24 08:28:15 UTC
(In reply to comment #11)
> I've all send all the patches for both Sigil and FlightCrew upstream and I just
> got a mail from upstreaming stating that they will integrate them over the next
> couple of days.

That's a great news and thanks for finishing the work I've started. I will take a look on the remaining reviews later this week.

Comment 13 Hans de Goede 2012-02-01 08:06:34 UTC
Note, during some tests I found out that my sigil packages would not open some epubs I've tracked this down to a problem with ZipArchive and build a fixed ZipArchive package. So please download the latest ZipArchive from koji before running any tests.

Comment 14 Hans de Goede 2012-04-04 14:11:43 UTC
Dan,

I know you're probably busy with other stuff, still any chance you could finish this review?

Thanks,

Hans

Comment 15 Dan Horák 2012-04-09 19:47:51 UTC
Created attachment 576293 [details]
diff between sigil and upstream libtidy

Hans, I'm working on the review again, sorry for the delays.

These are the changes between sigil and upstream version of libtidy. The good thing is that it doesn't change ABI or API in an incompatible way.

Comment 16 Mikko Tiihonen 2012-07-16 17:51:04 UTC
With 0.5.3 sources and http://people.fedoraproject.org/~jwrdegoede/sigil.spec the build works nicely when all the patches are commented out (the patches had already been applied upstream).

Comment 17 LightDot 2012-09-11 15:27:20 UTC
With 0.5.3 sources, with dropped patches, rebuilt FlightCrew and ZipArchive, rebuilt Sigil works just fine on Fedora 16 (local mock build).

Has this review stopped due to embedded libtidy? What needs to be done to finish this up...?

Comment 18 Mihai Limbășan 2012-09-11 15:36:51 UTC
Please note that, siarting with commit 0fdb1bcf3a6b7d74e7041c3f8336b80a58893c59 (details at http://code.google.com/p/sigil/source/detail?r=0fdb1bcf3a6b7d74e7041c3f8336b80a58893c59 ) , sigil has dropped ZipArchive and has switched to minizip ( http://www.winimage.com/zLibDll/minizip.html ) for ZIP support. Assuming Sigil 0.6.0 is released in time for F18, ZipArchive can be dropped since Sigil is the only consumer, AFAIK.

Comment 19 Mihai Limbășan 2012-09-11 15:39:31 UTC
Additional maintainer notE: Upstream Sigil now bundles zlib 1.2.7 ( http://code.google.com/p/sigil/source/detail?r=89f6780e8e00b1ad6754c8780e2447069c3f638d ) and PCRE 8.31 ( http://code.google.com/p/sigil/source/detail?r=a800f78e373e7fe75ccb7efaeb8bea97cc2aa9dd ).

Comment 20 Hans de Goede 2012-09-12 10:07:16 UTC
Hi,

(In reply to comment #17)
> With 0.5.3 sources, with dropped patches, rebuilt FlightCrew and ZipArchive,
> rebuilt Sigil works just fine on Fedora 16 (local mock build).
> 
> Has this review stopped due to embedded libtidy?

That, and a serious case of -ENOTIME, if an interested Fedora packager want to take this over, please feel free to do so,

> What needs to be done to finish this up...?

A good question, Dan, what is your intent of comment #15, do you want sigil to move to the system libtidy? And if so, just use the system libtidy as is (could be troublesome), or try to get the sigil patches added (probably also trouble some since they seem somewhat ebook specific)...

Regards,

Hans

Comment 21 Dan Horák 2012-10-03 19:53:14 UTC
(In reply to comment #20)
> Hi,
> 
> (In reply to comment #17)
> > With 0.5.3 sources, with dropped patches, rebuilt FlightCrew and ZipArchive,
> > rebuilt Sigil works just fine on Fedora 16 (local mock build).
> > 
> > Has this review stopped due to embedded libtidy?
> 
> That, and a serious case of -ENOTIME, if an interested Fedora packager want
> to take this over, please feel free to do so,
> 
> > What needs to be done to finish this up...?
> 
> A good question, Dan, what is your intent of comment #15, do you want sigil
> to move to the system libtidy? And if so, just use the system libtidy as is
> (could be troublesome), or try to get the sigil patches added (probably also
> trouble some since they seem somewhat ebook specific)...

ideally the local changes to libtidy would be integrated to upstream libtidy or to the Fedora package, libtidy seems as not-so-alive project, so even having the local copy in sigil is fine with me

One good thing about sigil upstream is that it now has checks for all the bundled libraries in the cmake control file.

Comment 22 Hans de Goede 2012-10-08 09:20:37 UTC
(In reply to comment #15)
> Created attachment 576293 [details]
> diff between sigil and upstream libtidy
> 
> Hans, I'm working on the review again, sorry for the delays.
> 
> These are the changes between sigil and upstream version of libtidy. The
> good thing is that it doesn't change ABI or API in an incompatible way.

Thanks for this, I also cannot spot any ABI or API changes, but there are a lot of behavioral changes which clearly make the bundled version not compatible with the system version and which are likely undesirable to add to the system version (such as case insensitive attribute matching.

So as Sigil upstream already suggested in a past mails about bundling with them, although they support using system versions of all the other libs, they believe it is best to stick with a bundled libtidy and I agree with them. I'll go and file an fpc ticket for this.

Comment 23 Dan Horák 2012-10-08 09:29:28 UTC
btw latest sigil is packaged at http://fedora.danny.cz/danny/development/SRPMS/repoview/sigil.html

I've also tried to create a patch for usign system spelling dictionaries, but the issue (http://code.google.com/p/sigil/issues/detail?id=1646) was closed as WontFix, I have asked John for the reason in a private email, but no response yet.

Comment 24 Hans de Goede 2012-10-08 09:45:12 UTC
Bundling exception request is here:
https://fedorahosted.org/fpc/ticket/219

Comment 25 Dan Horák 2012-10-08 09:51:36 UTC
re tidy - it has a new life at http://w3c.github.com/tidy-html5/

Comment 26 Hans de Goede 2012-11-17 13:56:17 UTC
Ok, good news, we've gotten an exception for tidy from the fpc, so here is a new version:

Spec URL: http://people.fedoraproject.org/~jwrdegoede/sigil.spec
SRPM URL: http://people.fedoraproject.org/~jwrdegoede/sigil-0.6.0-1.fc15.src.rpm

Never mind the .fc15 I'm using the pre-git erra makefiles to build not yet imported rpms...

This is based on the srpm from comment #23. It is exactly the same except for the release field in the latest changelog entry being fixed :)

Dan, I would like to suggest that I keep the submitter role and you the reviewer role for the rest of this review, and then after the review we co-maintain ?

Comment 27 LightDot 2012-11-17 15:27:28 UTC
Great news!

Ok, I can spot a small packaging issue - sigil has a .desktop file so this applies:

http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files

Basically, there must be "BuildRequires: desktop-file-utils" and if the .desktop file isn't modified during the install, something along the lines of "desktop-file-validate %{buildroot}/%{_datadir}/applications/foo.desktop" in the check or install section.

Comment 28 Hans de Goede 2012-11-19 11:39:27 UTC
(In reply to comment #27)
> Great news!
> 
> Ok, I can spot a small packaging issue - sigil has a .desktop file so this
> applies:
> 
> http://fedoraproject.org/wiki/Packaging:Guidelines#Desktop_files
> 
> Basically, there must be "BuildRequires: desktop-file-utils" and if the
> .desktop file isn't modified during the install, something along the lines
> of "desktop-file-validate %{buildroot}/%{_datadir}/applications/foo.desktop"
> in the check or install section.

Good point, here is a new version fixing this:
Spec URL: http://people.fedoraproject.org/~jwrdegoede/sigil.spec
SRPM URL: http://people.fedoraproject.org/~jwrdegoede/sigil-0.6.0-2.fc15.src.rpm

Comment 29 Dan Horák 2012-11-25 11:02:53 UTC
formal review is here, see the notes explaining OK* and BAD statuses below:

OK      source files match upstream:
            541bc65c86158433adb2c5926e3ae43e46ed4fb6  Sigil-0.6.0-Code.zip
OK      package meets naming and versioning guidelines.
OK      specfile is properly named, is cleanly written and uses macros consistently.
OK      dist tag is present.
OK      license field matches the actual license.
OK      license is open source-compatible (GPLv3+). License text included in package.
OK      latest version is being packaged.
OK      BuildRequires are proper.
OK      compiler flags are appropriate.
OK      package builds in mock (Rawhide/x86_64).
OK      debuginfo package looks complete.
OK*     rpmlint is silent.
BAD     final provides and requires look sane.
N/A     %check is present and all tests pass.
OK      no shared libraries are added to the regular linker search paths.
OK      owns the directories it creates.
OK      doesn't own any directories it shouldn't.
OK      no duplicates in %files.
OK      file permissions are appropriate.
OK      correct scriptlets present.
OK      code, not content.
OK      documentation is small, so no -docs subpackage is necessary.
OK      %docs are not necessary for the proper functioning of the package.
OK      no headers.
OK      no pkgconfig files.
OK      no libtool .la droppings.
OK      GUI app with desktop file

- the spell checker doesn't like ebook and ePub, can be ignored
- Provides: bundled(libtidy) needed for bundled stuff is missing

APPROVED, please fix the missing Provides before import

And one more note - I would like to keep the condition for using the bundled PCRE as I will build the latest sigil packages for older Fedoras in my repo. For Fedora 16 and 17 we can provide sigil 0.5.3 as it doesn't depend on UTF-16 support in pcre.

Comment 30 Hans de Goede 2012-11-25 11:21:21 UTC
(In reply to comment #29)
> - Provides: bundled(libtidy) needed for bundled stuff is missing
> 
> APPROVED, please fix the missing Provides before import

Will do.

> And one more note - I would like to keep the condition for using the bundled
> PCRE as I will build the latest sigil packages for older Fedoras in my repo.
> For Fedora 16 and 17 we can provide sigil 0.5.3 as it doesn't depend on
> UTF-16 support in pcre.

I was actually planning on keeping the condition for the bundled PCRE, and simply
building with a bundled PCRE for F-16 and F-17, given that we've a fix for this
bundling in the latest release I don't see this as an issue. I'll hold of with building for
F-16 and F-17 till we've an agreement on how to deal with this.

Comment 31 Hans de Goede 2012-11-25 11:25:40 UTC
New Package SCM Request
=======================
Package Name: sigil
Short Description: Free, Open Source WYSIWYG ebook editor
Owners: jwrdegoede sharkcz
Branches: f16 f17 f18
InitialCC:

Comment 32 Gwyn Ciesla 2012-11-25 16:01:24 UTC
Git done (by process-git-requests).

Comment 33 Dan Horák 2012-11-25 16:05:07 UTC
(In reply to comment #30)
> (In reply to comment #29)
> > - Provides: bundled(libtidy) needed for bundled stuff is missing
> > 
> > APPROVED, please fix the missing Provides before import
> 
> Will do.
> 
> > And one more note - I would like to keep the condition for using the bundled
> > PCRE as I will build the latest sigil packages for older Fedoras in my repo.
> > For Fedora 16 and 17 we can provide sigil 0.5.3 as it doesn't depend on
> > UTF-16 support in pcre.
> 
> I was actually planning on keeping the condition for the bundled PCRE, and
> simply
> building with a bundled PCRE for F-16 and F-17, given that we've a fix for
> this
> bundling in the latest release I don't see this as an issue. I'll hold of
> with building for
> F-16 and F-17 till we've an agreement on how to deal with this.

well, your option will make my life easier, so I agree with your plan :-)

Comment 34 Hans de Goede 2012-11-26 10:00:40 UTC
Imported, build for f16+ and bodhi tickets created for f16+, closing.


Note You need to log in before you can comment on or make changes to this bug.