Bug 848208 - (owncloud-csync) Review Request: owncloud-csync - a file synchroniser utility
Review Request: owncloud-csync - a file synchroniser utility
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rex Dieter
Fedora Extras Quality Assurance
:
: 503136 565902 (view as bug list)
Depends On: 844960
Blocks: kde-reviews mirall
  Show dependency treegraph
 
Reported: 2012-08-14 18:12 EDT by Joseph Marrero
Modified: 2012-09-17 18:39 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-08-24 15:22:59 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
rdieter: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Joseph Marrero 2012-08-14 18:12:57 EDT
Spec URL: http://jmarrero.fedorapeople.org/packages/csync/csync.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/csync/csync-0.50.8-1.fc17.src.rpm
Description: csync is an implementation of a file synchronizer which provides the
feature of roaming home directories for Linux clients. csync makes use
of libsmbclient in Samba/Windows environments.
Fedora Account System Username: jmarrero
Comment 1 Kevin Kofler 2012-08-14 18:16:27 EDT
Putting this on kde-reviews because it's used by the OwnCloud client.
Comment 2 Kevin Kofler 2012-08-14 18:17:46 EDT
*** Bug 503136 has been marked as a duplicate of this bug. ***
Comment 3 Kevin Kofler 2012-08-14 18:18:35 EDT
*** Bug 565902 has been marked as a duplicate of this bug. ***
Comment 4 Kevin Kofler 2012-08-14 18:21:31 EDT
I updated the old, long dead reviews for csync to point to this one.
Comment 5 Joseph Marrero 2012-08-14 18:40:47 EDT
thanks
Comment 6 Joseph Marrero 2012-08-14 20:08:51 EDT
Koji successful build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4391293
Comment 7 Joseph Marrero 2012-08-14 23:10:28 EDT
New files that fixed dependency which made the koji build successful:

Spec URL: http://jmarrero.fedorapeople.org/packages/csync/csync.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/csync/csync-0.50.8-2.fc17.src.rpm

Previous files moved to http://jmarrero.fedorapeople.org/packages/csync/old/*
Comment 8 Gregor Tätzner 2012-08-15 09:47:23 EDT
Hey Joseph,

it seems that this package doesn't build without iniparser v3.1+
-> please specify the correct version in the BuildReqs
(slightly off topic: do you know if the iniparser maintainer plans an update for the stable fedora releases?)

incorrect changelog:
-> the version informations are missing

- don't include INSTALL file

- add ChangeLog to %doc

- the fsf address in COPYING is wrong

- you don't need %defattr macro anymore

- use a fully versioned dep to the lib package in the devel package

doc packages:
- please use %docdir macro and name the doc dirs after their parent packages
i.e. put the user doc into %docdir/csync-%version/

Can you make use of the tests in the package?
Comment 9 Gregor Tätzner 2012-08-15 09:54:23 EDT
oh and I think the config files belong to the main package.
Comment 10 Joseph Marrero 2012-08-15 10:27:21 EDT
Hi Gregor, 
yes, the iniparser maintainer plans to update for 17 and 18, he said that he will do it today. you can view here: https://bugzilla.redhat.com/show_bug.cgi?id=844960

Fixing csync spec....
Comment 11 Joseph Marrero 2012-08-15 16:20:25 EDT
Spec URL: http://jmarrero.fedorapeople.org/packages/csync/csync.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/csync/csync-0.50.8-3.fc17.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=4394000

Previous files moved to http://jmarrero.fedorapeople.org/packages/csync/old/*

The conf files where kept in the libs, I read them it aprears to address the csync-lib package better. Let me know if any other change should be made.

-
Joseph
Comment 12 Joseph Marrero 2012-08-15 19:01:13 EDT
I got to install the doc packages in their respective directories with their version and all, I made this new package:

Spec URL: http://jmarrero.fedorapeople.org/packages/csync/csync.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/csync/csync-0.50.8-4.fc17.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=4394355

Previous files moved to http://jmarrero.fedorapeople.org/packages/csync/old/*

-
Joseph
Comment 13 Gregor Tätzner 2012-08-16 11:35:06 EDT
nice, additional notes:

- BuildRequires:  gcc-c++
is part of the minimal build env, remove it

- please escape the macros in the changelog with two %
i.e. %%{name}

-%files devel-doc
%{_docdir}/%{name}-devel-doc-%{version}/html
-> better put those files just in %{_docdir}/%{name}-devel-%{version}/html

-csync.x86_64: W: manual-page-warning /usr/share/man/man1/csync.1.gz 169: warning: macro `HTML-TAG' not defined
the man page is slightly broken, but this is no blocker

this one is making me really nervous:
csync-libs.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libcsync.so.0.1.8 /lib64/libssl.so.10
The binary contains unused direct shared library dependencies.  This may
indicate gratuitously bloated linkage; check that the binary has been linked
with the intended shared libraries only.
(rpmlint on a installed csync-libs)

any ideas?

And did you informed upstream about the address issue in COPYING?
Comment 14 Gregor Tätzner 2012-08-16 12:28:31 EDT
(In reply to comment #13)
> this one is making me really nervous:
> csync-libs.x86_64: W: unused-direct-shlib-dependency
> /usr/lib64/libcsync.so.0.1.8 /lib64/libssl.so.10
> The binary contains unused direct shared library dependencies.  This may
> indicate gratuitously bloated linkage; check that the binary has been linked
> with the intended shared libraries only.
> (rpmlint on a installed csync-libs)
> 
> any ideas?
Yes dear sir, you have to include this cmake flag:
-DCMAKE_SHARED_LINKER_FLAGS="-Wl,--as-needed" \

I don't think you need this anymore:
-DCMAKE_SKIP_RPATH=ON \
Comment 15 Joseph Marrero 2012-08-16 16:18:33 EDT
Thank you for all the help Gregor,
Fixed all the above as indicated.

Spec URL: http://jmarrero.fedorapeople.org/packages/csync/csync.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/csync/csync-0.50.8-5.fc17.src.rpm

Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=4397027

Previous files moved to http://jmarrero.fedorapeople.org/packages/csync/old/*

I sent a message to uptream with the fsf address issue.
Comment 16 Andreas Schneider 2012-08-16 18:50:58 EDT
This is just my upstream opinion but please don't package something which upstream is not reviewed. I know Klaas is doing good work but I'm still against code without review. Especially if it is a file synchronizer.
Comment 17 Rex Dieter 2012-08-16 19:15:07 EDT
Re: comment #16

who do you mean by "upstream" exactly in this context?
Comment 18 Kevin Kofler 2012-08-16 19:22:08 EDT
And what exactly do you want reviewed (and by whom)? The upstream csync code? The Fedora packaging (specfile)?
Comment 19 Kevin Kofler 2012-08-16 19:27:55 EDT
Oh, I see it now, the specfile is packaging csync from download.owncloud.com rather than from csync.org. It's very misleading from OwnCloud to ship csync tarballs which look exactly like vanilla upstream csync when they're actually not. OTOH, the only reason csync is being packaged at all is for mirall. If upstream csync.org csync is not (yet?) suitable for mirall, we cannot use that.
Comment 20 Rex Dieter 2012-08-16 19:38:59 EDT
icky, ok, so one way forward would be to opt to rename *this* one to something like owncloud-csync to clearly designate that it's a fork, and possibly patch to rename the binary and shlib accordingly.  It would *appear* that mirall/owncloud may in fact only use the library portion, so we could further consider packaging only that piece as well (to avoid potential conflicts with any stock csync that may come to fedora)
Comment 21 Rex Dieter 2012-08-16 19:40:02 EDT
Andreas, which approach would you prefer (especially if you're still interested in bringing stock/upstream csync to fedora)?
Comment 22 Larry O'Leary 2012-08-16 22:35:58 EDT
I can't answer for Andreas but it seems that the stock/upstream csync package should have been brought to Fedora back in 2009 (Bugs 565902) but stalled during the package review process. I know I have been waiting for this to happen for quite some time.
Comment 23 Gregor Tätzner 2012-08-17 02:31:50 EDT
(In reply to comment #20)
> icky, ok, so one way forward would be to opt to rename *this* one to
> something like owncloud-csync to clearly designate that it's a fork, and
> possibly patch to rename the binary and shlib accordingly.  It would
> *appear* that mirall/owncloud may in fact only use the library portion, so
> we could further consider packaging only that piece as well (to avoid
> potential conflicts with any stock csync that may come to fedora)

(In reply to comment #19)
> OTOH, the only reason csync is being packaged at all is for
> mirall. If upstream csync.org csync is not (yet?) suitable for mirall, we
> cannot use that.

Whats about skipping this package and just bundling csync-owncloud into mirall?
https://bugzilla.redhat.com/show_bug.cgi?id=848211
Comment 24 Andreas Schneider 2012-08-17 03:28:29 EDT
I will integrate the owncloud patches to csync, but I will not make a release if there aren't unit tests for this code and I haven't had time to review and pick the patches from owncloud for csync.

I hope to find time after my vacation for that.

However please don't package owncloud csync under the name csync in Fedora.
Comment 25 Joseph Marrero 2012-08-17 09:37:53 EDT
I think skiping this package is non sense, a lot of people wants the stock csync too right? So I will package csync uptream if that is ok by Andreas since the spec is almost the same for both.

And try and set csync owncloud to ocsync or owncloud-csync

then for mirall set it for look for owncloud-csync or bundle it with Mirall. If Klass is the uptream for Mirall and his version of Csync I guess the right call is to use his code for mirall and owncloud-csync until a version of mirall that uses csync. When Mirall uses the uptream csync as dep then csync-owncloud will not be neaded any more but we can't tell if this will ever happen.

Its hard to say it will, csync-owncloud has been under constant changes as per owncloud and mirall all this are in constant development.

By maintaining uptream csync, owncloud-csync and mirall I can work for this to happen in a timely maner.

Now the question is if @Adreas Schneider is ok having me maintaining his csync or if he wants to maintain the fedora package for uptream csync.
Comment 26 Joseph Marrero 2012-08-17 15:31:24 EDT
the upstream csync needs log4c-devel, this dependency is not met by fedora right now. I will not package csync upstream until its needed by mirall, so Andreas can pick it up If he wants. I will continue to package the csync dav branch made by Klass Freitag . I will rename it owncloud-csync.
Comment 27 Joseph Marrero 2012-08-17 17:18:52 EDT
So I renamed the package to owncloud-csync, made the necessary changes in the spec to build correctly.

NEW SRPM: http://jmarrero.fedorapeople.org/packages/owncloud-csync/owncloud-csync-0.50.8-6.fc17.src.rpm
NEW SPEC: http://jmarrero.fedorapeople.org/packages/owncloud-csync/owncloud-csync.spec
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=4401060


All old files are in: http://jmarrero.fedorapeople.org/packages/owncloud-csync/old/*

Please let me know if this is OK.
Comment 28 Kevin Kofler 2012-08-17 17:25:25 EDT
Andreas, if you still want to get vanilla csync in, we can reopen bug #844960 as long as nobody else submits it.
Comment 29 Kevin Kofler 2012-08-17 17:26:16 EDT
Sorry, wrong bug ID…

Andreas, if you still want to get vanilla csync in, we can reopen bug #565902 as long as nobody else submits it.
Comment 30 Gregor Tätzner 2012-08-18 05:24:00 EDT
looks good, except that the file names still indicate a unmodified csync. If Andreas is ever going to package stock csync, this will generate file conflicts.

After all comment #20 from rdieter is still valid. You can try to patch the CMakeLists.txt files to change the names of the generated libs.
Comment 31 Joseph Marrero 2012-08-18 11:38:00 EDT
I made 6 patches for the CMakeLists in the package, it builds and installs ok.

NEW SRPM: http://jmarrero.fedorapeople.org/packages/owncloud-csync/owncloud-csync-0.50.8-7.fc17.src.rpm
NEW SPEC: http://jmarrero.fedorapeople.org/packages/owncloud-csync/owncloud-csync.spec
Koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=4401620


All old files are in: http://jmarrero.fedorapeople.org/packages/owncloud-csync/old/*

Please let me know if there is anything else.
Comment 32 Joseph Marrero 2012-08-18 12:10:09 EDT
Also I made this: https://docs.google.com/folder/d/0B0IbyJGzQiLLRXBiZVRkMTk4bWc/edit

http://koji.fedoraproject.org/koji/taskinfo?taskID=4401292

This Is mirall with csync integrated as described here: http://owncloud.org/dev/sync-clients/linux-build/

The thing with this is that it builds just fine in Koji. But when Installing mirall-common installs just fine but when installing owncloud-client or mirall then I get this:

Error: Package: mirall-1.0.5-3.fc17.x86_64 (/mirall-1.0.5-3.fc17.x86_64)
           Requires: libcsync.so.0()(64bit)
 You could try using --skip-broken to work around the problem
 You could try running: rpm -Va --nofiles --nodigest



I checked the spec there is not csync dep there and while building CMAKE find csync libs and builds mirall and owncloud-client just fine.

But when Installing it complains about libcsync, in the above link It says that if I build mirall this way there is no need to install csync on the machine.

Let me know what path to pursue
and what goes better along the guidelines of fedora.
Comment 33 Rex Dieter 2012-08-18 13:26:49 EDT
Since we're changing the library soname, you'll need to rebuild mirall against the new one (else, it still wants the old name, as you discoverred).
Comment 34 Gregor Tätzner 2012-08-18 13:38:01 EDT
(In reply to comment #32)
> Also I made this:
> https://docs.google.com/folder/d/0B0IbyJGzQiLLRXBiZVRkMTk4bWc/edit
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=4401292
> 
> This Is mirall with csync integrated as described here:
> http://owncloud.org/dev/sync-clients/linux-build/

Actually this is not about integrating csync into mirall (in other terms: linking the csync lib statically). That page just describes how you can build mirall without having to install the csync devel files system wide. Nevertheless you would need libcsync at runtime in libdir. And thats your problem here: rpm detects the runtime deps of mirall but can't find libcsync (and libocsync is not suitable because you build mirall against libcsync)
Comment 35 Gregor Tätzner 2012-08-18 13:41:15 EDT
apart from that, please mark the doc packages as NoArch. just html stuff in there
Comment 36 Joseph Marrero 2012-08-18 13:58:15 EDT
Ok,
I will continue the path of creating the owncloud-csync and mirall packages separate, doing the last build of owncloud-csync to mark NoArch the docs and moving to mirall.

Let me know what else is needed to pass the review.
Comment 37 Rex Dieter 2012-08-18 14:03:09 EDT
Ok, I'll do the formal review, since no one's assigned the bug or set the review flag yet.
Comment 39 Rex Dieter 2012-08-18 14:28:43 EDT
naming: ok

sources: ok
$ md5sum *.bz2
322f6fa22ca0e8cd05f23dc0d075e7ca  csync-0.50.8.tar.bz2

licensing: ok

macros: ok

scriptlets: ok

builds/installs: ok

thanks, looks good now.


APPROVED
Comment 40 Joseph Marrero 2012-08-18 14:32:02 EDT
New Package SCM Request
=======================
Package Name: owncloud-csync
Short Description: a file synchroniser utility
Owners: jmarrero
Branches: f17 f18
Comment 41 Jon Ciesla 2012-08-18 21:44:52 EDT
Git done (by process-git-requests).
Comment 42 Fedora Update System 2012-08-19 13:10:45 EDT
owncloud-csync-0.50.8-8.fc17,mirall-1.0.5-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/owncloud-csync-0.50.8-8.fc17,mirall-1.0.5-3.fc17
Comment 43 Fedora Update System 2012-08-19 13:13:04 EDT
owncloud-csync-0.50.8-8.fc18,mirall-1.0.5-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/owncloud-csync-0.50.8-8.fc18,mirall-1.0.5-3.fc18
Comment 44 Fedora Update System 2012-08-19 17:51:16 EDT
owncloud-csync-0.50.8-8.fc17, mirall-1.0.5-3.fc17 has been pushed to the Fedora 17 testing repository.
Comment 45 Fedora Update System 2012-08-20 03:47:48 EDT
owncloud-csync-0.50.8-9.fc17,mirall-1.0.5-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/owncloud-csync-0.50.8-9.fc17,mirall-1.0.5-4.fc17
Comment 46 Fedora Update System 2012-08-20 03:58:35 EDT
owncloud-csync-0.50.8-9.fc18,mirall-1.0.5-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/owncloud-csync-0.50.8-9.fc18,mirall-1.0.5-4.fc18
Comment 47 Fedora Update System 2012-08-24 15:22:59 EDT
owncloud-csync-0.50.8-9.fc17, mirall-1.0.5-4.fc17 has been pushed to the Fedora 17 stable repository.
Comment 48 Fedora Update System 2012-09-17 18:39:26 EDT
owncloud-csync-0.50.8-9.fc18, mirall-1.0.5-4.fc18 has been pushed to the Fedora 18 stable repository.

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