Re: Intro and Intent to Package
Hello Christian,
> > "debhelper-compat" in d/control, as the other packages, so you can
> > remove the file d/compat (debhelper-compat declares both the compat
> > level and the debhelper version).
>
> Done. I left the debian/compat file, though, as my "debuild -- clean" complained
> about it being missing otherwise.
> Or maybe I'm doing it wrong. Maybe look at my control file?
Hmmm, I don't use debuild so I can't say for sure, but I'm confident
that debuild would not have issues with it. I pushed this change to
the branch "debian/samueloph/debhelper-compat", could you see if your
issue happens on it and check if you had done the same change
previously?
And as a side note, I'd like to recommend you to take a look and
consider using sbuild instead, it integrates well with gbp andI feel
like packaging became easier since I switched. The wikipage is
generally well maintained with instructions on how to setup:
https://wiki.debian.org/sbuild
Using debuild is fine, I'm just recommending sbuild because I know
some people don't use it only because they're unaware of it.
> A nice side-effect is that the build is now invoked with the equivalent of
> make -j$(nproc)
> :-)
Nice, I actually didn't know CDBS didn't do this, I was lucky enough
to only have worked with debhelper, I guess. I heard CDBS does make
some very specific packaging scenarios easier, but never got to see
them.
> I'm only kind of upstream. I'm not one of the regular contributors,
> but I work with
> them. So yes, we'll get this done, too.
Great.
> Ah sure. Maybe I didn't make this clear. By "snapshot" I didn't mean a snapshot
> of Git master, but rather a full copy of the source code as of version 2.9 (the
> latest release).
Right, in this case it's fine.
The rule of thumb is that we should import new upstream releases with
"gbp import-orig --uscan", which means that the tarball used should be
what d/watch points to. FWIW you can double-check that with "uscan -v
--no-download".
> Yeah the submodule in question is about "Kafel", a domain-specific language
> for writing syscall filters. I don't believe we should package this
> separately as
> of now. So I'll keep it in the original tarball.
This seems totally fine for me. The end decision of such things relies
on the ftp-master team when the package hits the NEW queue (I don't
think they will have a problem with it).
Review:
d/nsjail.install:
There are no issues with it, I just wanted to point out that the need
for this file can be removed by the Makefile, debhelper evaluates the
upstream's makefile and that means that you can setup a install target
which will then be picked by debhelper and install the things without
explicit declaration on the .install file. This is just an extra that
you might appreciate having upstream since it will make packaging more
straightforward for other distros and put the logic of "what files
needs to be installed" on upstream.
d/control:
Architecture: There are typos on the architectures "arm" and "mips64",
for reference, you can check the architectures here:
https://buildd.debian.org/status/package.php?p=aircrack-ng
Rules-Require-Root: This is a nice-to-have field in d/control, it
makes the build just slightly faster. You can refer to the lintian
entry for more details:
https://lintian.debian.org/tags/rules-requires-root-missing.html
But I just checked here and the package is good to use
"Rules-Require-Root: no" (no changes detected by diffoscope).
Vcs-* fields: Could you please add the fields Vcs-Browser and Vcs-Git?
You can see an example here:
https://salsa.debian.org/pkg-security-team/aircrack-ng/-/blob/7e9252bc7df8f17cf603e55eb17876a5ff219f07/debian/control#L23-24
Maintainer and Uploaders fields: For the package to be team
maintained, you will have to add the team as the maintainer, and then
you add yourself as an Uploader, example:
https://salsa.debian.org/pkg-security-team/aircrack-ng/-/blob/7e9252bc7df8f17cf603e55eb17876a5ff219f07/debian/control#L4-6
d/rules and d/copyright: I noticed you added a copyright header to
d/rules, and I understand why that might be required in your case, it
should just be noted that since the files under "debian/" are
packaging related, eventually there will be other copyright owners for
those files (as people contribute to the packaging), so the "debian/*"
entry in d/copyright will look more like this:
https://salsa.debian.org/pkg-security-team/aircrack-ng/-/blob/7e9252bc7df8f17cf603e55eb17876a5ff219f07/debian/copyright#L154-160
I don't think this will be a problem for you.
If possible, I would recommend removing the header from d/rules
because we are not used to do it and some people could get confused
(thinking that a CLA is needed), the copyright information of d/rules
would still live in d/copyright. This is not a blocker.
d/p/buildflags-override:
The patch is fine, but there might be a potential to make it more
straightforward, it looks like its goal is to not make use of
debhelper flags and strictly follow what's defined in upstream.
I understand you probably know most or all of this, but I will try to
be detailed;
Debhelper does not override flags per se, it just sets them on the
environment (setting them through the command line would cause the
override), which means the easiest way to not use them is to set the
variables in the Makefile with "=" instead of "+=" on the first call
to the variable assignment statement.
eg.: when the makefile has "CFLAGS +=", one can patch the first
assignment statement of that variable to "CFLAGS =", instead of
changing all the assignments.
The quickest example I could find is from Kali, that's an example of
the inverse-case (to use the debhelper flags instead of overriding
them) (please note that the patch description and title is wrong
though): https://gitlab.com/kalilinux/packages/john/-/blob/kali/master/debian/patches/allow-cflags-overriding.diff
Using debhelper flags is advisable since they change as time passes by
and they're all chosen with security and reliability in mind (think
hardening), this means that the binaries will automatically receive
the improvements of the newly defined debhelper flags. For a better
visualization of the thing, you can run "blhc --all" on the buildlog
to see which flags are missing, this is one of the checks we do on our
CI and on the packages in the archive (note: I later found out that
this patch wasn't actually overriding the debhelper flags, so blhc
will only show findings not related to the patch).
I tried to investigate what was the reasoning behind the patch, so
let's see if I can help;
Seems like the issue is (without the patch):
config.cc:85:25: error: ‘bool nsjail::NsJailConfig::has_chroot_dir()
const’ is deprecated [-Werror=deprecated-declarations]
...
config.cc:86:36: error: ‘const string&
nsjail::NsJailConfig::chroot_dir() const’ is deprecated
[-Werror=deprecated-declarations]
...
config.cc:88:39: error: ‘bool nsjail::NsJailConfig::is_root_rw()
const’ is deprecated [-Werror=deprecated-declarations]
...
cc1plus: all warnings being treated as errors
There are three occurrences of this type of error, debhelper makes it
an error, the default is to be a warning.
Considering all of this, I can see that the patch does fix the issue,
but only because it appends "-Wno-deprecated-declarations" to the
CXXFLAGS, please note that doing "override CXXFLAGS +=" would still
have the effect of appending to the debhelper flags because of the
operator "+=".
I have pushed an alternative simplified patch to the branch
"debian/samueloph/deprecation_warning", please take a look.
There's another alternative to the issue, which is to fix the
"deprecated-declarations" warnings in upstream, but working around it
with this patch is fine. I also could not really point out what was
the deprecation issue based on gcc's output.
d/salsa-ci.yml and d/gbp.conf: You can push those files, copying them
from another project, but if not, they will eventually get pushed by
our script. The benefit is that by pushing the ci definition you will
already start making use of it, as the CI is already enabled.
https://salsa.debian.org/pkg-security-team/proxytunnel/-/blob/5f0e286858391034ecb9cc72d9f10567d56c9814/debian/salsa-ci.yml
https://salsa.debian.org/pkg-security-team/proxytunnel/-/blob/5f0e286858391034ecb9cc72d9f10567d56c9814/debian/gbp.conf
d/rules:
>From lintian: "I: nsjail: hardening-no-bindnow usr/bin/nsjail":
https://lintian.debian.org/tags/hardening-no-bindnow.html
You can add the following in d/rules to enable all the hardening flags:
"export DEB_BUILD_MAINT_OPTIONS = hardening=+all"
It will make debhelper set extra flags to increase the security and
reliability of the binaries.
Typos:
Just in case you've missed it; lintian spotted a few typos on the
project, you might wanna fix that or log a ticket. Please note that
this is definitely not a blocker and can be addressed in the future, I
just wanted to report them out.
Alright, I believe that's it, I tried to perform an in-depth review
since it's the first contribution to the team and some of the things
here are not blockers, please feel free to discuss any of the topics.
Thank you for your work and contributions :)
Regards,
--
Samuel Henrique <samueloph>
Reply to: