Bug 1262185 - ownCloud bundles all kinds of crap (patchwork, punic, forks of smb4php and phpzipstreamer)
ownCloud bundles all kinds of crap (patchwork, punic, forks of smb4php and ph...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: owncloud (Show other bugs)
24
All All
unspecified Severity urgent
: ---
: ---
Assigned To: James Hogarth
Fedora Extras Quality Assurance
:
Depends On: php-punic 1262469 mcnetic/zipstreamer owncloud/tarstreamer league/flysystem interfasys/lognormalizer
Blocks: DuplicSysLibsTracker
  Show dependency treegraph
 
Reported: 2015-09-11 01:03 EDT by Adam Williamson
Modified: 2016-05-01 19:31 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-05-01 19:31:42 EDT
Type: Bug
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 Adam Williamson 2015-09-11 01:03:21 EDT
OK, I feel bad about this as half of it is my fault, but after I looked harder at the packaging guidelines in re bundling today, I think it's fair to say owncloud is clearly violating the hell out of them.

I and before me Gregor had been proceeding, I think, on the belief that unpackaged things could be bundled, but that really doesn't seem to be the case:

https://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries , "bundled libraries being defined as libraries which exist and are mantained independently, whether or not they are packaged separately for Fedora" and https://fedoraproject.org/wiki/Packaging:Guidelines#Duplication_of_system_libraries , "In this RPM packaging context, the definition of the term 'library' includes: compiled third party source code resulting in shared or static linkable files, interpreted third party source code such as Python, PHP and others."

It really wasn't compliant all the way back to review, in my opinion. The originally imported spec:

http://pkgs.fedoraproject.org/cgit/owncloud.git/tree/owncloud.spec?id=9ee05fbe8604bcba580b82acc6704d3c281fd8fe

has a 'bundling notes' section:

# bundling notes:
## smb4php - forked php class from http://www.phpclasses.org/package/4129-PHP-Stream-wrapper-to-access-Windows-shared-files.html
## openid/phpMyID - forked from http://siege.org/projects/phpMyID and adjusted to oC needs
## openid/SimpleOpenID - adjusted class from http://roosenmaallen.com/2008/03/20/simpleopenid-for-php/
## Google - copy of a google playground project https://code.google.com/p/gdata-samples/source/browse/trunk/oauth_playground/
# containing random OAuth.php lib from http://oauth.googlecode.com/svn/code/php/ - I don't know what to say
## mediawiki contains two files picked from the mediawiki distribution
# Fedora packages mediawiki but not in a way that it's files would be reusable
# (i.e. the package doesn't contain the files itself but the tarball + deployment
# script)

you could argue that some or all of those might have been worth an FPC exception, but according to the policy they clearly *needed* an FPC exception, not just packager and reviewer judgement calls, which seems to have been what was used.

Since then, well, the details have changed, but things have overall gotten worse. Here are the current 'bundling notes' for 8.0.7:

## 3rdparty/patchwork - https://github.com/nicolas-grekas/Patchwork-UTF8
# "patchwork/utf8": "~1.1"
#
# Used to set a UTF-8 locale (function isSetLocaleWorking does not
# just test whether setlocale works, it actually asks Patchwork to
# set a locale) and for its pure PHP implementation of the Normalizer
# class otherwise found in php-intl. See lib/private/util.php
# Patch from adamw to use php-intl's if available:
# https://github.com/owncloud/core/pull/6620
# We'd also have to set a UTF-8 locale some other way to entirely
# replace OC's use of Patchwork.

## 3rdparty/mcnetic/phpzipstreamer - https://github.com/McNetic/PHPZipStreamer
# "mcnetic/phpzipstreamer": "dev-master"
#
# OC's copy is somewhat behind upstream, and slightly forked: they imported
# it on 2014-02-20, and added a downstream patch to make it work with
# PHP 5.3 on 2014-03-17:
# https://github.com/owncloud/3rdparty/commit/da3c9f651a26cf076249ebf25c477e3791e69ca3
# Upstream implemented a different fix against PHP 5.3 on 2014-03-14:
# https://github.com/McNetic/PHPZipStreamer/commit/0af57cc0d113b27e44455be4be690908c4909d78
# but OC has never synced with that fix. See:
# https://github.com/owncloud/core/pull/3439

## apps/files_external/3rdparty/smb4php - forked php class from
# http://www.phpclasses.org/package/4129-PHP-Stream-wrapper-to-access-Windows-shared-files.html
# Replaced by https://github.com/icewind1991/SMB post-8.0

## 3rdparty/punic - https://github.com/punic/punic
# "punic/punic": "1.1.0"
#
# Used in date localization: lib/private/l10n.php
# Should be straightforward to unbundle

If someone wants to take the trouble to make OC compliant, I'd suggest they need to package and unbundle punic and patchwork, and probably request an exception for the phpzipstreamer fork (that whole situation is kind of a mess) and the smb4php fork.

8.1 gets rid of the smb4php fork (though the replacements need to be packaged - I believe Remi is on that), but adds a major wrinkle in that OC now needs sabredav 2.1, but we also have Horde depending on sabredav, and that still needs 1.8. So we'd need to split that package up somehow. This has already been discussed between me and Remi and Shawn to some extent. There are various other new unpackaged deps in 8.1 too.
Comment 1 Shawn Iwinski 2015-09-11 16:05:12 EDT
I just submitted review requests for php-punic (bug #1262468) and php-patchwork-utf8 (bug #1262469)
Comment 2 Fedora Admin XMLRPC Client 2015-09-17 14:03:25 EDT
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.
Comment 3 James Hogarth 2016-01-06 19:50:59 EST
As a quick update to this...

The 8.2.2 work has brought the bundling down to:

Provides: bundled(deepdiver1975/tarstreamer)
Provides: bundled(interfasys/lognormalizer)
Provides: bundled(league/flysystem)
Provides: bundled(pear/console_getopt)
Provides: bundled(pear/pear-core-minimal)
Provides: bundled(swiftmailer)
Provides: bundled(mcnetic/phpzipstreamer)
Comment 4 Remi Collet 2016-01-07 08:35:13 EST
why pear/console_getopt and pear/pear-core-minimal, both are available ?
Comment 5 James Hogarth 2016-01-07 10:54:42 EST
Because I managed to miss them whilst working through the others....

Guess there will be a 8.2.2-2 in due course with more hacking to unbundle done on it ;)
Comment 6 Remi Collet 2016-01-08 06:28:01 EST
mcnetic/phpzipstreamer => review #1296901
Comment 7 Remi Collet 2016-01-08 08:49:41 EST
deepdiver1975/tarstreamer => owncould/tarstreamer => review #1296939
Comment 8 Adam Williamson 2016-01-08 11:38:19 EST
As I recall, the phpzipstreamer in ownCloud is forked from upstream's, there are more detailed notes in the spec file; so just packaging upstream may not be enough to unbundle OC's usage.
Comment 9 Remi Collet 2016-01-08 11:45:42 EST
@Admn, thanks for the notice, but I think this is ok now.

From 8.2.2   3rdparty/composer.lock

            "name": "mcnetic/zipstreamer",
            "version": "v0.7",
            "source": {
                "type": "git",
                "url": "https://github.com/McNetic/PHPZipStreamer.git",
                "reference": "44c99c659abf4dac92882437c1da68de824ca9d0"
            },

Which is the exact revision in https://github.com/remicollet/remirepo/blob/94ad2fa705b18be2d1fd223c6b9a44289f0ab032/php/php-mcnetic-zipstreamer/php-mcnetic-zipstreamer.spec#L9

Diffing the sources is also ok (the only diff is my patch to fix the test suite)
Comment 10 Adam Williamson 2016-01-08 12:10:14 EST
ah, cool, I guess they finally reconciled with upstream then.
Comment 11 Remi Collet 2016-01-14 03:49:01 EST
league/flysystem => review request #1298475
Comment 12 Remi Collet 2016-01-14 11:21:58 EST
interfasys/lognormalizer => review #1298649
Comment 13 Jan Kurik 2016-02-24 10:29:06 EST
This bug appears to have been reported against 'rawhide' during the Fedora 24 development cycle.
Changing version to '24'.

More information and reason for this action is here:
https://fedoraproject.org/wiki/Fedora_Program_Management/HouseKeeping/Fedora24#Rawhide_Rebase
Comment 14 James Hogarth 2016-03-11 07:31:35 EST
So the 8.2 which is in progress of going out will be completely unbundled of php libraries thanks to Remi's hard work.

The expectation is that 9.0 will be the same.

The question remaining is that this was focused on the php side of things - how about the javascript side? The included jquery should be simple enough to unbundle to js-jquery1 but there's a few other things too such as bootstrap, underscore, handlebars, jcrop etc 

Should these also be unbundled similar to the php side of things or are we considering these acceptable bundling given the lack of composer.lock style information in the javascript world to lock down versions?

Perhaps just test unbundling jquery to js-jquery1 ?
Comment 15 Fedora Admin XMLRPC Client 2016-03-20 13:13:55 EDT
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.
Comment 16 James Hogarth 2016-05-01 19:31:42 EDT
Well thanks to the sterling work of many there are no bundled PHP libraries in owncloud in our repos at this point.

1) EPEL6 - about to be retired so can be ignored
2) EPEL7 - the 8.2.3 update which strips all the php bundling and the autoloader about to hit testing
3) F22/23/24/rawhide - the initial 8.2.3 update has already gone out with the unbundling, in testing is an additional bundling we caught in testing and the autoloader based build

As a result of the above I'm going to close this with NEXTRELEASE finishing the clean up.

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