Bug 1251500 - (kwsys) Review Request: kwsys - Kitware System Library
Review Request: kwsys - Kitware System Library
Status: CLOSED DEFERRED
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 1251198
  Show dependency treegraph
 
Reported: 2015-08-07 10:05 EDT by Mattias Ellert
Modified: 2015-08-21 08:03 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-08-21 07:25:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Mattias Ellert 2015-08-07 10:05:20 EDT
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 13:33:17 EDT
Is there any particular reason why you set SO version to 0.1?
Comment 2 Rex Dieter 2015-08-07 14:25:20 EDT
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 14:40:00 EDT
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 03:50:12 EDT
(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 10:58:43 EDT
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 10:59:55 EDT
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-13 23:56:07 EDT
Upstream queried here: http://public.kitware.com/pipermail/cmake-developers/2015-August/026010.html
Comment 8 Mattias Ellert 2015-08-14 04:36:50 EDT
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 09:47:21 EDT
"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 09:49:06 EDT
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-14 20:15:43 EDT
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-15 21:28:11 EDT
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-19 22:34:17 EDT
*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-19 22:37:00 EDT
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-19 23:01:14 EDT
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-19 23:08:55 EDT
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 03:33:43 EDT
Shall we close this or leave it open?
Comment 18 Kevin Kofler 2015-08-21 07:25:43 EDT
IMHO, let's reevaluate this when/if upstream does the refactoring to make KWSys smaller.
Comment 19 Christopher Meng 2015-08-21 08:03:05 EDT
TBH, gnulib shouldn't be packaged as well. But you can't persuade people to believe you.

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