On Sun, Jul 12, 2020 at 2:52 AM Samuel Henrique <samueloph@debian.org> wrote: > > > [...] > > 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 > TIL. Will take a look. > 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. > Precisely my reason :) > > 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. > Yeah. For me it was some package that used autotools and CDBS just took care of everything. I stumbled upon this long ago and it kinda stuck. debhelper always felt a bit arcane and with a lot of moving pieces. > > [...] > > 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". > That seems to do the right thing, i.e. uscan gives me the sensible output that there is no later version available. > > [...] > 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. > I'll file a bug for an "install" target. OTOH, I like that the packaging can put files where they belong in a Debian system. > 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 > Done. > 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). > Done. > 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 > Done. > 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. Done. Went with GPLv2+ for those files. This is allowed internally as well. > 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. > Sure, also done. > 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; > [...] > Indeed :) The intent was to a) work around the deprecation warning and b) not have debhelper overwrite the flags in the Makefile. The underlying issue was that environment variables would be ignored by the Makefile, while, when specified on the command line, the variables would be overwritten. Specifying "override" in the Makefile is also kind of ugly, as that enforces the project's settings and, as you wrote, prevents the hardening flags to take effect. > 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. > Thanks, will do. > 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 > Thanks! I must've missed those. > 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. > Will do. > 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. > I had already pushed a fix for those upstream, they were a bit embarrassing :) That fix will be in the next release (whenever they decide to do it). > 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 :) > Thanks for the thorough review. I can see how Google might have taken some inspiration from the Debian process when implementing code reviews - we're super used to that internally as well and I love the attention to detail. This week is super busy for me, so when I say "done" above, it means I will push my changes on Friday or so and ping you to take another look. > [...] -- Christian Blichmann | Senior Software Engineer | Google™ m: +41 79 7 18 79 43 | cblichmann@google.com
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature