Bug 848211 - (mirall) Review Request: mirall - owncloud desktop client
Review Request: mirall - owncloud desktop client
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
:
Depends On: owncloud-csync
Blocks: kde-reviews
  Show dependency treegraph
 
Reported: 2012-08-14 18:17 EDT by Joseph Marrero
Modified: 2012-09-17 18:39 EDT (History)
5 users (show)

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


Attachments (Terms of Use)
lib rename patch (1.16 KB, patch)
2012-08-18 13:28 EDT, Gregor Tätzner
no flags Details | Diff

  None (edit)
Description Joseph Marrero 2012-08-14 18:17:59 EDT
Spec URL: http://jmarrero.fedorapeople.org/packages/mirall/mirall.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/mirall/mirall-1.0.4-2.fc17.src.rpm
Description: Mirall owncloud-client enables you to connect to your private
ownCloud Server. With it you can create folders in your home
directory, and keep the contents of those folders synced with your
ownCloud server. Simply copy a file into the directory and the 
ownCloud Client does the rest.
Fedora Account System Username: jmarrero
Comment 1 Gregor Tätzner 2012-08-15 14:04:37 EDT
lets have a look:

- owncloud-client.desktop: error: value "Utility" for string list key "Categories" in group "Desktop Entry" does not have a semicolon (';') as trailing character

- BuildRequires:  oxygen-icon-theme
Really?

-Requires:       net-tools
-Requires:       iproute
I've never used mirall before, but this can't be right. iproute is a replacement for net-tools. Why would you install both?

-again the changelog is a little bit borked: please insert the appropriate versions

-%config %{_bindir}/../../etc/exclude.lst
just use configdir ;) Actually this file looks quite lost. Can you also put it in /etc/mirall or rename it? and you want to use noreplace, too

-there are .so files in libdir: please call ldconfig in post and postun

-drop defattr

-BuildRequires:  qt4-devel >= 4.7
You can remove that check. even f16 ships qt 4.8

- I think you can also remove the first two lines in the description. This "header" is not really necessary there

And it would be great if could you upgrade to the new mirall release

Thanks :)
Comment 2 Joseph Marrero 2012-08-15 17:05:46 EDT
Gregor thank you for the help,
this is the new versions fixing all the above and updating to mirall-1.0.5

Spec URL: http://jmarrero.fedorapeople.org/packages/mirall/mirall.spec
SRPM URL: http://jmarrero.fedorapeople.org/packages/mirall/mirall-1.0.5-1.fc17.src.rpm

the previous files are now moved to:
http://jmarrero.fedorapeople.org/packages/mirall/old/*

Let me know if I missed something.
Comment 3 Gregor Tätzner 2012-08-16 13:39:51 EDT
a more in deep look:

- the package contains desktop file icons - you must update the icon cache:
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache

- mirall.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmirallsync.so /lib64/libQtTest.so.4...
-> again set -Wl,--as-needed

- upstream names the owncloud-client binary just owncloud. This is dumb and much too generic. Maybe owncloud-sync-client would be more appropriate...be creative but don't forget the desktop file

- actually this package ships two apps: a sync client and some kind of installation tool. Would it make sense to split it up? I how far are those 2 apps related? Can you use one without the other?

- blocker:
mirall.x86_64: E: invalid-soname /usr/lib64/libmirallsync.so libmirallsync.so
The soname of the library is neither of the form lib<libname>.so.<major> or
lib<libname>-<major>.so.
mirall.x86_64: E: invalid-soname /usr/lib64/libowncloudsync.so libowncloudsync.so
The soname of the library is neither of the form lib<libname>.so.<major> or
lib<libname>-<major>.so.

->I suppose these are not dev libs, so they must be versioned

- "- moved exclude.lst to /etc/mirall/"
Is it that easy? Are you sure the config file is still read by the client?
Oh, and mark it as %config

- BuildRequires:  inetd desktop-file-utils
Builds fine without inetd?

