Bug 1157402 - Review Request: liblxqt - Core LXQT library
Summary: Review Request: liblxqt - Core LXQT library
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1151711 (view as bug list)
Depends On: lxqt-globalkeys
Blocks: qt-reviews liblxqt-mount lxqt-common lxqt-qtplugin lxqt-session lxqt-notificationd lxqt-about lxqt-openssh-askpass lxqt-runner lxqt-panel lxqt-policykit lxqt-powermanagement lxqt-config
TreeView+ depends on / blocked
 
Reported: 2014-10-27 07:50 UTC by Eugene A. Pivnev
Modified: 2016-02-12 12:25 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-11-07 16:21:29 UTC
Type: ---
Embargoed:
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Eugene A. Pivnev 2014-10-27 07:50:55 UTC
Spec URL: https://tieugene.fedorapeople.org/rpms/liblxqt/liblxqt.spec
SRPM URL: https://tieugene.fedorapeople.org/rpms/liblxqt/liblxqt-0.8.0-1.fc20.src.rpm
Description: Core utility library for all LXQT components
Fedora Account System Username: tieugene
Koji builds:
http://koji.fedoraproject.org/koji/taskinfo?taskID=7945877 (F22)
http://koji.fedoraproject.org/koji/taskinfo?taskID=7945892 (F21)

Comment 1 Helio Chissini de Castro 2014-10-29 17:58:40 UTC
Updated specs with merge from Eugene original package

Spec URL: 
SRPM URL: https://heliocastro.fedorapeople.org/lxqt/liblxqt-0.8.0-3.fc21.src.rpm
Description: https://heliocastro.fedorapeople.org/lxqt/liblxqt.spec
Fedora Account System Username: heliocastro

Comment 2 Helio Chissini de Castro 2014-10-29 18:00:29 UTC
Missing description

Spec: https://heliocastro.fedorapeople.org/lxqt/liblxqt.spec
SRPM URL: https://heliocastro.fedorapeople.org/lxqt/liblxqt-0.8.0-3.fc21.src.rpm
Description: Core Shared library for LXQt desktop suite
Fedora Account System Username: heliocastro

Comment 3 Rex Dieter 2014-10-30 12:58:12 UTC
*** Bug 1151711 has been marked as a duplicate of this bug. ***

