Bug#822107: RFS: eja/9.3.12-1 [ITP]
Hi Gianfranco,
debian directory review:
control:
1) unsecure VCS uri
replaced with https://github.com/ubaldus/eja.git
2) outdated std-version
updated to 3.9.8
3) wrap-and-sort please (just run the command wrap-and-sort)
done
4) extended description too short, and wrap to 80 column please
wrapped to 80 columns and I hope improved the description.
5) "Version" field is wrong and useless, please remove
removed
changelog:
6) one single entry with
"Initial release: closes: #ITP_BUG"
(open an ITP bug)
7) refresh signature timestamp
8) target suite is unstable
ok, bug is #824919
9) *.dirs <-- remove please and install directories inside the makefile
ok
10) copyright: is not mentioning all the copyrights, e.g. lua, some
gpl2 code? and probably more
probably repack the source to exclude embedded lua, or patch the
copyright file
http://metadata.ftp-master.debian.org/changelogs/main/l/lua5.2/unstable_copyright
I hope I added every licence and holder.
build review:
11) custom makefile :( (just sad, with 10 lines of cmake file you can
avoid all this custom stuff probably)
I promise I will study cmake and try to improve :)
12) embedded lua :( (you should detect it, and use the embedded if no
system lua is provided)
I hope I did it in the right way, I tried to compile with and without
already installed Lua and it seems to be working fine.
13) no debug -g option (please add it, and let dh_strip strip the dbg
symbols)
I added the -g option but I could not find any way to run the dh_strip,
is this automatic?
14) I would appreciate a make install working regardless of debian or
not.
(just find the lua at build time in a if statement)
"$(CC) $(CFLAGS) -o eja eja.c $(shell pkg-config --libs --cflags
lua5.2) -lm $(MYLIBS)"
this is wrong in general, specially when wl-asneeded flag is the
default in gcc
$(CC) $(CFLAGS) -o eja eja.c $(shell pkg-config --cflags lua5.2) -lm
$(MYLIBS) $(shell pkg-config --libs lua5.2)
^^ this might work instead.
(still sad, but working)
I hope I did it in the right way, it seems to be working fine on my
environment.
15) install something like this?
override_dh_auto_install:
dh_auto_install -- prefix=/usr
ok
16) (eja.h... what?)
this file is needed by eja.c to "embed" the Lua files in lib/*.lua and
has to be created on the fly, I mean this is the only way I found out to
be working fine but of course I am open to any better suggestion.
17) missing harderning flags (google for them)
http://debomatic-amd64.debian.net/distribution#unstable/eja/9.3.12-1/blhc
I am not sure if this is the right solution but I added them in
debian/rules.
18) missing watch file
I hope I have done it right.
19) I: eja: spelling-error-in-binary usr/bin/eja seperator separator
replaced (was in json.lua).
20) I: eja: description-synopsis-might-not-be-phrased-properly
should be better now.
21) I: eja: package-contains-empty-directory usr/lib/eja/
should also be removed after eja.dirs deletion.
(all from
http://debomatic-amd64.debian.net/distribution#unstable/eja/9.3.12-1/lintian)
(I removed the lua directory prior to run check-all-the-things)
apt-get install -t experimental check-all-the-things
$ codespell --quiet-level=3
<some>
I tried the same and it complains about helpFull/helpful but indeed
helpFull is the name of the option meaning "full help".
# Parsing /proc/cpuinfo is not portable at all, use /sys instead.
$ grep -rF /proc/cpuinfo .
./lib/proc.lua: local procCpuInfo=ejaFileRead('/proc/cpuinfo')
honestly this library is really for /proc access but I found out I could
calculate the total amount of cores from /sys so I added it as first
option when counting them.
$ luacheck -q .
<lots>
$ grep -r '/tmp/' .
$ find -type f \( -iname '*.c' -o -iname '*.cc' -o -iname '*.cxx' -o
-iname '*.cpp' -o -iname '*.h' -o -iname '*.hh' -o -iname '*.hxx' -o
-iname '*.hpp' \) -exec pmccabe {} + | sort -nr
<some>
test -d ./debian &&
! grep -s -q native debian/source/format &&
! test -e debian/upstream/metadata &&
echo 'Please add some upstream metadata:
https://wiki.debian.org/UpstreamMetadata'
Please add some upstream metadata:
https://wiki.debian.org/UpstreamMetadata
I could not run these tests, I will try again later but if you already
have the output maybe it would help me to sort it out.
this should be enough as first review :)
please don't be scared, most of them needs some seconds of work.
Thank you very much for your time :)
Reply to: