Bug 772362

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

Please see for my work on unbundling stuff in sigil - http://fedora.danny.cz/sigil/

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

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.

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

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.

(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> - 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.

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 .

Ping: Sigil 0.5.0 stable was just released.

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

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.

(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.

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.

Dan,

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

Thanks,

Hans

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.

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).

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...?

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.

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 ).

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

(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.

(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.

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.

Bundling exception request is here:
https://fedorahosted.org/fpc/ticket/219

re tidy - it has a new life at http://w3c.github.com/tidy-html5/

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 ?

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.

(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

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.

(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.

New Package SCM Request
=======================
Package Name: sigil
Short Description: Free, Open Source WYSIWYG ebook editor
Owners: jwrdegoede sharkcz
Branches: f16 f17 f18
InitialCC:

Git done (by process-git-requests).

(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 :-)

Imported, build for f16+ and bodhi tickets created for f16+, closing.