Review of lightread

[Stéphane Graber]

On 08/06/2012 09:48 AM, Didier Roche wrote:
> Le 06/08/2012 15:39, Stéphane Graber a écrit :
>> On 08/06/2012 09:23 AM, Didier Roche wrote:
>>> Hey guys,
>>>
>>> As dpm asked me, I did a review of lightread source code and packaging
>>> themselves.
>>>
>>> The code is cleaned, using sass and some popular javascript libraries,
>>> clearly showing the guy know what he is doing. I just removed an
>>> harmless .DS_Store which isn't useful in this context, as well as a
>>> readabilty.js unsued file. Even if it's unused, the goal of this file is
>>> to use a third-party service to prettify the blog entry. There is a
>>> token and so on. However, this one isn't even called, so I felt better
>>> to completely remove it (which is just about removing that file right
>>> now).
>>>
>>> https://code.launchpad.net/~didrocks/ubuntu-app-reviews/lightread
>>>
>>> The packaging itself works from /opt and comply the standards of the
>>> ARB. So a definitive +1 from me.
>>>
>>> Cheers,
>>> Didier
>> Just spent 5 minutes reviewing it here too.
>>
>>   - debian/changelog will need update prior to upload (version number)
> What should it be updated to? (the version seems to make sense to me)
>>   - lintian spotted two small problems:
>> W: lightread: executable-not-elf-or-script
>> opt/extras.ubuntu.com/lightread/share/lightread/media/app/scripts/lib/keymaster.js
>>
>> W: lightread: executable-not-elf-or-script
>> opt/extras.ubuntu.com/lightread/share/lightread/media/app/manifest.json
> As I've already the above branch prepared, I've removed the executable
> bits (hard to spot as /opt is not supported at all by lintian and we
> already get tons of lintian warnings because of the platform which has
> never been prepared to /opt…)
>>   - As already mentioned, the package ships a runtime.d directory but
>> without the usual hook in there. The hook should be included or the
>> directory removed.
> 
> This hook is created automatically by dh_python. It's useless, right,
> with /opt as nobody before taking the decision of mandating /opt made
> sure the platform was prepared with it. Apart from having an even more
> hackish debian/rules, I don't think we should bother about that one.
> 
> changes pushed to my previous url.
> Cheers,
> Didier

I have seen a bunch of other packages going through the ARB that
actually had a valid runtime.d hook running pycompile on the file in
/opt/ which seems to be what we'd want in most cases?

The packaging currently uses:
»···if [ -f
debian/lightread/usr/share/python/runtime.d/lightread.rtupdate ]; then \
»···»···rm debian/lightread/usr/share/python/runtime.d/lightread.rtupdate; \
»···fi

I believe removing these altogether should work, or it should be changed
to an "rm -rf debian/lightread/usr/share/python" which would technically
be shorter and cleaner than the current code.

-- 
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 900 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/app-review-board/attachments/20120806/7140ea7a/attachment-0001.pgp>