Bug#1004345: RFS: ricochet-refresh/3.0.11-1
Hiya,
First off, just wanted to quickly say thank you for offering your help
on this, and second, I replied like 3 days ago but didn't realize I'm
meant to include <BUG_#>@bugs.debian.org in the recipient list, so this
message was in limbo for a few days :(
> 1. Embedded copy of Tor
> I observe that this source package contains a copy of tor. Debian
> really doesn't like embedded copies like that. Is there some reason
> why you can't use the copy of Tor that's in Debian already ?
We don't actually compile any of tor to embed - we just use the ed25519
donna header in $TOR_SRC_ROOT/src/ext/ed25519/donna/ed25519_donna_tor.h,
to avoid rolling our own crypto (and afaik tor does not install this or
any headers).
> 2. In debian/rules, there is a bunch of stuff to modify the compile
> flags.
> * Why does dh + cmake not get the install prefix etc. right by
> itself ? Is there a Debian bug about that ?
> * There are various options like -z now, -D_FORTIFY_SOURCE=2, -g,
> etc, which, again, ISTM that dh cmake ought to get right.
So maybe this is due to my lack of intimate experience with debian, but
since we need to override certain CMake configure options (using
override_dh_*), does that not mean that the rest of the flags need to be
set manually as well? or does debuild still use it's own flags in
addition to the ones specified in override_dh_*? Maybe there's a better
way to add configure/compile options that I didn't pick up in the DPM?
> * Why are we hand-tuning the parallelism ?
This I just massaged from the example given in the DPM, see here,
section 4.9.1: https://www.debian.org/doc/debian-policy/ch-source.html
> And, I see that the "uptream tarball" you provided in your RFS is not
> identical to (any version of?) the git branch. I'm not sure how it
> was produced. AFAICT from the upstream web page, the official source
> code is in git. So I would have expected any upstream tarball to be
> the result of "git archive".
Hmm interesting - I didn't actually know git archive was a thing so I'll
be sure to use that next time! Best guess is my 1am brain edited a file
and forgot to add it to the git repo, I'll fix this up in a few hours.
> Additionally, I see you have a contraption to remove .gitignore and
> .gitattributes from your tarballs. I think this is ill-conceived.
> There is no reason you can't put a .gitignore into a tarball.
> I think the .gitignore is part of the preferred form for modification
> (ie, part of the source code.)
Again, not quite sure, but I'll use git archive for the next upload :>
As far as the tor submodule goes, it's entirely possible for us to just
create an ext folder like tor does, and plonk the file into there, and
just include it manually without having to use tor as a submodule. I
guess the rationale here is if any security critical patches were
pushed to the tor repository, then we'd get that patch without manually
updating the file (whether the patch intentionally patched some security
issue or not).
With FMT, imho we shouldn't have it as a submodule at all, since I have
to intentionally leave it out of the orig tarball since the repo doesn't
comply with dfsg, and the maintainer refuses to fix it (something to do
with precompiled bootstrap.js :/). FMT is a small enough lib that we can
just install it on our build machines, and it's already required as a
dependency in our debian builds.
Some clarification (either direct, or a resource) on the behavior of dh
would be a huge help! I tried looking through the DPM, but couldn't find
anything directly about it.
Regards,
Marco
On 28/1/22 03:01, Ian Jackson wrote:
Hi. I put on my "Debian" hat had a look at this package. I started
with the git branch git@github.com:m-simonelli/ricochet-refresh
885f03138fe6e88006bdb672ce478751d119eff3 and then also ended up
downloading the dsc and comparing it.
Here are my comments. I hope they're not too discouraging...
1. Embedded copy of Tor
I observe that this source package contains a copy of tor. Debian
really doesn't like embedded copies like that. Is there some reason
why you can't use the copy of Tor that's in Debian already ?
Please forgive me if the answer to this is something I ought to
already know. But I think if there is a good answer it ought to be in
some in-tree document, maybe debian/README.source.
(After I noticed this, I stopped my review, since it seemed likely that
changing this would cause substantial other changes.)
2. In debian/rules, there is a bunch of stuff to modify the compile
flags. I think it is good practice to leave a comment next to all
override_dh_* to explain why it is needed, if it is not enitrely
obvious.
In particular,
* Why does dh + cmake not get the install prefix etc. right by
itself ? Is there a Debian bug about that ?
* There are various options like -z now, -D_FORTIFY_SOURCE=2, -g,
etc, which, again, ISTM that dh cmake ought to get right.
* Why are we hand-tuning the parallelism ?
etc.
I'm not very familiar with cmake so perhaps this is all "normal for
cmake" but if so that would be me as contrary to the usual philosophy
of dh(1), which is usually to do the right thing by default.
3. Use of git and tarballs
This is not a blocker. And I'm going to make some comments which will
definitely not be shared by everyone. And I appreciate that much
Debian packaging really likes to think about tarballs. I'll work with
.dscs and tarballs if there isn't a better way.
But:
I see you used git submodules. IMO git submodules are extremely poor.
This message is not the right place for a full explanation, but they
(i) fundamentally break many of git's basic design principles, and
(ii) consequently break many very normal git operations. Separately
(iii) the design and implementation are full of avoidable bugs.
In general, I would recommend git subtree instead - at least if the
subproject is not too large compared to the superproject, which isn't
the case here. But I'm not sure this program ought to have a bundled
copy of Tor anyway. So what precisely ought to be done about this
depends on what to do about the embedded Tor copy.
And, I see that the "uptream tarball" you provided in your RFS is not
identical to (any version of?) the git branch. I'm not sure how it
was produced. AFAICT from the upstream web page, the official source
code is in git. So I would have expected any upstream tarball to be
the result of "git archive".
If you *do* have official upstream tarballs, then the most common
Debian practice nowadays is to use pristine-tar.
Additionally, I see you have a contraption to remove .gitignore and
.gitattributes from your tarballs. I think this is ill-conceived.
There is no reason you can't put a .gitignore into a tarball.
I think the .gitignore is part of the preferred form for modification
(ie, part of the source code.)
Of course I'm happy to talk about everything more (here, irc, or
private email).
Regards,
Ian.
Reply to: