Re: Request to review and upload unhide 20210124-2
Hello Daichi,
To make the review easier and avoiding possible conflicts, I suggest
that you rebase your changes on the main repo, I have done this on my
side for now but it's a good practice that will save you from a
headache:
git checkout debian/master
git remote add team git@salsa.debian.org:pkg-security-team/unhide.git
git fetch team
git rebase team
> * Fix path for python3 interpreter which unhideGui.py invokes
https://salsa.debian.org/dfukui/unhide/-/commit/2543fb0034d238df0ad1e94856522faba7bdbba4
I can see that without this patch, the following lintian gets thrown:
E: unhide: wrong-path-for-interpreter /bin/python3 != /usr/bin/python3
[usr/lib/unhide/unhideGui.py]
That seems like an outdated lintian finding, since starting on
bookworm we can assume that all systems have the merged-usr root
filesystem layout, so this shouldn't matter.
But in any case, a patch is fine.
I recommend changing the "Copyright" part of the patch in favor of a
DEP3 header, you can use "Author":
https://dep-team.pages.debian.net/deps/dep3/
> The most important part to be discussed would be how to incorporate a private python module ToolTip.py [2] into the unhide deb package.
> Note that only unhideGui.py is supposed to use the module.
> Referring to a similar discussion [3], I am trying to install ToolTip.py and unhideGui.py to /usr/lib/unhide/ and then symlink the latter one to /usr/sbin/unhideGui.
> What do you think about this change? Do you think the change above makes sense?
I think that's fine, I'd be interested to hear better alternatives if
anybody knows it, but at least we know this will work with no issues.
> By the way, the new source package was tested using salsa CI and all jobs successfully passed [4].
Great.
I got the time to checkout unhideGui in more detail now:
It looks like parts of it are in French, with no support for translations:
https://github.com/YJesus/Unhide/blob/master/unhideGui.py#L291
https://github.com/YJesus/Unhide/blob/master/unhideGui.py#L294
https://github.com/YJesus/Unhide/blob/master/unhideGui.py#L303
This worries me a bit since it might mean that the GUI is not up to
standards for packaging it on Debian.
I could be wrong, so let me know if you feel otherwise. I'm now
thinking we should not ship it with the package but I wouldn't oppose
it if anybody acks these issues and says it would still benefit our
users.
I'm gonna still review the inclusion of the GUI nonetheless, as it
might be useful:
1) desktop file:
For GUI applications it's always a good idea to provide a .desktop
file, so users can start it from their DEs. But in the case of unhide,
the GUI would need to change to identify when it's not running as root
and call polkit (or something similar) to get the privileges.
You don't need to do this, but it's a nice addition for the future
(and if we end up shipping the GUI).
2) unhideGui:
I think the name of the binary could be improved to try to follow the
pattern we usually see on unix tools (and in the other binaries of
unhide), maybe unhide-gui (all lowercase)?
3) commit:
There's just a single commit which is adding the GUI, the ToolTip and
a few other things:
https://salsa.debian.org/dfukui/unhide/-/commit/b095ddb22e6c1ad3d403f7ae0c244b3a1a76cc36
I suggest either trying to break down changes like these into multiple
commits or writting a bullet-point summary of all the changes of that
commit in its description.
There are a couple of changes which might have been done by mistake or
are unneeded:
3.1) add-new-filename-ToolTip.py.patch:
https://salsa.debian.org/dfukui/unhide/-/commit/b095ddb22e6c1ad3d403f7ae0c244b3a1a76cc36#9c7d4d4cec790969b52d1b790c004d070274f8d6_0_1
I don't think this patch is needed, as "tar_list" is only used by
upstream to generate the tarball.
3.2) allocate-pid-arrays-from-heap.patch:
https://salsa.debian.org/dfukui/unhide/-/commit/b095ddb22e6c1ad3d403f7ae0c244b3a1a76cc36#1f4bc5bbd803ea3626eeb2d591d190112c858dad_1_1
The changes to these patch were probably done automatically by some
tool, they're not needed as the patch already has the DEP-3 headers.
Thank you for the patience waiting for the review, I try to always
reply within 7 days but I didn't have a lot of free time lately. As a
rule of thumb, feel free to always ping me after 7 days if you get no
reply.
Cheers,
--
Samuel Henrique <samueloph>
Reply to: