On Tue, 19 May 2009 19:46:46 +0530
Y Giridhar Appaji Nag <appaji@debian.org> wrote:
> I took a look at the package and have a bunch of comments:
>
> - It is not necessary that README.source be installed, README.source is for
> source packages only.
Fixed in collab-maint.
By the way, how am i supposed to upload a fixed version of the package to
mentors.debian.net without adding a spurious changelog entry?
> - Any reason why you call ./debian/rules unpatch and not just depend on the
> unpatch target?
So that calling `make clean' executes the clean target of the patched
Makefile.
It isn't really needed at the moment, but were I to further patch the
Makefile, I wouldn't need to modify the rules file. Moreover, since the
build system is patched right before building, it seems correct to remove
the patches right after clean.
> - Per policy, binary-arch and binary-indep are optional. In your case there
> aren't complex arch/indep parts, so you can just have a binary: target.
I think you're mistaking binary-{arch,indep} for build-{arch,indep}. While
the latter are optional, the former are required.
> - It might be a good idea to use a examples file rather than listing all the
> files installed as examples (you have more than one or two example files).
I tried creating an examples file, but neither debian/examples nor
debian/scrotwm.examples did the trick. Can you confirm this?
> - apm executable in Debian is in /usr/bin and not in /usr/sbin. Should you
> Patch baraction.sh appropriately?
>
> - There are references to a "scrot" program in screenshot.sh, where does one
> find that program?
I'm not sure it's worth it to patch the examples.
I've patched the system-wide configuration file so that Scrotwm can be run
without any kind of configuration, but the example files are provided just
as guideline.
> - Since spawn_term uses x-terminal-emulator, you would want to Recommend
> x-terminal-emulator | xterm?
I Recommend dwm-tools for a similar reason, so it makes sense.
Fixed in collab-maint.
> - Are you recommending xfonts-terminus package because terminus-medium is the
> default setting in scrotwm.conf? Just wondering if you could downgrade that
> to a suggests.
Based on my tests, some fonts might not work. Moreover, as I explained above,
I'd like the window manager to be usable right after installation without the
need for further configuration.
> The package looks neat otherwise, thank you for the good work :)
Thank you for taking the time to review it!
--
Andrea Bolognani <eof@kiyuko.org>
Resistance is futile, you will be garbage collected.
Attachment:
pgpVKpfR5e9Hj.pgp
Description: PGP signature