Bug 488995 - Review Request: pidgin-tlen - Tlen IM Pidgin plugin
Summary: Review Request: pidgin-tlen - Tlen IM Pidgin plugin
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Simon
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-03-06 17:07 UTC by Dominik 'Rathann' Mierzejewski
Modified: 2009-12-19 21:28 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-12-19 21:28:42 UTC
Type: ---
Embargoed:
dominik: fedora-review?


Attachments (Terms of Use)

Description Dominik 'Rathann' Mierzejewski 2009-03-06 17:07:30 UTC
Spec URL: http://rathann.fedorapeople.org/review/pidgin-tlen.spec
SRPM URL: http://rathann.fedorapeople.org/review/pidgin-tlen-0-0.1.20090209.fc9.src.rpm
Description:
This is a Tlen chat plugin for Pidgin and libpurple messengers.

Comment 1 Simon 2009-03-27 08:52:01 UTC
I didn't find your surname in fas. You need a sponsor, right?
I will add the need sponsor blocker, if i am wrong, then please remove it.

some issues:
--
%global snap 20090209
why global instead of define?
--
%{__make}
please use commands like they are, not as a macro.
make instead of %{__make}
--
BuildRoot Tag
https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
your buildroot tag is not valid!
--
%defattr(644,root,root,755)
please use %defattr(-,root,root,-)
--
makro couples 
please use:
$RPM_OPT_FLAGS and $RPM_BUILD_ROOT
or
%{buildroot} and %{optflags}
please use one of this couples, but do not mix this.
--
BuildRequires: pidgin-devel
Requires: pidgin
if you have as BR pidgin-devel, then will rpm requires pidgin automaticly.
you don't need Requires: pidgin
--
this is a tlen plugin, so it will be need 
Requires: libtlen
or?! I didn't test it, yet.

Comment 2 Dominik 'Rathann' Mierzejewski 2009-03-27 09:29:33 UTC
(In reply to comment #1)
> I didn't find your surname in fas. You need a sponsor, right?
> I will add the need sponsor blocker, if i am wrong, then please remove it.

As you've already found out, I don't need a sponsor.

> some issues:
> --
> %global snap 20090209
> why global instead of define?

https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define

And it's been ratified by FESCo already.

Although in this particular case it doesn't make a difference.

> --
> %{__make}
> please use commands like they are, not as a macro.
> make instead of %{__make}

OK, but what's wrong with using macros?

> --
> BuildRoot Tag
> https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
> your buildroot tag is not valid!

OK.

> --
> %defattr(644,root,root,755)
> please use %defattr(-,root,root,-)

Why?

> --
> makro couples 
> please use:
> $RPM_OPT_FLAGS and $RPM_BUILD_ROOT
> or
> %{buildroot} and %{optflags}
> please use one of this couples, but do not mix this.

You're picky, but OK.

> --
> BuildRequires: pidgin-devel
> Requires: pidgin
> if you have as BR pidgin-devel, then will rpm requires pidgin automaticly.
> you don't need Requires: pidgin

IIRC it won't, because plugins are dlopen()'d, but I'll re-check.

> --
> this is a tlen plugin, so it will be need 
> Requires: libtlen
> or?! I didn't test it, yet.  

No, it has its own implementation. I'm not sure if it makes sense to ask upstream to use libtlen because libtlen hasn't been updated in quite a while.

Comment 3 Simon 2009-03-27 11:39:24 UTC
> https://fedoraproject.org/wiki/PackagingDrafts/global_preferred_over_define
> And it's been ratified by FESCo already.
> Although in this particular case it doesn't make a difference.
https://fedoraproject.org/wiki/Packaging:GuidelinesTodo
should be okay.


>> please use commands like they are, not as a macro.
>> make instead of %{__make}
> OK, but what's wrong with using macros?
Nothing, it's just a sugestion. I would prefer make, so I tried to convince you. If you don't like it, you can use macros of course, if you want.


>> %defattr(644,root,root,755)
>> please use %defattr(-,root,root,-)
> Why?
This is the part you should answer why you are not using %defattr(-,root,root,-)
https://fedoraproject.org/wiki/Packaging/Guidelines#File_Permissions
If you have a good reason to differ from default, I will have no problem with this.


> You're picky, but OK.
I'm definitly not picky. :-)


> IIRC it won't, because plugins are dlopen()'d, but I'll re-check.
yeah i was wrong, sorry! my fault!!!


> No, it has its own implementation. I'm not sure if it makes sense to ask
> upstream to use libtlen because libtlen hasn't been updated in quite a while.
That puts another complexion on the matter.
https://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries


If you decide to patch it out, please comment your patch(es)
https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment

Comment 4 Christoph Wickert 2009-04-14 17:50:50 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > %defattr(644,root,root,755)
> > please use %defattr(-,root,root,-)
> 
> Why?

Because it's both in the packaging and the review guidelines.
http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions

> > makro couples 
> > please use:
> > $RPM_OPT_FLAGS and $RPM_BUILD_ROOT
> > or
> > %{buildroot} and %{optflags}
> > please use one of this couples, but do not mix this.
> 
> You're picky, but OK.

No he's not, it's also part of the guidelines:
http://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

Comment 5 Dominik 'Rathann' Mierzejewski 2009-04-14 21:47:11 UTC
(In reply to comment #3)
> >> please use commands like they are, not as a macro.
> >> make instead of %{__make}
> > OK, but what's wrong with using macros?
> Nothing, it's just a sugestion. I would prefer make, so I tried to convince
> you. If you don't like it, you can use macros of course, if you want.

Changed.

> >> %defattr(644,root,root,755)
> >> please use %defattr(-,root,root,-)
> > Why?
> This is the part you should answer why you are not using
> %defattr(-,root,root,-)
> https://fedoraproject.org/wiki/Packaging/Guidelines#File_Permissions
> If you have a good reason to differ from default, I will have no problem with
> this.

I don't. Changed.

> > No, it has its own implementation. I'm not sure if it makes sense to ask
> > upstream to use libtlen because libtlen hasn't been updated in quite a while.
> That puts another complexion on the matter.
> https://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries

It's not duplication, because the code is different (only auth.c is taken from an older release of libtlen). Anyway, I've sent upstream an email asking about libtlen usage.

http://rathann.fedorapeople.org/review/pidgin-tlen.spec
http://rathann.fedorapeople.org/review/pidgin-tlen-0-0.2.20090209.fc10.src.rpm

Comment 6 Simon 2009-04-30 13:05:18 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=490140
https://bugzilla.redhat.com/show_bug.cgi?id=495310

I will wait, because this topic is hot!

Comment 7 Simon 2009-06-30 05:10:45 UTC
2 months without response.
i will close this. please feel free to reopen it again, without a local copy of libtlen

Comment 8 Dominik 'Rathann' Mierzejewski 2009-08-21 19:27:40 UTC
(In reply to comment #7)
> 2 months without response.

Yeah, sorry about that. I got tangled up in real-life.

> i will close this. please feel free to reopen it again, without a local copy of
> libtlen  

I got a reply from upstream. They said they use only a some parts of libtlen and that it's not feasible to use the external library.

Comment 9 Simon 2009-09-29 09:26:29 UTC
okay, I noted that before.
fact is, you can't use an internal, even it is forked...

Comment 10 Simon 2009-10-09 05:01:59 UTC
Ask upstream to create a tarball of the forked libtlen, pacakge it, open a review request, import it to fedora and build your plugin against it.
this won't be a problem..
If upstream don't create a tarball. You can do it and upload it to your fedorapeople space and perhaps a little html document with some descriptions or so (for URL-tag ins pec, a directory listing isn't a good page. people like content). then you can do the steps i discribed...

Comment 11 Dominik 'Rathann' Mierzejewski 2009-10-13 21:13:06 UTC
(In reply to comment #10)
> Ask upstream to create a tarball of the forked libtlen, pacakge it, open a
> review request, import it to fedora and build your plugin against it.
> this won't be a problem..

There's no "forked libtlen" in this plugin. Upstream has simply taken some code (a header file and some functions IIRC) from libtlen, nothing more.

> If upstream don't create a tarball. You can do it and upload it to your
> fedorapeople space and perhaps a little html document with some descriptions or
> so (for URL-tag ins pec, a directory listing isn't a good page. people like
> content). then you can do the steps i discribed...  

I could probably try to port this plugin to use external libtlen, but I don't have time for that right now.


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