Bug 485638 - Review Request: dmenu - Dynamic X menu
Review Request: dmenu - Dynamic X menu
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-15 13:41 EST by Jan Blazek
Modified: 2009-12-11 07:55 EST (History)
12 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-12-11 07:55:37 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Jan Blazek 2009-02-15 13:41:19 EST
Spec URL: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu.spec
SRPM URL: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu-3.9-1.src.rpm

Description: 

Dynamic menu is a generic menu for X, originally
designed for dwm. It manages huge amounts (up to
10.000 and more) of user defined menu items
efficiently.

rpmlint output: 3 packages and 1 specfiles checked; 0 errors, 0 warnings.
mock build log: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/build.log

Please note that this is my first fedora package and I am seeking a sponsor.
Comment 1 Suravee Suthikulpanit 2009-02-18 16:02:50 EST
This is an un-official pre-review.

- May I suggest append "%{dist}" at the end of release string.
Comment 2 Jan Blazek 2009-02-21 03:29:17 EST
(In reply to comment #1)
> This is an un-official pre-review.
> 
> - May I suggest append "%{dist}" at the end of release string.

Thank you. It's repaired now.

New SPEC: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu.spec
New SRPM: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu-3.9-2.fc10.src.rpm
Comment 3 Till Maas 2009-03-31 07:24:57 EDT
Imho it is better to use a patch instead of the sed commands and modify the config.mk in a way that you can pass the %{optflags} on the commandline. This is easier to read and can be submitted upstream.

I also recommend to remove the "@"-signs from the Makefile to see which commands are actually run, which helps to verify in the build.log, whether or not the %optflags are really honoured:
sed -i 's/\t@/\t/' Makefile

Btw. strncat is not properly used in dmenu.c, whis is shown in a compiler warning:
CC dmenu.c                                                                                                                                                                          
In function 'strncat',                                                                                                                                                              
    inlined from 'kpress' at dmenu.c:400,                                                                                                                                           
    inlined from 'run' at dmenu.c:562,                                                                                                                                              
    inlined from 'main' at dmenu.c:725:                                                                                                                                             
/usr/include/bits/string3.h:153: warning: call to __builtin___strncat_chk might overflow destination buffer
Here is a patch by me:
http://till.fedorapeople.org/files/dmenu-3.9-strncat.patch
There is no need to submit it upstream, because they removed the strncat.
Comment 5 Jan Blazek 2009-04-08 08:52:17 EDT
I've send the patch to upstream (mailing list)
http://lists.suckless.org/dwm/0904/7801.html
Comment 6 Jens Petersen 2009-04-19 06:14:26 EDT
4.0 was released apparently.
Comment 7 Jens Petersen 2009-04-19 06:29:46 EDT
Please don't use %{version} in the patch filenames since it breaks when bumping version.
Comment 9 Till Maas 2009-04-19 15:28:10 EDT
Sorry, I meant to reply earlier. The LDFLAGS now contain "-s", which strips the binaries. This should also be changed, maybe with advancing the Makefile patch to also allow to remove the -s flag.
Comment 10 Jan Blazek 2009-04-19 17:43:23 EDT
(In reply to comment #9)
> Sorry, I meant to reply earlier. The LDFLAGS now contain "-s", which strips the
> binaries. This should also be changed, maybe with advancing the Makefile patch
> to also allow to remove the -s flag.

Is it OK to make one patch for both LDFLAGS and OPTFLAGS?

New SPEC: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu.spec
New SRPM:
http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu-4.0-2.fc10.src.rpm
New mock log: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/build.log
Comment 11 Chess Griffin 2009-06-23 23:03:52 EDT
Hello-

I cannot give you an official review, but here are a few minor nits:  

You have "Source:" and "Patch0:" (one has 0 and one does not).  The documentation[1][2] references "Source0:" even there is only one source download.  Either way, they should be consistent, so you may want to change "Source:" to "Source0:".

Additionally, (and this is a very minor quibble), you have some parts of the spec file with one blank line between sections and other parts with two.  Personally, I would stick with one style or the other.  It just looks cleaner.

FWIW, I would love to see this package in Fedora.  :-)

[1] http://fedoraproject.org/wiki/PackageMaintainers/CreatingPackageHowTo
[2] http://fedoraproject.org/wiki/Packaging/SourceURL
Comment 12 Jan Blazek 2009-06-29 16:14:01 EDT
(In reply to comment #11)
Hi,

thank you your suggestions. You're right about the blank line consistency and Source vs Source0, although it doesn't affect the functionality.

New SPEC: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu.spec
New SRPM: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/dmenu-4.0-3.fc10.src.rpm

PS: I'd also love to see this package in Fedora :-)
Comment 13 Rahul Sundaram 2009-08-13 11:27:12 EDT
Remove the buildroot definition and you dont need to clean it in %install section anymore.

https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

Add a comment explaining what the patch does and it's upstream status

https://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment
Comment 14 Till Maas 2009-08-13 11:40:32 EDT
(In reply to comment #13)
> Remove the buildroot definition and you dont need to clean it in %install
> section anymore.
> 
> https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

To be more precise: It's is only not needed, if the package is not going into EPEL 5 or 4.
Comment 15 Rahul Sundaram 2009-08-14 07:17:41 EDT
Right but then I would suggest that you keep them only in the EL branches and remove unnecessary cruft from the Fedora branches but that's left to the maintainer.
Comment 16 Rahul Sundaram 2009-08-14 07:20:39 EDT
Didn't realize this one requires a sponsor. Taking myself off as the reviewer although I can review it unofficially and make it easy for the sponsor.
Comment 17 Susi Lehtola 2009-08-14 08:02:49 EDT
I don't agree. I think keeping the BuildRoot stuff doesn't hurt anything: it's just a few lines, and it avoids using conditionals that are a lot easier to get wrong. Having one and only spec file for the different branches is a plus, also.

**

- The %description line fits on two lines. No need to use four.

- Any reason why you are not using SMP make? If it doesn't work, please document it.

- Your patch doesn't have a comment. Add one explaining what it does and why it is necessary.

- I think you are missing a Requires: on the package that provides the font dmenu uses.

**

I am a sponsor, so I can sponsor you if necessary. You just need to do a few informal reviews first.
Comment 18 Mamoru TASAKA 2009-09-10 13:23:46 EDT
Please assign this bug to someone who is actually reviewing this
review ticket _formally_ , which must be a sponsor and set fedora-cvs
flag properly.
Comment 19 Till Maas 2009-09-10 14:43:04 EDT
(In reply to comment #18)
> Please assign this bug to someone who is actually reviewing this
> review ticket _formally_ , which must be a sponsor and set fedora-cvs
> flag properly.  

Nobody is reviewing this, therefore I reset thes fedora-review flag, which you probably meant with fedora-cvs flag.
Comment 20 Till Maas 2009-09-19 08:35:23 EDT
Jan, did you perform some inofficial reviews, like one is advised on the how to get sponsored wiki page[0]?

[0] https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Reviewing_Packages
Comment 21 Jason Woofenden 2009-10-03 16:18:59 EDT
Jan,

I'm getting a 403 error when I try to download your srpm :(
Comment 22 Jan Blazek 2009-10-03 16:36:17 EDT
(In reply to comment #21)
> Jan,
> 
> I'm getting a 403 error when I try to download your srpm :(  

Sorry, that account doesn't work anymore. I've moved it to new url and added bitmap-fonts to Requires.

http://appolito.ic.cz/packages/dmenu-4.0-4.fc11.src.rpm
http://appolito.ic.cz/packages/dmenu.spec
Comment 23 Till Maas 2009-10-14 13:54:19 EDT
Jan, I still cannot find any inofficial reviews from you, like I stated in comment:20. I highly doubt it, that someone will move forward and sponsor you, without you doing these inofficial reviews, like it is recommended on the "how to get sponsored wiki page"[0].

[0]
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group#Reviewing_Packages
Comment 24 Jason Tibbitts 2009-10-15 18:21:14 EDT
*** Bug 529253 has been marked as a duplicate of this bug. ***
Comment 25 Simon 2009-12-11 07:32:42 EST
What's the current state of this bug?
Comment 26 Jan Blazek 2009-12-11 07:46:34 EST
Sorry, but I'm quite busy ATM. If someone wants to pick up this package feel free to do it.
Comment 27 Simon 2009-12-11 07:49:45 EST
Then please close this bug and i will open a new one....
I need this pkg for my i3 desktop

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