Comment 4 Rex Dieter 2014-10-30 13:10:05 UTC
helio, all those Requires: pkgconfig in -devel pkg should get autodetected (so you shouldn't need to list anything explicitly).  If any dependencies are missed by autodetection, that's a bug we can fix in the lxqt pkgconfig files (ie, they could be missing Requires: or Requires.private: keys)

Comment 5 Rex Dieter 2014-11-03 19:34:17 UTC
I can start the formal review:

1. License: LGPL-2.1+ NOT ok, MUST use one listed on
https://fedoraproject.org/wiki/Licensing
probably want
License: LGPLv2

2. MUST drop extraneous -devel dependency
BuildRequires:	pkgconfig(Qt5Xdg) >= 1.0.0
it is already list in the main package (no need to duplicate it here)

3. SHOULD not use %make_install macro (it's only compatible with very recent rpm versions, certainly not centos6), I'd recommend using instead:
make install/fast DESTDIR=%{buildroot} -C build

4. SHOULD not omit translation,
rm -f %{buildroot}/%{_datadir}/lxqt-qt5/translations/liblxqt/liblxqt_sr
or at least provide justification why.

5. SHOULD track library soname, instead of
%{_libdir}/liblxqt-qt5.so.*
use
%{_libdir}/liblxqt-qt5.so.0*
(this will give you warning when incompatible library versions land when updating the package)


I'd encourage you to consider adopting something close to helio's version, it does not suffer any of these issues.

Comment 6 Helio Chissini de Castro 2014-11-03 20:01:05 UTC
Eugene, Rex, can we just start from my latest build ?
Is just an evolution from the Eugene anyway, and them we settle this down for good.

https://heliocastro.fedorapeople.org/lxqt/liblxqt-0.8.0-4.fc21.src.rpm

Comment 7 Rex Dieter 2014-11-03 23:48:50 UTC
I don't care, but that's up to Eugene how to proceed for now.  We can certainly merge additional necessary changes after the review is done.

Comment 8 Eugene A. Pivnev 2014-11-04 07:07:55 UTC
(In reply to Helio Chissini de Castro from comment #6)
> Eugene, Rex, can we just start from my latest build ?
> Is just an evolution from the Eugene anyway, and them we settle this down
> for good.
> 
> https://heliocastro.fedorapeople.org/lxqt/liblxqt-0.8.0-4.fc21.src.rpm

We can't start before review is done.
And I agree with Rex - I can add your patches after.

Comment 9 Rex Dieter 2014-11-06 13:27:58 UTC
ping, eugene, can you post updated .spec to comply with issues raised in comment #5 ?

Comment 10 Eugene A. Pivnev 2014-11-06 17:36:49 UTC
(In reply to Rex Dieter from comment #9)
> ping, eugene, can you post updated .spec to comply with issues raised in
> comment #5 ?

Oops... Sorry - I get lost in lxqt-related emails :-)
1. #3 - maybe - to make centos6-only workaround?
2. #4 - the same (this is rhel6-specific)
3. About Helio's patche - I need investigations - how they will works. Will try tomorrow.
4. What about I will not update release with "comment #5" fixes from 1 to 2?

Comment 11 Rex Dieter 2014-11-06 17:57:59 UTC
The only items that are review blockers are MUST items (1,2), the others would be nice (and recommended), but not required to pass review.

Comment 12 Eugene A. Pivnev 2014-11-06 19:02:51 UTC
(In reply to Rex Dieter from comment #11)
> The only items that are review blockers are MUST items (1,2), the others
> would be nice (and recommended), but not required to pass review.

1,2 was not replied because they are must.
I was interesting in details.

So, if you permit me _not_ update release - then I will tomorrow:
* fix #1, #2, #5
* make centos6-workaround for #3, #5.
* Helio's patches will be in next (or current) release. Need tryings.
* release will be "1"
Ok?

PS. Because fine number :-)

Comment 13 Rex Dieter 2014-11-06 20:16:36 UTC
It is good practice to increment Release and add a changelog entry for *every* change, particularly important in package reviews.

See also:
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

p.s. I don't care about helio's patches for now, we can address that after the review.

Comment 14 Eugene A. Pivnev 2014-11-07 06:20:49 UTC
(In reply to Rex Dieter from comment #5)
> I can start the formal review:

#1..5 fixed:
Spec: https://tieugene.fedorapeople.org/rpms/liblxqt/liblxqt.spec
SRPM: https://tieugene.fedorapeople.org/rpms/liblxqt/liblxqt-0.8.0-2.fc20.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=8060403
(other distros haven't libqtxdg 1.0.0)

Comment 15 Rex Dieter 2014-11-07 12:34:39 UTC
didn't address item 4, why?

Nor satisfactorily address 3, instead of simplifying as I suggested, you made it more complicated by introducing a conditional.

Either way, those were not blockers, the rest looks good, thanks.

APPROVED

Comment 16 Helio Chissini de Castro 2014-11-07 13:09:59 UTC
Now can you add me as a co-maintainer of the package ? I already did the ACL's requests and then i can push the patches

Comment 17 Rex Dieter 2014-11-07 14:26:01 UTC
New Package SCM Request
=======================
Package Name: liblxqt
Short Description: Core LXQT library
Upstream URL: http://www.lxde.org
Owners: heliocastro tieugene rdieter
Branches: f19 f20 f21 el6 epel7
InitialCC:

Comment 18 Gwyn Ciesla 2014-11-07 15:46:57 UTC
Git done (by process-git-requests).

Comment 19 Rex Dieter 2014-11-07 16:21:29 UTC
imported, thanks.

Comment 20 Raphael Groner 2016-02-12 12:25:34 UTC
Removing alias to allow general search for bugs.


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