keep up :)
Comment 4 Kevin Kofler 2012-08-16 19:16:11 EDT
> - blocker:
> mirall.x86_64: E: invalid-soname /usr/lib64/libmirallsync.so libmirallsync.so
> The soname of the library is neither of the form lib<libname>.so.<major> or
> lib<libname>-<major>.so.
> mirall.x86_64: E: invalid-soname /usr/lib64/libowncloudsync.so
> libowncloudsync.so
> The soname of the library is neither of the form lib<libname>.so.<major> or
> lib<libname>-<major>.so.
>
> ->I suppose these are not dev libs, so they must be versioned

Says what guideline?

Lack of library versioning is an upstream issue. I am not aware of any Fedora guideline requiring shared libraries to be versioned, and in fact I've been told multiple times that Fedora does NOT recommend inventing our own library versioning because the versioned sonames we come up with might conflict with later upstream ones.

It IS recommended that shared libraries be versioned, but it must be done by upstream.
Comment 5 Joseph Marrero 2012-08-17 21:58:08 EDT
this is the new version
I splited the apps, to mirall-common wich is a dependency of owncloud-client and mirall.

Tested owncloud and mirall and they work just fine without each other installed. 
mirall common holds config file (/etc/exclude.lst) and lang.

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

the previous files are moved to:
http://jmarrero.fedorapeople.org/packages/mirall/old/*

asked uptream about the posiblity of moving /etc/exclude.lst to /etc/mirall/exclude.lst and adding versioning to the libs.
Comment 6 Gregor Tätzner 2012-08-18 13:28:02 EDT
Very well, the objective is now to make use of the new owncloud-csync libs. For that you have to patch the buildsys again. I've attached a small patch...something in this way should do the job

And can you comment please on that BuildReq to xinetd?

(In reply to comment #5)
> asked uptream about the posiblity of moving /etc/exclude.lst to
> /etc/mirall/exclude.lst and adding versioning to the libs.
excellent
Comment 7 Gregor Tätzner 2012-08-18 13:28:49 EDT
Created attachment 605354 [details]
lib rename patch
Comment 8 Rex Dieter 2012-08-18 14:03:31 EDT
I'll do the formal review today, since no one's assigned the bug or set the review flag yet.
Comment 9 Rex Dieter 2012-08-18 14:54:49 EDT
naming: ok

sources: ok
$ md5sum mirall*.bz2
b7a96411f092bb16f88e3868a558032f  mirall-1.0.5.tar.bz2

1. MUST: build fails, currently does not build against the renamed owncloud-csync, will need some variant of  the patch from comment #7

2. MUST: missing icon scriptlets for owncloud-client subpkg
Comment 10 Rex Dieter 2012-08-18 14:56:10 EDT
oh,  and please document/justify (or remove) the following dependencies:
BuildRequires: inet
Requires: iproute
Requires: oxygen-icon-theme
Comment 12 Rex Dieter 2012-08-18 17:09:31 EDT
licensing: ok

scriptlets:  ok

macros: ok

3. SHOULD remove 
-DCMAKE_INSTALL_PREFIX=%{_prefix} -DCMAKE_INSTALL_SYSCONFDIR=%{_sysconfdir}
the default %cmake macro already includes these

otherwise, good work.  3 can be fixed post-review.


APPROVED.
Comment 13 Joseph Marrero 2012-08-18 17:16:23 EDT
New Package SCM Request
=======================
Package Name: mirall
Short Description: owncloud desktop client
Owners: jmarrero
Branches: f17 f18
Comment 14 Gwyn Ciesla 2012-08-18 21:45:19 EDT
Git done (by process-git-requests).
Comment 15 Fedora Update System 2012-08-19 13:10:51 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 16 Fedora Update System 2012-08-19 13:13:10 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 17 Fedora Update System 2012-08-19 17:51:23 EDT
owncloud-csync-0.50.8-8.fc17, mirall-1.0.5-3.fc17 has been pushed to the Fedora 17 testing repository.
Comment 18 Fedora Update System 2012-08-20 03:47:56 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 19 Fedora Update System 2012-08-20 03:58:41 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 20 Fedora Update System 2012-08-24 15:23:06 EDT
owncloud-csync-0.50.8-9.fc17, mirall-1.0.5-4.fc17 has been pushed to the Fedora 17 stable repository.
Comment 21 Fedora Update System 2012-09-17 18:39:33 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.