On 07/05/2016 08:02 AM, Sean Whitton wrote:
> control: owner -1 !
> control: tag -1 +moreinfo
>
> Dear Jacob,
>
> This looks like a nice alternative to redshift-gtk. Thanks for
> packaging it. I can't sponsor the upload, but I hope this review is
> useful to you.
Thanks! The review definitely helped.
>
> 1. Could you explain why you are packaging your fork rather then the
> original? (This kind of thing should go in the ITP.)
The original was released in a blog post as a sort of code dump
(http://www.tedunangst.com/flak/post/sct-set-color-temperature )
I thought it would improve long term maintainability if there was a real
build system and active upstream. It's pretty hard to package software
without a tarball or VCS. If this is discouraged, I'd be happy to email
the real upstream about it, but I simply thought this would be easier
for everyone.
If I need to send a mail explaining this to the ITP I can do that.
>
> 2. Could you put your Debian packaging in git, please? It makes
> reviewing easier. Perhaps as a 'debian' branch of your repo.
Whoops. Actually I already do this in a different git repo and I just
filled out the relevant fields of d/control incorrectly. Thanks for
catching that!
>
> 3. The formatting of the long description in debian/control is a bit
> strange. Please separate paragraphs using a line with the string
> " .". Probably best to wrap at 70 chars, too.
I think I fixed this now.
>
> 4. The wording of the long description could be improved. The first
> sentence isn't really a sentence -- it would be better to write "sct
> is a small C program to change the screen color temperature. It can
> be used to reduce or increase the amount of blue light produced by
> the screen." Please take another look at your wording :)
Wow that description was a mess. I've fixed it up now and I think it's
much clearer.
> 5. Have you considered calling the binary package 'sct'? That is what
> someone might guess when they want to install this with apt-get.
That's a good idea. I just changed it.
>
> 6. 'sct' is a very short command name for /usr/bin ... have you
> confirmed that it doesn't clash with any other packages in Debian?
> You might have to set the priority to 'extra'.
Is there any way to check if other packages have binaries called sct? A
quick search of packages.d.o would seem to indicate there is not but is
there a better way to check?
>
> 7. The language in d/copyright ("I doubt if it's copyrightable" etc.)
> isn't appropriate. You need to determine whether or not it is
> copyrightable and make a clear statement of that.
That wording is not mine, but written by Ingo Thies, who would have
copyright over the whitepoints data if it is copyrightable. Putting it
there was the advice I got from debian-legal (see below).
>
> 8. This doesn't make sense (doesn't follow DEP-5 machine-readable
> copyright file format) -- please check:
>
> Files: sct.c
> Copyright: 2016 Ted Unangst <tedu@openbsd.org>
> whitepoints data copyright 2013 Ingo Thies <ithies@astro.uni-bonn.de>
> License: public-domain-sct and public-domain-colorramp
This was based off of a discussion in debian-legal (starts here:
https://lists.debian.org/debian-legal/2016/06/msg00018.html ). They did
not explain how to properly format this except to say I should
explicitly put the license grant from Ingo (the "I doubt if it's
copyrightable" stuff) into d/copyright. I have no idea how to properly
format this.Any ideas you have on how to write this up correctly are
welcome.
I've read DEP-5 but this is complicated because part of sct.c is
copyright Ingo Thies but the rest is copyright Ted Unangst.
>
> 9. Please install the README into /usr/share/doc.
Done.
> 10. You're missing at least one build dependency. Please try building
> in a clean sid chroot (see the pbuilder or sbuild tools).
Yep. Was missing libx11-dev and libxrandr-dev (dpkg -S is the best :) ).
I've fixed it now. Thanks for finding that.
Thanks again for the review! I've now uploaded a new package to mentors.
--
Jacob Adams
GPG Key: AF6B 1C26 E2D0 A988 432B 94F4 24C0 2B85 B59F E5A9
Attachment:
signature.asc
Description: OpenPGP digital signature