Bug 485638

This is an un-official pre-review.

- May I suggest append "%{dist}" at the end of release string.

(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

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.

Thanks for suggestions.
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-3.fc10.src.rpm
New mock log: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/build.log
rpmlint doesn't have any complaints

I've send the patch to upstream (mailing list)
http://lists.suckless.org/dwm/0904/7801.html

4.0 was released apparently.

Please don't use %{version} in the patch filenames since it breaks when bumping version.

Updated to 4.0 and corrected the patch filename.

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-1.fc10.src.rpm
New mock log: http://www.stud.fit.vutbr.cz/~xblaze17/packages/dmenu/build.log

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.

(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

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

(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 :-)

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

(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.

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.

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.

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.

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.

(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.

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

Jan,

I'm getting a 403 error when I try to download your srpm :(

(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

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

*** Bug 529253 has been marked as a duplicate of this bug. ***

What's the current state of this bug?

Sorry, but I'm quite busy ATM. If someone wants to pick up this package feel free to do it.

Then please close this bug and i will open a new one....
I need this pkg for my i3 desktop