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>
