Hi Andreas, I appreciate your review. See my comments below.
On 25/12/2016 13:21, Andreas Henriksson wrote:
Hello Lucio Correia,
On Thu, Dec 15, 2016 at 02:58:46PM -0200, Lucio Correia wrote:
Breno,
I've upload a new package with requested fixes.
dget -x https://mentors.debian.net/debian/pool/main/w/wok/wok_2.3.0-1.dsc
Let me know if anything else is necessary for wok.
I took a quick look at this package and here are a few notes I made.
I don't personally intend to sponsor you as this seems to be outside
my expertice.
- Seems like you've made an effort both to remove and use/depend
on packaged javascript components like jquery etc, as well as
adding missing sources for the minified bundled things.
This should avoid the need for repackaging the orig tarball
to remove source-less files, but at the same time it's a bit hard
to check if you catched everything. Also, while I guess it's not
strictly forbidden to bundle javascripts that aren't already packaged
separately I think it's atleast frowned upon. You might want to
consider joining in on the javascript packaging efforts and get
(all of) your javascript dependencies packaged separately.
OK, I will try to do it for the ones we don't touch. There are some libs
that we made changes to (either js, css or html) so we will continue to
ship those ones into our packages.
- postinst pokes around in /etc and that is always a source for finding
policy violating / release critical bugs. For example you check
only for configure stage and enables the nginx site. This means
you'll likely trigger on package upgrades and reenable a site that
the local administrator might have disabled. Fix would likely
be to check $2 for version to detect if configure is running under
upgrade or newly-installed-package.
OK
- postrm removes files without checking ownership.
I think it's ok for the purge action to be brutal, but the remove
action should under no conditions cause harm to potentially locally
configured things. For example you better check if sites-enable/wok
is a symlink before removing it. The local admin might have
replaced any of the files you installed under /etc with his own
so removing them without very careful checking is likely to not
end well.
OK
- consider dropping the conditional clauses for invoke-rc.d in both
your postinst and postrm. The invoke-rc.d has been around for many
releases and should always be available (or it would be a bug in
the base system). I'm aware policy contains similar to what you're
doing but policy is simply severely outdated. Directly invoking
an init.d script can be potentially harmful and should never
be done in todays system. (Policy even explicitly forbids it.)
OK, will consider dropping
- 0001-Do-not-install-firewalld-conf-in-Debian.patch
why? Please always document why.
(fwiw, firewalld is available in debian also.)
OK, I will readd it
- 0001-Rename-wokd-to-wok.patch
why?
I kept this from Frederic's initial work on kimchi packages. At that
time, it seems lintian complained about having different names for init
file and service file.
Anyway, I would prefer to keep wokd as in upstream.
- 0001-Update-Datatables-Switch-and-Editable-libraries.patch
patch is full of trailing whitespace addition noise.
OK
- 0005-Add-nginx.service-as-wokd.service-dependency.patch
Executing kill in ExecStop is bad as it's async. This is bad
and potentially causes 'restart' to fail as 'stop' action
will not wait for stop to actually finish.
OK
HTH.
Regards,
Andreas Henriksson
PS. you might want to block ginger RFS on ginger-base RFS and
ginger-base RFS on wok RFS.
OK
I will upload new packages after fixing these.
Thanks!
--
Lucio Correia
