Bug 844080 (libmatecomponent) - Review Request: libmatecomponent -- Libraries for matecomponent package of MATE-Desktop
Summary: Review Request: libmatecomponent -- Libraries for matecomponent package of M...
Keywords:
Status: CLOSED ERRATA
Alias: libmatecomponent
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: MATE-DE-tracker
TreeView+ depends on / blocked
 
Reported: 2012-07-28 16:56 UTC by Dan Mashal
Modified: 2012-08-27 03:25 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-08-27 03:25:18 UTC
Type: Bug
Embargoed:
rdieter: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Dan Mashal 2012-07-28 16:56:43 UTC
Spec URL: http://vicodan.fedorapeople.org/libmatecomponent.spec
SRPM URL: http://vicodan.fedorapeople.org/mate-common-1.4.0-2.fc17.src.rpm
Description: Libraries for matecomponent package of MATE-Desktop

Comment 1 Mario Blättermann 2012-07-29 10:47:03 UTC
You might drop a few packages from BuildRequires because they are recursive dependencies:

autoconf (needed by automake)
automake (needed by gtk2-devel and intltool)
glib2-devel (needed by gtk2-devel)
libxml2 (needed by libxml2-devel)
mate-corba (needed by mate-corba-devel)
gettext (needed by intltool)

And you should review the runtime requirements in the same manner. Don't mention anything there if it is not really needed. Moreover, it would be better to write one line per dependency, or write the packages in an alphabetical order at least. Would be better readable.

After doing a scratch build without any entries in "Requires:" you should have a look at the resulting packages to see which of the requirements has been picked up automatically by rpm.

BTW, the gnome-doc-utils package is no really requirement for MATE, isn't it? Shouldn't this be mate-doc-utils instead? But I've seen that mate-doc-utils needs gnome-doc-utils anyway. Somewhat odd... We speak about an independent GNOME fork an require its tools...?

Comment 2 Wolfgang Ulbrich 2012-07-29 14:17:32 UTC
@Dan Mashal
I can build official release libmatecomponent-1.4.0 without any problems.
It's all done for my repo.
No need to use a own upstream.
And why you add http://vicodan.fedorapeople.org/mate-common-1.4.0-2.fc17.src.rpm
as SRPM for libmatecomponent in your description?

Comment 3 Mario Blättermann 2012-07-29 14:22:52 UTC
(In reply to comment #2)
> And why you add
> http://vicodan.fedorapeople.org/mate-common-1.4.0-2.fc17.src.rpm
> as SRPM for libmatecomponent in your description?

This obviously should be
SRPM URL: http://vicodan.fedorapeople.org/libmatecomponent-2.0-2.fc17.src.rpm

I assume it was Ctrl-C and Ctrl-V without have a second look at it...

Comment 4 Wolfgang Ulbrich 2012-07-29 14:25:54 UTC
(In reply to comment #1)
> BTW, the gnome-doc-utils package is no really requirement for MATE, isn't
> it? Shouldn't this be mate-doc-utils instead? But I've seen that
> mate-doc-utils needs gnome-doc-utils anyway. Somewhat odd... We speak about
> an independent GNOME fork an require its tools...?
The reason for this is that some conflicting files with gnome-doc-utils are rmoved in mate-doc-utils.
So mate-doc-utils need some files from gnome-doc-utils.
Sounds a little bit crazy, but it works.

Comment 5 Wolfgang Ulbrich 2012-07-29 14:27:33 UTC
(In reply to comment #3)
> (In reply to comment #2)
> > And why you add
> > http://vicodan.fedorapeople.org/mate-common-1.4.0-2.fc17.src.rpm
> > as SRPM for libmatecomponent in your description?
> 
> This obviously should be
> SRPM URL: http://vicodan.fedorapeople.org/libmatecomponent-2.0-2.fc17.src.rpm
> 
> I assume it was Ctrl-C and Ctrl-V without have a second look at it...

Yeah but as i said
no reason for use a own upstream

Comment 6 Mario Blättermann 2012-07-29 14:28:51 UTC
(In reply to comment #4)
> The reason for this is that some conflicting files with gnome-doc-utils are
> rmoved in mate-doc-utils.
> So mate-doc-utils need some files from gnome-doc-utils.
> Sounds a little bit crazy, but it works.

Off topic: If this problem will continue, then upstream developers should think about to drop the g-d-u stuff completely and switch to itstool instead.

Comment 7 Wolfgang Ulbrich 2012-07-29 15:51:08 UTC
sucessful scratch build with official release package

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

Comment 8 Mario Blättermann 2012-07-29 16:22:48 UTC
(In reply to comment #7)
> sucessful scratch build with official release package
> 
> http://koji.fedoraproject.org/koji/taskinfo?taskID=4340166

What means "official release package"? Where do you've got this from?

Too many BuildRequires again:

BuildRequires: 	libxml2-devel >= %{libxml2_version}
BuildRequires: 	mate-corba-devel >= %{mate_corba_version}
BuildRequires: 	intltool >= 0.14-1
BuildRequires: 	automake autoconf libtool
BuildRequires: 	gtk-doc
BuildRequires: 	flex, bison, zlib-devel, popt-devel
BuildRequires: 	dbus-glib-devel
BuildRequires: 	gettext
BuildRequires:  mate-common

To drop:

gettext (needed by intltool)
autoconf (needed by automake)
automake (needed by mate-corba-devel)



$ rpmlint -i -v *
...
libmatecomponent.i686: W: summary-not-capitalized C libmate component system
Summary doesn't begin with a capital letter.
...
libmatecomponent.i686: W: conffile-without-noreplace-flag /etc/matecomponent-activation/matecomponent-activation-config.xml
A configuration file is stored in your package without the noreplace flag. A
way to resolve this is to put the following in your SPEC file:
%config(noreplace) /etc/your_config_file_here

libmatecomponent.i686: E: incorrect-fsf-address /usr/share/doc/libmatecomponent-1.4.0/COPYING
The Free Software Foundation address in this file seems to be outdated or
misspelled.  Ask upstream to update the address, or if this is a license file,
possibly the entire file with a new copy available from the FSF.

libmatecomponent.i686: W: no-manual-page-for-binary matecomponent-slay
Each executable in standard binary directories should have a man page.
...
libmatecomponent.src:68: E: hardcoded-library-path in %{_prefix}/lib/matecomponent/servers
A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
should be replaced by something like /%{_lib} or %{_libdir}.
...
libmatecomponent.src:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 4)
The specfile mixes use of spaces and tabs for indentation, which is a cosmetic
annoyance.  Use either spaces or tabs for indentation, not both.
...
libmatecomponent.x86_64: E: incorrect-fsf-address /usr/share/doc/libmatecomponent-1.4.0/COPYING
The Free Software Foundation address in this file seems to be outdated or
misspelled.  Ask upstream to update the address, or if this is a license file,
possibly the entire file with a new copy available from the FSF.
...
7 packages and 0 specfiles checked; 41 errors, 19 warnings.


Not the whole output, interesting stuff only. All this has to be handled before we can speak about "Ready for review".

Comment 9 Wolfgang Ulbrich 2012-07-29 16:37:29 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > sucessful scratch build with official release package
> > 
> > http://koji.fedoraproject.org/koji/taskinfo?taskID=4340166
> 
> What means "official release package"? Where do you've got this from?

http://pub.mate-desktop.org/releases/1.4/libmatecomponent-1.4.0.tar.xz

> Too many BuildRequires again:
> 
> BuildRequires: 	libxml2-devel >= %{libxml2_version}
> BuildRequires: 	mate-corba-devel >= %{mate_corba_version}
> BuildRequires: 	intltool >= 0.14-1
> BuildRequires: 	automake autoconf libtool
> BuildRequires: 	gtk-doc
> BuildRequires: 	flex, bison, zlib-devel, popt-devel
> BuildRequires: 	dbus-glib-devel
> BuildRequires: 	gettext
> BuildRequires:  mate-common
> 
> To drop:
> 
> gettext (needed by intltool)
> autoconf (needed by automake)
> automake (needed by mate-corba-devel)
> 
> 
> 
> $ rpmlint -i -v *
> ...
> libmatecomponent.i686: W: summary-not-capitalized C libmate component system
> Summary doesn't begin with a capital letter.
> ...
> libmatecomponent.i686: W: conffile-without-noreplace-flag
> /etc/matecomponent-activation/matecomponent-activation-config.xml
> A configuration file is stored in your package without the noreplace flag. A
> way to resolve this is to put the following in your SPEC file:
> %config(noreplace) /etc/your_config_file_here
> 
> libmatecomponent.i686: E: incorrect-fsf-address
> /usr/share/doc/libmatecomponent-1.4.0/COPYING
> The Free Software Foundation address in this file seems to be outdated or
> misspelled.  Ask upstream to update the address, or if this is a license
> file,
> possibly the entire file with a new copy available from the FSF.
> 
> libmatecomponent.i686: W: no-manual-page-for-binary matecomponent-slay
> Each executable in standard binary directories should have a man page.
> ...
> libmatecomponent.src:68: E: hardcoded-library-path in
> %{_prefix}/lib/matecomponent/servers
> A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
> should be replaced by something like /%{_lib} or %{_libdir}.
> ...
> libmatecomponent.src:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4,
> tab: line 4)
> The specfile mixes use of spaces and tabs for indentation, which is a
> cosmetic
> annoyance.  Use either spaces or tabs for indentation, not both.
> ...
> libmatecomponent.x86_64: E: incorrect-fsf-address
> /usr/share/doc/libmatecomponent-1.4.0/COPYING
> The Free Software Foundation address in this file seems to be outdated or
> misspelled.  Ask upstream to update the address, or if this is a license
> file,
> possibly the entire file with a new copy available from the FSF.
> ...
> 7 packages and 0 specfiles checked; 41 errors, 19 warnings.
> 
> 
> Not the whole output, interesting stuff only. All this has to be handled
> before we can speak about "Ready for review".

I know, i did this quick scratch build only for showing
that building with current upstream from mate works.

But, thanks for your hints i will check it later.

Comment 10 Wolfgang Ulbrich 2012-07-29 16:58:37 UTC
(In reply to comment #6)
> (In reply to comment #4)
> > The reason for this is that some conflicting files with gnome-doc-utils are
> > rmoved in mate-doc-utils.
> > So mate-doc-utils need some files from gnome-doc-utils.
> > Sounds a little bit crazy, but it works.
> 
> Off topic: If this problem will continue, then upstream developers should
> think about to drop the g-d-u stuff completely and switch to itstool instead.

The mate devs knows about this
https://github.com/mate-desktop/mate-doc-utils/issues/2

But they have release 1.4.x now, and it's summertime vacation time.
Maybe in winter if they want to do that.
But feel free to make comment at this issue report.

Comment 11 Dan Mashal 2012-08-08 08:50:01 UTC
Spec URL: http://vicodan.fedorapeople.org/matespec/libmatecomponent.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatecomponent-1.4.0-3.fc17.src.rpm
Description: Libraries for matecomponent package of MATE-Desktop

Comment 12 Dan Mashal 2012-08-08 09:19:53 UTC
$ rpmlint libmatecomponent.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.


$ rpmlint libmatecomponent-1.4.0-3.fc17.src.rpm 
libmatecomponent.src: W: spelling-error Summary(en_US) matecomponent -> mate component, mate-component, component
libmatecomponent.src: W: spelling-error %description -l en_US matecomponent -> mate component, mate-component, component
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

Comment 13 Dan Mashal 2012-08-10 07:35:51 UTC
This package is ready for review. Leigh, would you like to take this one?

Comment 14 Wolfgang Ulbrich 2012-08-10 17:44:00 UTC
In relation to your last uploaded version 1.4.0-3 some suggestions.

1. Why you use pkgconfig as Requires and BuildRequires ?
Those are sufficient.
BuildRequires: 	libxml2-devel
BuildRequires: 	mate-corba-devel
BuildRequires:  mate-common
BuildRequires:  flex
BuildRequires:  popt-devel
BuildRequires:  byacc

2. You don't need BuildRequires in -devel again.

3. We need a provide under main package for future packages.
Provides: 		libmatecomponent-activation = %{version}-%{release}

and under -devel
Provides: 		libmatecomponent-activation-devel = %{version}-%{release}

repoquery --whatrequires libmatecomponent-activation
python-mate-matecomponent-0:1.2.0-3.fc16.i686
python-mate-matecomponent-0:1.2.0-3.fc16.x86_64
python-mate-matecomponent-0:1.4.0-1.fc16.i686
python-mate-matecomponent-0:1.4.0-1.fc16.x86_64

4. use --disable-static as configure flag

5. libmatecomponent x86_64  needs to own the directory that i686 packages install into.
For this we need the libmatecomponent-multishlib.patch, required for multilib installs  /usr/lib/matecomponent/servers
and in spec file under %install those lines.

for serverfile in $RPM_BUILD_ROOT%{_libdir}/matecomponent/servers/*.server; do
    sed -i -e 's|location *= *"/usr/lib\(64\)*/|location="/usr/$LIB/|' $serverfile
done

# noarch packages install to /usr/lib/matecomponent/servers
mkdir -p $RPM_BUILD_ROOT%{_prefix}/lib/matecomponent/servers


Those actions and the patch creates two directories on x86_64 /usr/lib/matecomponent/servers/ and /usr/lib64/matecomponent/servers/

All suggestions are very recommended to do.
Here is all done
Spec: http://raveit65.fedorapeople.org/Mate-Desktop/fc18/SPECS/libmatecomponent.spec
Patch: http://raveit65.fedorapeople.org/Mate-Desktop/patches/libmatecomponent-multishlib.patch

PS: pls upload actual sources before you asked for e reviewer.

Comment 15 leigh scott 2012-08-10 20:07:02 UTC
(In reply to comment #13)
> This package is ready for review. Leigh, would you like to take this one?

Sorry I haven't got the free time before my holiday to do the review and I'm pretty sure you wouldn't want to wait 5-6 weeks for me.

Comment 16 Dan Mashal 2012-08-11 02:09:26 UTC
Spec URL: http://vicodan.fedorapeople.org/libmatecomponent.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/1.4.0-3.fc17.src.rpm
Description: Libraries for matecomponent package of MATE-Desktop

Comment 17 Dan Mashal 2012-08-11 02:23:18 UTC
oops updated urls/versions here:

Spec URL: http://vicodan.fedorapeople.org/libmatecomponent.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatecomponent-1.4.0-4.fc17.src.rpm
Description: Libraries for matecomponent package of MATE-Desktop

Please review.

Comment 18 Dan Mashal 2012-08-11 02:25:20 UTC
Spec URL: http://vicodan.fedorapeople.org/matespec/libmatecomponent.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatecomponent-1.4.0-4.fc17.src.rpm
Description: Libraries for matecomponent package of MATE-Desktop

One more try. ;)

Comment 19 Rex Dieter 2012-08-11 02:34:36 UTC
i can review

Comment 20 Dan Mashal 2012-08-11 02:35:26 UTC
thx

Comment 21 Rex Dieter 2012-08-11 02:46:41 UTC
1.   SHOULD make -libs subpkg for shared libraries.  else,  the main pkg is going to get  multilib'd, and will likely lead to  multilib conflicts

2.  MUST include  in %files:
%{_libdir}/matecomponent-2.0/
%{_libdir}/matecomponent/
so this will get owned, as well as everything recursively under it

same goes  for
%{_libdir}/matecorba-2.0/
if nothing else lower in the stack owns this dir.

3.   MUST include in %files devel
%{_includedir}/libmatecomponent-2.0/
%{_includedir}/matecomponent-activation-2.0/
(so this will get owned, as well as everything recursively under it)

4.  SHOULD: -devel remove
%doc AUTHORS COPYING README
no need do duplicate the same stuff from the main pkg

5. SHOULD do verbose %build, use
make ... V=1

naming:  ok

scriptlets: ok

macros: ok

sources: ok
7ba05884fec91eb5c3bb2cf7300b0e16  libmatecomponent-1.4.0.tar.xz


address at least MUST items 2,3, and we're pretty close.

Comment 22 Rex Dieter 2012-08-11 02:48:06 UTC
checked about for
%{_libdir}/matecorba-2.0/
looks  like mate-corba should own that, but doesn't.  i'll go fix that it in git now.

Comment 23 Dan Mashal 2012-08-11 03:19:54 UTC
OK.

Fixed the rest of the stuff here:

Spec URL: http://vicodan.fedorapeople.org/matespec/libmatecomponent.spec
SRPM URL: http://vicodan.fedorapeople.org/materpms/srpms/libmatecomponent-1.4.0-5.fc17.src.rpm
Description: Libraries for matecomponent package of MATE-Desktop

Comment 24 Dan Mashal 2012-08-11 03:22:44 UTC
I believe the other dirs will be owned by matecomponent.

Comment 25 Rex Dieter 2012-08-11 04:20:28 UTC
looks like you put too much into the -libs pkg, it only really needs:

%{_libdir}/libmatecomponent-2.so.0*
%{_libdir}/libmatecomponent-activation.so.4*

these can  go here or in the main pkg
%{_libdir}/matecomponent-2.0/*
%{_libdir}/matecomponent/*
%{_libdir}/matecorba-2.0/*

these definitely still belong in -devel:
%{_libdir}/libmatecomponent-2.so
%{_libdir}/pkgconfig/libmatecomponent-2.0.pc
%{_libdir}/pkgconfig/matecomponent-activation-2.0.pc
%{_libdir}/libmatecomponent-activation.so

and remember to move the ldconfig scriptlets to  -libs too:
%post libs -p /sbin/ldconfig
%postun libs -p /sbin/ldconfig

Comment 27 leigh scott 2012-08-11 05:50:58 UTC
(In reply to comment #26)
> Done. Please check it.
> 
> Spec URL: http://vicodan.fedorapeople.org/matespec/libmatecomponent.spec
> SRPM URL:
> http://vicodan.fedorapeople.org/materpms/srpms/libmatecomponent-1.4.0-6.fc17.
> src.rpm

remove this bit as there are no libs in the main package

%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig

Comment 29 leigh scott 2012-08-11 06:16:53 UTC
You don't need the flags here (remove --disable-static from the autogen line)


NOCONFIGURE=1 ./autogen.sh --disable-static

as you specify it in %configure

%configure --disable-static

Comment 30 Dan Mashal 2012-08-11 08:36:29 UTC
doesnt make a difference.

Comment 31 Rex Dieter 2012-08-11 14:42:08 UTC
So, -libs dep on the main package needs to be *not* arch'd

Requires: %{name} = %{version}-%{release}

and the main pkg and  -devel should have:

Requires: %{name}-libs%{?_isa} = %{version}-%{release}

Please fix prior to doing any koji builds (or we can sort it together post-review if you like).


leigh's recent comments are indeed valid, but doesn't rise to the level of review blockers... I would appreciate those issues being dealt-with prior to doing any official fedora builds too.


APPROVED.

Comment 32 Wolfgang Ulbrich 2012-08-11 17:50:50 UTC
(In reply to comment #29)
> You don't need the flags here (remove --disable-static from the autogen line)
> 
> 
> NOCONFIGURE=1 ./autogen.sh --disable-static
> 
> as you specify it in %configure
> 
> %configure --disable-static

Dan, if you use NOCONFIGURE=1 the configure part of ./autogen.sh is disable.
So it doesn't matter if you use a configure flag or not at this point, because configure called by ./autogen.sh is disable.
But it looks better if you remove this ;)

Can you add from my comment 14
3. We need a provide under main package for future packages.
Provides: 		libmatecomponent-activation = %{version}-%{release}

and under -devel
Provides: 		libmatecomponent-activation-devel = %{version}-%{release}

because.....

repoquery --whatrequires libmatecomponent-activation
python-mate-matecomponent-0:1.2.0-3.fc16.i686
python-mate-matecomponent-0:1.2.0-3.fc16.x86_64
python-mate-matecomponent-0:1.4.0-1.fc16.i686
python-mate-matecomponent-0:1.4.0-1.fc16.x86_64

python-mate is a future package we need for future mate-applets.

and also point 5 we need.


Thank you for your attention

Comment 33 Dan Mashal 2012-08-11 23:06:10 UTC
Fixed prior concerns, and they will be in the first koji build.

Comment 34 Dan Mashal 2012-08-11 23:08:09 UTC
New Package SCM Request
=======================
Package Name: libmatecomponent
Short Description: Libraries for matecomponent package of MATE-Desktop
Owners: vicodan raveit65 rdieter
Branches: f16 f17 f18
InitialCC:

Comment 35 Gwyn Ciesla 2012-08-12 17:50:23 UTC
Git done (by process-git-requests).

Comment 36 Fedora Update System 2012-08-12 22:17:42 UTC
libmatecomponent-1.4.0-8.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-8.fc16

Comment 37 Fedora Update System 2012-08-12 22:17:53 UTC
libmatecomponent-1.4.0-8.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-8.fc17

Comment 38 Wolfgang Ulbrich 2012-08-13 01:30:42 UTC
I'm in doubt that this package work.
see comment 32
pls trust

MADE IN GERMANY

Comment 39 Rex Dieter 2012-08-13 01:56:43 UTC
Go ahead and commit fixes as you see fit, use your best judgement.

Comment 40 Fedora Update System 2012-08-14 01:00:02 UTC
libmatecomponent-1.4.0-8.fc17 has been pushed to the Fedora 17 testing repository.

Comment 41 Fedora Update System 2012-08-14 11:34:21 UTC
libmatecomponent-1.4.0-11.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-11.fc16

Comment 42 Fedora Update System 2012-08-14 11:35:58 UTC
libmatecomponent-1.4.0-11.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-11.fc17

Comment 43 Fedora Update System 2012-08-14 17:03:54 UTC
libmatecomponent-1.4.0-12.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-12.fc16

Comment 44 Fedora Update System 2012-08-14 17:04:58 UTC
libmatecomponent-1.4.0-12.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-12.fc17

Comment 45 Fedora Update System 2012-08-26 03:56:24 UTC
libmatecomponent-1.4.0-12.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-12.fc17

Comment 46 Fedora Update System 2012-08-26 03:56:36 UTC
libmatecomponent-1.4.0-12.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/libmatecomponent-1.4.0-12.fc16

Comment 47 Fedora Update System 2012-08-27 03:25:18 UTC
libmatecomponent-1.4.0-12.fc17 has been pushed to the Fedora 17 stable repository.


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