Review of lightread

[Didier Roche]

Le 06/08/2012 16:22, Stéphane Graber a écrit :
> 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.
>
You don't really want this file anyway as you don't have the right 
symlink to the pyshared area (in /usr) and so can just support one 
version of .pyc, not multiple of them: so one python version only. And 
as this is a ARB requirement to rebuild the application on each 
supported release, the runtime file is useless as I doubt we will 
introduce a newer python version in precise and it will be rebuilt on 
quantal.