Bug 1251500 (kwsys)

Summary: Review Request: kwsys - Kitware System Library
Product: [Fedora] Fedora Reporter: Mattias Ellert <mattias.ellert>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED DEFERRED QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: dominik, fedora, i, kevin, orion, package-review, rdieter
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2015-08-21 11:25:43 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: 1251198    

Description Mattias Ellert 2015-08-07 14:05:20 UTC
Spec URL: http://www.grid.tsl.uu.se/review/kwsys.spec
SRPM URL: http://www.grid.tsl.uu.se/review/kwsys-0.1-0.1.20150807gitb9df3e4.fc24.src.rpm

Description:
KWSys provides a platform-independent API to many common system
features that are implemented differently on every platform.

Fedora Account System Username: ellert

Comment 1 Dominik 'Rathann' Mierzejewski 2015-08-07 17:33:17 UTC
Is there any particular reason why you set SO version to 0.1?

Comment 2 Rex Dieter 2015-08-07 18:25:20 UTC
Not sure it's wise to even use shared libs here at all (rather than static libs)... upstream almost certain makes no ABI guarantees, so any updates make break stuff without notice.

Are static libs supported (without much fuss)?

Comment 3 Rex Dieter 2015-08-07 18:40:00 UTC
answering my own question: looks like it can build statically, using:
 -DKWSYS_BUILD_SHARED:BOOL=OFF -DBUILD_SHARED_LIBS:BOOL=OFF

Comment 4 Mattias Ellert 2015-08-08 07:50:12 UTC
(In reply to Dominik 'Rathann' Mierzejewski from comment #1)
> Is there any particular reason why you set SO version to 0.1?

The 0.1 chosen in order to follow https://fedoraproject.org/wiki/Packaging:Guidelines#Downstream_.so_name_versioning

(In reply to Rex Dieter from comment #2)
> Not sure it's wise to even use shared libs here at all (rather than static
> libs)... upstream almost certain makes no ABI guarantees, so any updates
> make break stuff without notice.

I really don't see the point of insisting on unbundling kwsys if you then rebundle it again by making it a static library.

For castxml it is not a big problem - it is a single application so linking against a static library will only bundle one copy of the code into the application which is not worse than the unbundled case - but also not much better.

For the other user of kwsys in Fedora that I am aware of - InsightToolkit - the currently bundled copy of the kwsys sources is compiled into a shared library (libitksys). Most of the other libraries in this package - and there are many - link to this library. If this would be unbundled using a common static library from Fedora each of these libraries would get a bundled copy of the static library - which I think would be a worse situation than the currently one bundled shared copy.

In my opinion - if the Fedora common version would be a static library I think it would be better to accept kwsys as a copylib. Unbundling only makes sense if the Fedora common version is a shared library.

Comment 5 Rex Dieter 2015-08-08 14:58:43 UTC
You did not address the ABI concern.  Upstream explicitly says that even API may not be stable here... so that implies that it's binary interface is of little or no concern either.

Why a bundled *shared* library never has problems... is because the app and the library are always rebuilt (and relinked) together.  That will not be the case if kwsys lib is rebuilt and upgraded separately.

At least with a static lib, applications (runtime) would be safe from such breakage, and we'd be tracking all of kwsys consumers anyway... and can rebuild things as needed.

It's your package, if you want to be the one to deal with the fallout of debugging and fixing applications that break whenever kwsys is updated, then go for it.

Comment 6 Rex Dieter 2015-08-08 14:59:55 UTC
Either way, I'd strongly encourage you to ask upstream for advice/feedback here on how best to unbundle it.

Comment 7 Orion Poplawski 2015-08-14 03:56:07 UTC
Upstream queried here: http://public.kitware.com/pipermail/cmake-developers/2015-August/026010.html

Comment 8 Mattias Ellert 2015-08-14 08:36:50 UTC
In my opinion, creating a package containing a static library would be the sum of two evils. You get all the work of maintaining a new package but still each package using the library get one (or more) bundled copies of the object code. You get a lot of work for no or little gain. It really is the worst of all options.

I also believe that packaging a static library would violate the FPC decision to unbundle (or at least the spirit of the decision) and would require a revision of the FPC decision to include a static linking exception. 

Either the package should be unbundled properly - which means as a shared library, or the decision to not treat kwsys as a copylib should be revisited. Doing an unbundle-then-reboundle-again solution that a static library would result in is really not appealing.

Comment 9 Rex Dieter 2015-08-14 13:47:21 UTC
"I also believe that packaging a static library would violate the FPC decision to unbundle"

you believe wrong, but feel free to ask fpc for clarification if you believe otherwise.

See also bug #1251198 , where another cmake maintainer (besides myself) also thinks this unbundling is... unwise (especially in the current incarnation)

Comment 10 Rex Dieter 2015-08-14 13:49:06 UTC
Further, I didn't read any mention of "must be shared library" in any FPC decisions on this topic.  Did I miss something?

Comment 11 Kevin Kofler 2015-08-15 00:15:43 UTC
It's true that a static library satisfies the FPC requirements for unbundling, but I also find this quite pointless and counterproductive. The point of unbundling should be to use a shared library.

Comment 12 Jason Tibbitts 2015-08-16 01:28:11 UTC
Packaging this as a static library is completely fine.  Obviously not as good a solution as a shared library were that possible, but not really an issue.  If you check the FPC meeting logs you'll see explicit discussion of a static library.  In fact, a static library has always been an acceptable solution to a bundling issue in the case that making a shared library is not feasible.

To fix an issue in.....

Bundled code without tracking: patch every single user, if we can figure out what actually bundles the code.

Bundled code with proper Provides: bundled(kwsys): patch and rebuild every user, which we can easily find with --whatprovides.

Static library: Patch the library exactly once, rebuild it and rebuild all users.

Shared library: Patch the library and rebuild it.

The first is never acceptable.  Any existing code should add the Provides: bit as soon as bundling is discovered.

Comment 13 Ben Boeckel 2015-08-20 02:34:17 UTC
*Puts CMake upstream hat on*

kwsys is meant to be copied on use (like gnulib). Making an external library of it (even static) means reworking how *all* projects with kwsys even use it today. *All* symbols are mangled in it and you'd end up with N copies of kwsys anyways: one with a cmsys (cmake) namespace, another with vtksys (VTK and ParaView), and itksys (ITK). It'd probably just need to ship the source so the files can be configured just as they are. But then you hit the "no ABI guarantee" problem between the projects.

One thing that we can do is strip kwsys down and move functionality which only CMake cares about into CMake itself. This, however, seems to still leave a large intersection between VTK, ParaView, and ITK:

- STL compatibility with *old* compilers; may go away as CMake doesn't self-host on some of these older compilers such as Borland 5;
- directory listings;
- glob support;
- command line argument parsing;
- regular expression;
- md5 and base64 routines;
- system information (e.g., number of processors);
- hash_map and hash_set implementations (or just typedefs if supported);
- process launching;
- share library support (dlopen and the like); and
- SystemTools (file path and filesystem utility functions).

I think kwsys should really get the FPC exception as gnulib has, so I vote for "use Provides: bundled(kwsys)" or something. It may even make sense to hack the version/release of the Provides to indicate the upstream revision of it and the mangled namespace such as "20150803-1.gitdad68c33.cmsys" (for CMake master right now).

Comment 14 Ben Boeckel 2015-08-20 02:37:00 UTC
Notes:

> One thing that we can do is strip kwsys down and move functionality which only CMake cares about into CMake itself.

No timeline.

> STL compatibility with *old* compilers; may go away as CMake doesn't self-host on some of these older compilers such as Borland 5;

Old compilers are still expected to self-host CMake, so some of the STL stuff may have to stick around, but that may be able to move into CMake itself. VTK (and therefore ParaView) at least already has GCC 4.1 and VS2008 as minimum requires as of 6.3. I do not know about ITK.

Comment 15 Orion Poplawski 2015-08-20 03:01:14 UTC
Ben - Thank you very much for your input.  However, let's discuss the merits of unbundling in https://fedorahosted.org/fpc/ticket/555  I've reopened that and hope to discuss it again at tomorrow's FPC meeting.

Comment 16 Ben Boeckel 2015-08-20 03:08:55 UTC
OK, will keep future discussion there.

> I've reopened that and hope to discuss it again at tomorrow's FPC meeting.

I'll try to pop in.

Comment 17 Mattias Ellert 2015-08-21 07:33:43 UTC
Shall we close this or leave it open?

Comment 18 Kevin Kofler 2015-08-21 11:25:43 UTC
IMHO, let's reevaluate this when/if upstream does the refactoring to make KWSys smaller.

Comment 19 Christopher Meng 2015-08-21 12:03:05 UTC
TBH, gnulib shouldn't be packaged as well. But you can't persuade people to believe you.