Bug 596461

Summary: Review Request: lzma-sdk - SDK for lzma compression
Product: [Fedora] Fedora Reporter: Gwyn Ciesla <gwync>
Component: Package ReviewAssignee: Jason Tibbitts <j>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: a.badger, bruno, bugzilla.acct, fedora-package-review, gilboad, notting, randyn3lrx, supercyper1, tcallawa
Target Milestone: ---Flags: j: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-11-30 00:41:35 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 564522, 662732    
Attachments:
Description Flags
Igor gave permission via email for LZMA-SDK to be used under LGPLv2 none

Description Gwyn Ciesla 2010-05-26 19:18:41 UTC
Description:

LZMA SDK provides the documentation, samples, header files, libraries,
and tools you need to develop applications that use LZMA compression.

LZMA is default and general compression method of 7z format
in 7-Zip compression program (www.7-zip.org). LZMA provides high
compression ratio and very fast decompression.

LZMA is an improved version of famous LZ77 compression algorithm. 
It was improved in way of maximum increasing of compression ratio,
keeping high decompression speed and low memory requirements for
decompressing.


SRPM: http://zanoni.jcomserv.net/fedora/lzma-sdk/lzma-sdk-4.6.5-1.fc12.src.rpm
SPEC: http://zanoni.jcomserv.net/fedora/lzma-sdk/lzma-sdk.spec

Packaged primarily for https://bugzilla.redhat.com/show_bug.cgi?id=564522

Does not build a library, merely provides the sdk to build against.  Not sure if I've packaged it entirely correctly, but I have upx building against it on my machine.  I know there are encoding issues on the docs, not sure how to fix them, as I'm already fixing others and these should be picked up also.

Comment 1 Chen Lei 2010-05-27 01:08:06 UTC
I don't known if it's necessrary, what's the opinion of FPC?

Bundling sources(normorlly BSD or MIT license) in GPL+ applications is quite common and is permitted in fedora, it may be impossible for all packages to split out their bundled sources. 

e.g.
Many Input methods bundles IMdkit.

Comment 2 Chen Lei 2010-05-27 08:47:10 UTC
gentoo seems to be allowing bundled lzma-sdk, as I known gentoo is quite strict in bundling system-libs.
See http://www.gentoo-portage.com/app-arch/upx-ucl

Comment 3 Bruno Wolff III 2010-05-27 12:28:12 UTC
I think it is better to package it once and have things pull source from it there, rather than bundle copies.

Comment 4 Chen Lei 2010-05-28 02:55:12 UTC
(In reply to comment #3)
> I think it is better to package it once and have things pull source from it
> there, rather than bundle copies.    

Can squashfs build agaist lzma465?

We now have one source package in repo - xorg-x11-server-source?

Comment 5 Bruno Wolff III 2010-05-28 04:30:28 UTC
Once the devel version of squashfs started building against the xz lib I didn't worry about it. I am still not in a hurry, as despite what the kernel newbies page says, lzma squashfs file systems are not supported by the upstream kernel yet.

Comment 6 Gwyn Ciesla 2010-06-01 15:13:23 UTC
(In reply to comment #1)
> I don't known if it's necessrary, what's the opinion of FPC?

Both Toshio and myself think it is, and we're on FPC.  There's not been a formal ruling, but that wouldn't be the process anyway, it would have to be granted an exception to the bundled library clause by FESCO.
 
> Bundling sources(normorlly BSD or MIT license) in GPL+ applications is quite
> common and is permitted in fedora, it may be impossible for all packages to
> split out their bundled sources. 

Not impossible, though possibly a gigantic pain. :)

> e.g.
> Many Input methods bundles IMdkit.    

Does that make it the right think to do?

Re squashfs, I have no idea.

Comment 7 Bruno Wolff III 2010-06-01 15:38:38 UTC
At one time the development version of squashfs-tools needed the LZMA SDK, but Phillip added an enhancement so that it could also use the xz library. I tested that and it works. So LZMA support in squashfs is not dependent on the LZMA SDK. (We just need support upstream in the kernel.)

Comment 8 Gwyn Ciesla 2010-06-02 12:06:55 UTC
So I just need this for upx then.

Comment 9 Chen Lei 2010-06-02 12:14:54 UTC
(In reply to comment #8)
> So I just need this for upx then.    

You may just need those files in the lzma source.


lzma465/C/LzFind.c 
lzma465/C/LzFind.h
lzma465/C/LzmaDec.c
lzma465/C/LzmaDec.h
lzma465/C/LzmaEnc.c
lzma465/C/LzmaEnc.h
lzma465/C/Types.h

Comment 10 Gwyn Ciesla 2010-06-02 17:49:11 UTC
Possible, but since I don't know how to make that determination, and this package might be of use to future packages, I see no reason not to do the whole shebang.

Comment 11 Bruno Wolff III 2010-06-02 18:30:11 UTC
I took a quick look at the spec file and have a couple of questions.

The .exe file is removed in %prep. Might that need to be removed sooner so that it doesn't get included in the srpm? (I'm worried that some nonredistributable library might be included in that build.)

Would there be any point to having a way to get just the header files and documentation, but not the c source? (Maybe the main package provides the header files and documenation, and a -source subpackage provides the rest?)

Comment 12 Gwyn Ciesla 2010-06-02 18:47:54 UTC
A. Sure.  I can make a modified tarball.

B. Why not the source in main and the headers in -devel?

Comment 13 Chen Lei 2010-06-03 10:13:59 UTC
(In reply to comment #10)
> Possible, but since I don't know how to make that determination, and this
> package might be of use to future packages, I see no reason not to do the whole
> shebang.    



I think packaging lzma-sdk is a bit complicated and may need some time to consider how to do is the best. Also, considring lzma 4.x is dead in upstream, it'll be must appropriate to package lzma 9.x. Once, upx support lzma 9.x, we can switch from bundled lzma to lzma source in the repo. I think bundling a dead upstream source is harmless. 

Since you already ship lzma in upx 3.04, I suggest you also ship lzma 4.65 in upx 3.05 as well temporary, this won't introduce new bugs. Considering upx 3.05 is a pure bugfix release, updating to the latest version will fix some bugs in upx 3.04

Comment 14 Gwyn Ciesla 2010-06-03 13:30:38 UTC
What's complicated about it?  Are there specific issues not addressed by the package as is?  I see no reason not to go ahead with it and stop bundling it in upx.

WRT 9.x, I agree, but I don't think waiting for that is the appropriate solution.

Comment 15 Chen Lei 2010-06-05 13:47:57 UTC
(In reply to comment #14)
> upx.
> WRT 9.x, I agree, but I don't think waiting for that is the appropriate
> solution.    

Bunding lzma-sdk is not a bug, because lzma-sdk is designed for bundling(MS Windows philosophy). p7zip also bundles the whole lzma-sdk in all linux distribution, no one complains about this.


Packaging lzma-sdk seems not very easy, the tatball is not designed for packaging.

See http://packages.debian.org/source/sid/lzma

Comment 16 Bruno Wolff III 2010-06-05 14:27:16 UTC
I think claiming bundling it isn't a bug because the people wrote it anticipated it being used that way, is not valid reasoning for Fedora.
The problems caused by bundling (which is why there is a rule), are still going to apply.
If p7zip is doing that, then it is a problem that should be addressed, not copied.

To move forward on this, it might make sense to bring this up at a packagers meeting and see what other people think about the proper way to package that provides source to be included by other packages. If there is agreement on how to do it, we can get this reviewed and included. And then later start cleaning up other packages.

It might be useful to have the p7zip owner at that meeting to get their take on things.

Comment 17 Chen Lei 2010-06-05 14:45:42 UTC
(In reply to comment #16)
> I think claiming bundling it isn't a bug because the people wrote it
> anticipated it being used that way, is not valid reasoning for Fedora.
> The problems caused by bundling (which is why there is a rule), are still going
> to apply.
> If p7zip is doing that, then it is a problem that should be addressed, not
> copied.
> To move forward on this, it might make sense to bring this up at a packagers
> meeting and see what other people think about the proper way to package that
> provides source to be included by other packages. If there is agreement on how
> to do it, we can get this reviewed and included. And then later start cleaning
> up other packages.
> It might be useful to have the p7zip owner at that meeting to get their take on
> things.    

p7zip now requires lzma 9.1, but upx still requires lzma 4.6, so the problem is a bit complicated. Maybe writing a wiki documents all packages bundling lzma-sdk is more easier.

Do you mind to open a ticket on fesco about bundling source file issue? Currenly, packaging guideline only forbid bundling system libs.

Comment 18 Bruno Wolff III 2010-06-05 14:55:50 UTC
Shouldn't the packaging committee discuss this first?

Comment 19 Gwyn Ciesla 2010-06-29 13:42:14 UTC
If you want an exception for UPX, go to FESCO.  If you want to make sure lzma-sdk is being packaged properly, that's for FPC.  

Personally, I think packaging lzma-sdk so we can quit bundling is the way to go, not just for UPX.  I'm not sure when the next FPC meeting is, but I'm not sure why we can't move forward on this as is.

Comment 20 Gwyn Ciesla 2010-08-12 20:04:45 UTC
Interesting thing.  Just saw put 2 and 2 together, and did some digging.  

https://admin.fedoraproject.org/pkgdb/acls/name/SevenZip

This is recently orphaned, and uses the same tarball I use in lzma-sdk.  It only ships the Java bits.

Comment 21 Gwyn Ciesla 2010-08-12 20:14:38 UTC
Only Frinika uses it, also orphaned.  I could take both, convert SevenZip to provide everything in a way that Frinika can use, and use it for UPX also, and drop this review.  Thoughts?

Comment 22 Toshio Ernie Kuratomi 2010-08-14 21:07:59 UTC
I think I'd rename SevenZip to lzma-sdk if you do that, though.  It's also likely a good idea to have it rereviewed since it's providing more features.  Having all of this built from a single package sounds like a great idea though.

Comment 23 Chen Lei 2010-08-15 02:54:44 UTC
(In reply to comment #22)
> I think I'd rename SevenZip to lzma-sdk if you do that, though.  It's also
> likely a good idea to have it rereviewed since it's providing more features. 
> Having all of this built from a single package sounds like a great idea though.

Do you have a suggestion about how to package a sdk like lzma?
It seems it can't be packaged as a static libraries, most developers only copy a few source files from sdk. Currently, at least p7zip and upx need lzma-sdk, however they use two incompatible versions(p7zip use lzma 9.x, upx use lzma 4.x).

Comment 24 Toshio Ernie Kuratomi 2010-08-16 21:37:11 UTC
From a security and other bug tracking point of view, static libraries and copying source files from the sdk seem to be the same to me unless the source is being modified.  So question #1: Is the source being modified?

As for needing different versions, this is the same as having parallel stacks for any library.  We'd really like for everything to be using the same version but it's not disallowed to have a backwards compatible stack as well.  We need to be careful to get any bugfixes, esp. security related applied to the backwards compat stack.

Comment 25 Chen Lei 2010-08-17 02:13:35 UTC
(In reply to comment #24)
> From a security and other bug tracking point of view, static libraries and
> copying source files from the sdk seem to be the same to me unless the source
> is being modified.  So question #1: Is the source being modified?
> 
Thanks, so we just package source files to some place in %{_datadir}? Actually, lzma 4.x is dead and won't have any bugfix.

Comment 26 Gwyn Ciesla 2010-08-20 12:59:47 UTC
I think what I've done here, and how I use if with UPX, works well.  I put the files in %{_datadir}, then have UPX BR it and point the build at it, and it works.

Toshio, if you don't object, I'll go ahead with SevenZip then.

Comment 27 Toshio Ernie Kuratomi 2010-08-20 14:47:10 UTC
No objection to merging the packages.  Using the lzma-sdk name and continuing the review here seems like a good idea.

Comment 28 Gwyn Ciesla 2010-08-23 15:58:46 UTC
If I'm re-reviewing, I don't see the point in resuscitating the other package, since the only thing that needed it, Frinika, is also orphaned.  Also, in that case, I don't see the point in adapting this package to work with Frinika, unless someone plans to unorphan it.  I don't.  If someone wants to and wants me to create the appropriate -java subpackage, I can, but mine includes the java bits anyway.

So, I think this is ready to be reviewed as-is.

Comment 29 Chen Lei 2010-09-05 03:14:11 UTC
Firefox 4.0 also includes a copy of lzma sdk 9.07.

Comment 30 Jason Tibbitts 2010-11-18 14:56:24 UTC
This ticket is somewhat confusing.  First, there's already a package named "lzma" in the distribution, so either this is a duplicate or the ticket summary is incorrect.  Second, there's been 29 comments since the original src.rpm link was posted; is that really what should be reviewed?

Comment 31 Gwyn Ciesla 2010-11-18 15:09:21 UTC
This is a different piece of code than the lzma package.  There have been 30 comments, but no changes made, so the original src.rpm should be valid.  MOst of the comments have been existential debate about the nature of the package, not the package itself.

Comment 32 Jason Tibbitts 2010-11-18 15:16:25 UTC
So this is intended to replace the existing lzma package?

Comment 33 Gwyn Ciesla 2010-11-18 15:24:52 UTC
Not at all.  It's intended to exist solely to allow building of upx without bundling this code.

Comment 34 Jason Tibbitts 2010-11-18 15:31:33 UTC
And therein lies my confusion.  We already have a package named "lzma"; we can't have another.

Comment 35 Gwyn Ciesla 2010-11-18 15:35:27 UTC
I just now understood the source of your confusion.  It should be lzma-SDK, not lzma - SDK.

Comment 36 Jason Tibbitts 2010-11-25 03:01:51 UTC
Now, for another dumb question: are the later sdk versions (920 seems to be current) something you'd also want to package?  I guess the API changes with each version and packages need a specific version to build against, but I don't quite understand why version 465 gets to _the_ lzma-sdk package instead of being called lzma465-sdk or something.

Otherwise I can't really argue with this package; of course rpmlint hates it because the package name doesn't end in -devel, but I do think this is at least better than bundling the actual source in various packages and fundamentally it isn't really any different than a static library.

Comment 37 Chen Lei 2010-11-25 03:16:37 UTC
Actually,we can retire lzma packages in fedora since it was already replaced by xz by a long time and retired in RHEL 6.

It also seems current package guideline start to allow bundling copylibs like lzma sdk:
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Copylibs

Comment 38 Gwyn Ciesla 2010-11-25 04:46:36 UTC
@Jason:  I suppose we could go with lzma465-sdk, to allow the 920, etc, if there's a need, or keep it this way and call the next one lzma920-sdk, if it's packaged.  Given Chen's comments WRT xz, I doubt it will be needed.

@Chen: Yes, but as I've stated before, not for the purpose that this package is intended.  I can't build upx against anything xz provides, ergo this package is required.  I don't think Copylibs applies here, since it's not modified.

Comment 39 Jason Tibbitts 2011-01-27 19:09:51 UTC
As much as I think this kind of thing is distasteful, I can't think of a better way to avoid the bundling issue while still allowing upx in the distribution.  Not that I really see the point of upx, but that's not up to me.

Not sure if this submission is sufficiently old that it predates the new unnecessity of buildroot, but just to be sure: if you don't want to build this for el4 or el5, you can remove BuildRoot:, %clean and the first line of %install.

Is there any file in this package that should remain executable?  If not, perhaps simply all of the find calls with just
  find . -type f -exec chmod -x {} \;
or whatever your preferred variant is?

And while I'm in %prep:
Why not use %setup -c instead of doing everything manually?  Seems that would simply things a good bit.

There are a few line endings that need encoding:
  lzma-sdk.noarch: W: wrong-file-end-of-line-encoding
   /usr/share/doc/lzma-sdk-4.6.5/7zFormat.txt
   /usr/share/doc/lzma-sdk-4.6.5/7zC.txt
   /usr/share/doc/lzma-sdk-4.6.5/history.txt
   /usr/share/doc/lzma-sdk-4.6.5/lzma.txt
   /usr/share/doc/lzma-sdk-4.6.5/Methods.txt
   /usr/share/doc/lzma-sdk-4.6.5/7zFormat.txt
   /usr/share/doc/lzma-sdk-4.6.5/7zC.txt
   /usr/share/doc/lzma-sdk-4.6.5/history.txt
   /usr/share/doc/lzma-sdk-4.6.5/lzma.txt
   /usr/share/doc/lzma-sdk-4.6.5/Methods.txt

I really hate thinking about this kind of thing, but any time you see "Public Domain" you have to worry.  I've asked the legal list if this is properly public domain.

I think the perl(SevenZip) provide is bogus.  I can't imagine where it's coming from, unless rpm is on enough crack to parse
  package SevenZip;
out of Java/SevenZip/CRC.java or Java/SevenZip/LzmaAlone.java.  Actually, I think that is where it comes from.  Needs to be filtered in any case.

* source files match upstream.  sha256sum:
  c935fd04dd8e0e8c688a3078f3675d699679a90be81c12686837e0880aa0fa1e
   lzma465.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
? license field matches the actual license.
* BuildRequires are proper (none).
* package builds in mock (rawhide, x86_64).
* package installs properly.
X rpmlint has valid complaints.
X final provides and requires:
X  perl(SevenZip)  
   lzma-sdk = 4.6.5-1.fc15
  =
   (no requires)

* no bundled libraries.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.

Comment 40 Jason Tibbitts 2011-01-27 19:24:23 UTC
And it's looking like the answer is no, this is not public domain software.  The effects of this are unpleasant.  I think at this point spot is trying to get in contact with the author to get a permissive license placed on the code.

Comment 41 Gwyn Ciesla 2011-01-27 19:49:34 UTC
Bah.  Thank you.  Let me know the outcome.  If that fails, I'll just have to withdraw this and pull upx.  And then probably ucl, since upx is the only thing using it. . .

Comment 42 Thom Carlin 2011-02-19 13:07:49 UTC
Jason, any word on licensing?

Comment 43 Jason Tibbitts 2011-02-19 17:17:36 UTC
Any word on the licensing wouldn't come from me, sorry.  I should have blocked FE-Legal, though.

Comment 44 Gwyn Ciesla 2011-03-14 19:49:46 UTC
Adding Sport to CC list in search of an update.

Comment 45 Tom "spot" Callaway 2011-04-04 19:18:22 UTC
Sent another email to Igor Pavlov today, hopefully I'll get a response this time.

Comment 46 Gwyn Ciesla 2011-04-04 19:27:23 UTC
Awesome, thanks!

Comment 47 Tom "spot" Callaway 2011-04-05 17:31:16 UTC
Created attachment 490051 [details]
Igor gave permission via email for LZMA-SDK to be used under LGPLv2

Comment 48 Tom "spot" Callaway 2011-04-05 17:32:21 UTC
Lifting FE-Legal, as LGPLv2 is an acceptable license. Please be sure to package a copy of the email record of the licensing permission in the package as a %doc.

Comment 50 Jason Tibbitts 2011-04-08 20:14:47 UTC
It doesn't appear that all of the issues were addressed.  Specifically, this is still providing perl(SevenZip).

Comment 52 Jason Tibbitts 2011-04-12 19:20:19 UTC
Hmm, I'm not seeing any change in that perl(SevenZip) thing, but I'm not sure I understand why.  The macro usage seems proper but it doesn't seem to actually filter anything.  I used the rpm 4.9 filtering macro and it works fine:
  %define __provides_exclude .*SevenZip.*
but it looks like you intend to support old releases with this spec so that won't work generally.

I'm also still seeing some files that need EOL conversion:

lzma-sdk.noarch: W: wrong-file-end-of-line-encoding
  /usr/share/doc/lzma-sdk-4.6.5/7zFormat.txt
lzma-sdk.noarch: W: wrong-file-end-of-line-encoding
  /usr/share/doc/lzma-sdk-4.6.5/7zC.txt
lzma-sdk.noarch: W: wrong-file-end-of-line-encoding
  /usr/share/doc/lzma-sdk-4.6.5/history.txt
lzma-sdk.noarch: W: wrong-file-end-of-line-encoding
  /usr/share/doc/lzma-sdk-4.6.5/lzma.txt
lzma-sdk.noarch: W: wrong-file-end-of-line-encoding
  /usr/share/doc/lzma-sdk-4.6.5/Methods.txt

Finally, the spec seems to have grown a mixed-use-of-spaces-and-tabs thing (tab on the Source1: line) which isn't a particularly big deal but I guess you might want to fix it if you're in there.

Comment 53 Gwyn Ciesla 2011-04-27 14:44:17 UTC
SRPM: http://zanoni.jcomserv.net/fedora/lzma-sdk/lzma-sdk-4.6.5-4.fc14.src.rpm
SPEC: http://zanoni.jcomserv.net/fedora/lzma-sdk/lzma-sdk.spec


All done but the EOL files.  They're already getting corrected in %prep, and the versions installed in /u/s/lzma-sdk are fine, but the ones in %doc aren't.  What am I missing, other than that I could probably remove tthe /u/s ones?

Comment 54 Tom "spot" Callaway 2011-08-09 17:00:22 UTC
This package has a unique layout, it isn't divided by base/devel like most of Fedora's packages, because this code isn't actually built, it's just dropped in /usr/share.

Is the idea that other packages will BuildRequires: this SDK package, then build the SDK code directly into their applications? If so, I'm a bit uncomfortable with that approach, since it is effectively bundling copies of the lzma-SDK into other packages.

Especially since we already seem to have more recent (and compiled) versions of this code in the "lzma" package.

If this is just an issue of the older lzma being needed because of incompatibilities with the newer "lzma" package, I'd rather see this be a compat library package.

Comment 55 Jason Tibbitts 2011-08-09 17:08:03 UTC
I had completely forgotten about this.

From what I gather, the idea is to simply "work around" our rules against bundling by sticking the bundled source out into a separate package.  It still gets compiled as source by the build system of the consuming package.

I think this is marginally better than simply bundling, because (assuming multiple consumers) there's one obvious place where bugs should be fixed and it can be determined what needs to be rebuilt.  Honestly I don't see it as functionally any different than a static library in that respect.

Comment 56 Gwyn Ciesla 2011-08-09 17:12:47 UTC
Yes, and those don't work with UPX, which is the whole problem.  This was suggested as a way to sort of bundle but in one place so we could find and rebuild packages using it.  I agree, it's ugly as sin, but unless there's a better idea, it's this, it's remove LZMA support from UPX, or it's remove UPX from Fedora.

I hate this.  I really do.  But I don't see another way forward.

Comment 57 Tom "spot" Callaway 2011-08-09 17:16:03 UTC
Is UPX using the C interface? I'd still rather see it link to a library, with headers in a -devel.

Comment 58 Gwyn Ciesla 2011-08-09 17:31:05 UTC
C and C++.  I'd like that too.  They don't seem interested, and my C-fu is insufficent to make it so, though if someone could point me in the right general direction I'd take a shot at it.

Comment 59 Tom "spot" Callaway 2011-08-09 19:47:19 UTC
Is upx the only consumer of this in Fedora?

Comment 60 Tom "spot" Callaway 2011-08-09 20:04:48 UTC
So, the reason I ask that question in Comment 59 is because I didn't export "C" all of the functions in the lzma-sdk library, just the ones that upx needs. If anything else wants to use other functions in the lzma-sdk from C++ code, those functions will need the same magic applied. Should be straightforward from the patch though.

Here's the new SRPM and SPEC I worked up:

New SRPM: http://spot.fedorapeople.org/lzma-sdk-4.6.5-5.fc15.src.rpm
New SPEC: http://spot.fedorapeople.org/lzma-sdk.spec

Here are the bits to fix up upx to use it:

http://spot.fedorapeople.org/upx-3.07-use-lzma-sdk-lib.patch
http://spot.fedorapeople.org/upx.spec

I smoke tested these changes and upx successfully compressed an ELF binary and it ran afterwards.

As ugly as this might be, I like this package a _LOT_ better. Unfortunately, given that I basically rewrote this package, I don't think it would be fair for me to review it. I will, however, offer to comaintain it with you (lzma-sdk, not upx).

Comment 61 Jason Tibbitts 2011-08-11 15:54:12 UTC
Note that I'm still on to review this, assuming that Jon agrees with what you've done and wants to proceed.  Just let me know.

Comment 62 Thom Carlin 2011-08-16 12:36:48 UTC
Asked Jon in 564522.

Comment 63 Gwyn Ciesla 2011-08-17 01:50:53 UTC
Sorry for the delay, stuff happened. :/

Very nice, thank you Spot!   Consider this blessed by me and something I'll maintain.  Tibbs, you may do the honors, if you're still willing.

Our long national nightmare, etc, etc.

My favourite part: "- rework package to be more normal"  Awesomely understated. :)

Comment 64 Thom Carlin 2011-10-01 10:20:26 UTC
Hi Jason, are you still willing to review this?

Comment 65 Gwyn Ciesla 2011-11-28 18:36:13 UTC
Tibbs, ping? :)

Comment 66 Jason Tibbitts 2011-11-28 19:52:59 UTC
Damn it, I'm sorry.  I saw some similar package go by and thought someone had simply taken this over.

I have to run home soon but I will carve out some time tonight to finish this off.

Comment 67 Gwyn Ciesla 2011-11-28 20:01:46 UTC
No worries, life happens.  BTW, I've relocated the files:

SRPM: http://fedorapeople.org/~limb/review/lzma-sdk/lzma-sdk-4.6.5-5.fc15.src.rpm
SPEC: http://fedorapeople.org/~limb/review/lzma-sdk/lzma-sdk.spec

Comment 68 Jason Tibbitts 2011-11-29 18:36:19 UTC
OK, I've already run my checklist so this is about looking for new stuff.

The license issues have been dealt with, and the clarification is included in the package.

The dependency issue was fixed.

All of the previous rpmlint warnings went away.  However, there are some new ones:

lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 __cxa_pure_virtual
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 vtable for __cxxabiv1::__si_class_type_info
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 vtable for __cxxabiv1::__class_type_info
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 vtable for __cxxabiv1::__vmi_class_type_info
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 typeinfo for char const*
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 typeinfo for int
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 __gxx_personality_v0
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 operator delete(void*)
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 operator new[](unsigned long)
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 __cxa_end_catch
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 __cxa_allocate_exception
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 operator delete[](void*)
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 __cxa_begin_catch
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 __cxa_throw
lzma-sdk.x86_64: W: undefined-non-weak-symbol
  /usr/lib64/liblzmasdk.so.4.6.5 operator new(unsigned long)

I have to admit I don't understand these, especially the "typeinfo for *" ones.  I'm guessing this needs a link against come C++ standard library, but it's not clear to me why that didn't happen.  Maybe gcc doesn't do that by default unless called as g++?  makefile.gcc does specify g++ as the compiler but the make call in %build specifically overrides that.

lzma-sdk.x86_64: W: unused-direct-shlib-dependency
  /usr/lib64/liblzmasdk.so.4.6.5 linux-vdso.so.1
lzma-sdk.x86_64: W: unused-direct-shlib-dependency
  /usr/lib64/liblzmasdk.so.4.6.5 /lib64/libm.so.6
The first is odd.  The second isn't a big deal, I guess; the makefile explicitly specifies -lm on the link line.

I'd say everything's OK here except for that bit about gcc versus g++.  Any ideas there?

Comment 69 Gwyn Ciesla 2011-11-29 18:57:04 UTC
I can say with authority that I'm as clueless as you are here.  Does anyone on this BZ's CC list have any thoughts?

Comment 70 Jason Tibbitts 2011-11-29 19:09:57 UTC
Well, changing gcc to g++ on the make line results in a package that still builds, and all of those undefined-non-weak-symbol things go away.  The unused-direct-shlib-dependency things are still there, but I don't think they illustrate any problem.

No idea at all if the resulting library still works, though.

Comment 71 Gwyn Ciesla 2011-11-29 19:14:09 UTC
I'm not seeing these rpmlint errors.  I'm using rpmlint -i on the RPMS and SRPMS on f16, what are you using?

Comment 72 Jason Tibbitts 2011-11-29 19:21:48 UTC
You must install the package and then run rpmlint lzma-sdk.  My build scripts install the package into the mock chroot for this purpose.

Comment 73 Gwyn Ciesla 2011-11-29 19:35:49 UTC
Interesting, I was not aware of this rpmlint functionality.  I'm curious why that would be different from rpmlint -i <foo>.rpm, but that's neither here nor there.

Googling for a minute leaves me no more sure about this than I was previously.  I'll try it with g++ and see if upx still uses the lzma functionality.

Comment 74 Gwyn Ciesla 2011-11-29 19:47:16 UTC
I changed the two gcc on line 74 to g++ and I still see those errors.  Did you change something else?

Comment 75 Jason Tibbitts 2011-11-29 19:53:29 UTC
Well, obviously you'd only want to change the first one, since CXX_C is supposed to be the C compiler according to what's in the makefile:

make -f makefile.gcc clean all CXX="g++ %{optflags} -fPIC" CXX_C="gcc %{optflags} -fPIC"


But it did appear to work for me; calls the the compiler in the build log changed to g++, the undefined-non-weak-symbol warnings did go away, and inspection reveals that the c++ standard library did get linked in.  I'm building in mock for rawhide.

Comment 76 Gwyn Ciesla 2011-11-29 20:40:39 UTC
Jackpot, that did it.  It builds, no u-n-w-s errors, upx builds, upx compresses with lzma, resulting binary still runs. :)

SRPM:
http://fedorapeople.org/~limb/review/lzma-sdk/lzma-sdk-4.6.5-6.fc15.src.rpm
SPEC: http://fedorapeople.org/~limb/review/lzma-sdk/lzma-sdk.spec

Comment 77 Jason Tibbitts 2011-11-29 22:34:27 UTC
Cool, looks good to me as well (after foxing the fc16->fc15 typo in the URL above).

APPROVED

Sorry it took so long.

Comment 78 Gwyn Ciesla 2011-11-30 00:03:24 UTC
Awesome, thanks so much!  Sorry about the typo, I'm glad you were able to fox it. :)


New Package SCM Request
=======================
Package Name: lzma-sdk
Short Description: SDK for lzma compression
Owners: limb
Branches: 
InitialCC:

Comment 79 Gwyn Ciesla 2011-11-30 00:06:28 UTC
Git done (by process-git-requests).

Comment 80 Gwyn Ciesla 2011-11-30 00:41:35 UTC
Imported and built.  Thanks all!