Re: RFS: wmaker
On Sun, Nov 13, 2011 at 5:43 PM, Rodolfo kix Garcia <kix@kix.es> wrote:
> Hi Paul,
>
> thanks a lot for your review!
Of course!
>
> On 13/11/2011 20:25, Paul Tagliamonte wrote:
>> INADD, but here's a review,
>>
>> General note:
>> - You've got a lot of stuff in ./debian/ - I would invite you to
>> consider creating a few directories to put files that relate to
>> eachother (theme related stuff, and so forth)
>
> I took the wmaker package from the previous maintainer, and I try to
> update it to the current standard-version, dep-3/5,... Some things are
> adopted from the previous package.
>
> I have a lot of files in ./debian because this package creates nine or
> ten binary packages. The files are *.install, *.symbols, *.docs,...
>
> But probably yes, I can move/delete some files:
>
> delete: TODO, nightly_build.sh
> move: WMWindowAttributes, debian.tiff.uu, appearance.menu-method,
> Debian.theme
>
>> You're overriding a lot of stuff. Can you please explain them all?
>> Some of them look valid...
>
> First, I don't know how to solve these problems.
> I try to use "http://www.debian.org/doc/packaging-manuals/menu.html/"; to
> create/update the windowmaker menu's. But Window Maker is an exception
> and is not too easy to create the menus. In
> http://www.debian.org/doc/packaging-manuals/menu.html/ch3.html you can find:
>
> Window Maker
> This section is reserved for wmaker as a special case.
> All wmaker specific entries must go here.
>
> For this reason we have these overrides. I try to change some things,
> but then the WindowMaker menu system fails :-(
>
>> You should consider putting a comment block by each override, and note
>> exactly why. If lintian's in error, please report a bug (and don't
>> override!)
>
> I think a little on this. IMO wmaker is an exception, and probably is
> easy to continue with the overrides and don't modify lintian.
If this is true (and it might be) - you need to explain why you need
to override, and why modifying lintian would not be a good idea, as a
comment in that file :)
>
>> wmaker: menu-item-creates-new-root-section
>> wmaker: menu-command-not-in-package
>> wmaker: menu-item-contains-unknown-tag
>> wmaker: menu-item-needs-tag-has-unknown-value
>>
>> ./debian/debian.tiff.uu
>> - You can avoid uuencoding with source format 3 (as you're using)
>
> I will try to find more info about it.
>
>> ./debian/nightly_build.sh
>> - what is this, and why do we need it in the archive? :)
>
> Yes, this file is used in the webpage to create a nighty build :-). Of
> course should be removed from the Debian package.
>
>> ./debian/TODO
>> - Consider keeping track of this in the BTS
>
> Should be removed too. The TODO info don't apply now.
Radical :)
>
>> ./debian/copyright
>> You say:
>> License: public-domain
>>
>> but then you go on to say:
>>
>> They may be distributed freely and/or modified as long as the original
>> Author is mentioned!
>>
>> That's not public domain :)
>>
>> Also:
>> License: public-domain
>> do What The Fuck you want to Public License
>
> WTF license!! ;-) I try to select one license, but I found nothing.
> http://dep.debian.net/deps/dep5/ What should I do?
I'm not sure. I'll pass this off to someone who knows what they're doing :)
>
>> Lastly:
>> you have a GPL-2 block without files.
>
> No. Is the License paragraph. Is not correct?
Ah, I see what you did. That's for the bit up on the top.
I seem to recall that not being right, but I'm not sure :)
>
>> ./debian/rules
>> I've never seen such a large dh-short rules file before.
>
> I can write a book about it. It took a lot of time. Was hard. Probably
> is a good idea to review some parts.
I had to do the same a few weeks back. It's a lot of work, but it's
worth it to squash d/rules often :)
>
>> Consider some of the following:
>>
>> XLOCALE := --disable-locale
>> [snip]
>> WMAKER_OPTIONS := $(XLOCALE) $(MODELOCK) $(XINERAMA) \
>> [snip]
>>
>> You're going to have to modify this file anyway, why not keep a
>> well-formatted list of the args directly.
>
> The package, in the previous version, had this info. Are "possible"
> options, then we select only a few of them:
>
> XINERAMA := --enable-xinerama
> # USERMENU := --enable-usermenu
>
> XINERAMA is used, USERMENU is not used, but is an option...
>
> I don't have preferences about it. If you think is better put the args
> directly, perfect.
>
> I know that I will send this package some times to mentors :-( It has
> many many thinks.
Of course. The sponsoring DD might have more of an opinion, but for
me, it seems like that uses up twice the space for no additional gain.
Whatever :)
>
>> There's more, but I think it just needs a good hard think on what
>> really needs to be in there.
>> I mean, it's huge.
>>
>> I didn't get a chance to get too much farther then this, I just ran
>> out of laptop battery. Someone else can take over - it looks like this
>> needs a good stern clean.
>
> Thanks a lot for your time. Your comments are very good. Probably a
> second opinion should be nice, because this package is a little bit "rare"
>
I have a package in similar shape. I know what it's like :)
>> Other then that, it's in fairly nice shape. Well done on maintaining
>> such a huge package.
>
> Thanks, your comments are very welcome!
>
>> Overrides aside, lintian reports one source pedant:
>
> I don't remember, but if I remove the quilt from the build-depends, I
> got an error about quilt dependence. But I am not sure.
Yeah, I've not had enough time to run through it again. I seem to
recall quilt being in debian/rules for some reason I didn't grok at
the time.
>
>>
>> P: wmaker source: unneeded-build-dep-on-quilt
>> N:
>> N: This package build-depends on quilt, which is not required since
>> N: dpkg-source will apply patches at unpack time for 3.0 (quilt) source
>> N: packages.
>> N:
>> N: Remember to remove any references to quilt in the rules file (e.g.
>> N: "--with quilt", dh_quilt_* or quilt makefile includes).
>> N:
>> N: This tag is meant to remind people that with 3.0 (quilt), quilt is not
>> N: necessary. If you keep the build-depends on quilt to ease backporting to
>> N: older releases, then please ignore/override this tag.
>> N:
>> N: Severity: pedantic, Certainty: possible
>> N:
>> N: Check: patch-systems, Type: source
>> N:
>>
>> Have a super great day,
>> Paul
>
> Thanks a lot!
>
> Best Regards,
> kix.
>
>
Best of luck!
Paul
--
All programmers are playwrights, and all computers are lousy actors.
#define sizeof(x) rand()
:wq
Reply to: