Re: Request for review/upload of regripper 3.0-1
Hello,
I have looked at regripper. Here are my comments/questions:
In the README I see this:
> The following Perl module files have been modified, and the modified versions are
> provided as part of this repo:
> C:\Perl\site\lib\Parse\Win32Registry\WinNT\File.pm
> C:\Perl\site\lib\Parse\Win32Registry\WinNT\Base.pm
> C:\Perl\site\lib\Parse\Win32Registry\WinNT\Key.pm
>
> If you're using the Windows EXE versions of the tools, this is irrelevant, as the
> modified files are "compiled" into the EXE. However, if you're installing on Linux,
> copy the files from the repo to the appropriate locations in your installation.
Have you looked at what kind of modifications have been made and whether they
are still relevant ?
Because right now, with the way how you install these files, I don't think
that they are used at all:
- you modify the perl search path to add /usr/lib/regripper at the end of the
search path (with the quite ugly "push @INC, $str") so it will not take
precedence over the system wide module
- you are installing them as /usr/lib/regripper/{File,Base,Key}.pm and not
as /usr/lib/regripper/Parse/Win32Registry/WinNT/{File,Base,Key}.pm
I don't really like the changes in 10_modify-paths-in-srcs.patch (but I
also don't like the initial upstream code that you had to modify):
-#push(@INC,$str);
+push(@INC,$str);
Is this really needed given that my question above ? Also I think it adds
"/usr/bin" to @INC and not /usr/lib/regripper as you hoped.
-#my $plugindir = $str."plugins/";
-#my $plugindir = File::Spec->catfile("plugins");
+my $plugindir = $str."/usr/lib/regripper/plugins/";
+my $plugindir = File::Spec->catfile("/usr/lib/regripper/plugins");
You don't need to define the variable twice.
In the interest of having a single codebase that works on Linux and
Windows, I think we should aim to patch the code so that it finds the
plugins directory next to where the code lives. Given that $0 returns
/usr/bin/regripper or possibly (./rip.pl if called manually in
/usr/lib/regripper), you would likely have to use Cwd::realpath($path)
to find out the real location of the code.
If you don't want to pursue the upstreaming of this patch, then I would
simply use "use lib '/usr/lib/regripper'" instead of your push @INC
and unconditional setting of $plugindir like you did.
Looking at the autopkgtest, I have a problem with listplugins as this test
will automatically fail for any new plugin added. I'd rather have tests
that do not fail just because we have a new plugin... the janitor bot can
prepare new upstream snapshots and we don't want the bot to believe that
his snapshot is broken just because it contains a new plugin. I think it's
OK to fail if a plugin disappears.
Hint: you could use "combine $TARGET not $ACTUAL" (moreutils package) to
see if some lines disappeared from the CSV output.
Small nitpicks:
* in 40_correct_mountdev2_plugin.patch and
50_correct_scanbutton_plugin.patch you have "Origin" fields that are not
really useful. You are the author of the patch and thus you are the origin,
not the upstream author (even if he merged your patches).
* in 20_correct-encoding.patch you are missing a leading space at the
start of the long description of the patch (before "upstream stores")
Cheers,
PS: I have moved the repository to pkg-security, please continue
to work there, I have added you as maintainer on this project so that you
can drop your copy in your account (if you want to use it further, you
will have to drop and fork it back as I dropped the relationship between
both repositories):
https://salsa.debian.org/pkg-security-team/regripper
On Fri, 21 May 2021, Jan Gru wrote:
> Hi Arnaud, hi Samuel,
>
>
> On 20.05.21 16:15, Arnaud Rebillout wrote:
>
> >
> > The regripper package is now ready for upload I believe.
> >
> > The last thing to be done is to merge
> > https://salsa.debian.org/jgru/regripper/-/merge_requests/5
> >
> I merged this merge request into debian/master [1] and added salsa-ci.yml in
> addition [2].
>
> > The other last thing to do is to move the git repo to pkg-security. I'm
> > not sure who has the permissions to do so. At the moment the repo can be
> > found at:
> >
> > https://salsa.debian.org/jgru/regripper/
> >
> Looking forward to the upload.
>
> MANY THANKS for your help again, Arnaud!
>
>
> Best regards,
>
> Jan
>
> ---
>
> [1] https://salsa.debian.org/jgru/regripper/-/commit/8b4c8002cb4b99573636048e1a1bee26514cca82
>
>
> [2] https://salsa.debian.org/jgru/regripper/-/commit/dc378bd2ae7365b2ab429bdd669b6f121d3a5a8c
>
>
--
⢀⣴⠾⠻⢶⣦⠀ Raphaël Hertzog <hertzog@debian.org>
⣾⠁⢠⠒⠀⣿⡁
⢿⡄⠘⠷⠚⠋ The Debian Handbook: https://debian-handbook.info/get/
⠈⠳⣄⠀⠀⠀⠀ Debian Long Term Support: https://deb.li/LTS
Reply to: