Dear mentors,

I am looking for a sponsor for my package "ricochet-refresh":

* Package name : ricochet-refresh
Version : 3.0.11-1
Upstream Author : Blueprint for Free Speech
<ricochet@blueprintforfreespeech.net>
* URL : https://ricochetrefresh.net
* License : Ricochet, tor, public-domain
* Vcs : https://github.com/blueprint-freespeech/ricochet-refresh
Section : contrib/comm

It builds these binary packages:

ricochet-refresh - Anonymous metadata-resistant instant messaging over tor

To access further information about this package, please visit the
following URL:

https://mentors.debian.net/package/ricochet-refresh/

Alternatively, one can download the package with dget using this command:

dget -x
https://mentors.debian.net/debian/pool/contrib/r/ricochet-refresh/ricochet-refresh_3.0.11-1.dsc

Changes for the initial release:

ricochet-refresh (3.0.11-1) unstable; urgency=medium
.
* Initial Debian release. (Closes: #1003510)

Regards,

I am looking at this, and liasing with Marco on irc.

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.

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.

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

Marco Simonelli writes ("Re: Bug#1004345: RFS: ricochet-refresh/3.0.11-1"):

Oops.  No problem.

Let me get onto the technical details.

That's a pure header file, without an implementation.  What are you
linking against ?

I agree that you shouldn't be rolling your own crypto.  But surely
there must be a better way ?  AFAICT "tor/src/ext" is itself stuff
vendored by C Tor ?  Surely there is some library we can reuse here.

I wasn't familiar with the term "donna" in this context, but AFAICT
this is just the name for an implementation of ed25519 ?  Ie
https://github.com/floodyberry/ed25519-donna

Is there some particular reason why you need that specific library ?
Would it be possible to substitute the implementation from one of the
crypto libraries already in Debian ?  Failing that, the preferred
Debian way would be to package ed25519-donna as a library in its own
package, and, I guess, make both the src:tor and src:ricochet-refresh
packages depend on it.

But, is there something wrong with libsodium ?

I appreciate that this guideline against vendoring everything may come
as a bit of a culture shock.  I don't think this RFS review is the
right place to go into the details of the rationale, but I would like
to say that (i) I can see why upstreams like to do this
(ii) nevertheless, Debian has good reasons for trying to avoid
using vendored copies.

I also noticed that there's some git submodule metadata for "fmt", but
that that wasn't in your RFS dsc, and that you have instead used the
existing libfmt packaged in Debian.  Good :-).

See the manual for "dh_auto_configure".  It provides a way to *add*
arguments.  Is that not sufficient ?

If it isn't then I guess you should be calling dpkg-buildflags by hand
and plumbing the results through, but hopefully that's not needed.

Ah.  Hmm.  Yes.  Well, I'm sorry about this, but the Debian Policy
Manual doesn't know about all the things that dh(1) does
automatically.  In this case I think dh will just DTRT if your compat
is high enough.

Err, you don't seem to have a debian/compat ?  Which dh/debhelper
compat level did you target/test with ?  See COMPATIBILITY LEVELS
in debhelper(7).

OK.  Err, not sure if this explains all the discrepancies but using
git-archive will definitely fix it.

Note that I'm also saying you should get rid of the gitattributes.
I don't think they are needed ?

Getting rid of the in-tree copy of fmt would be a +1 from me.

As I say I generally prefer to work in git.  My preferred way to
receive a revised RFS would be as a git branch, which would presumably
not contain any submodules.  That would probably be a descendant (in
git terms) of an upstream branch.

But if this is too complicated git fu I can work with a .dsc instead.

The debhelper manpages are generally pretty good.  Happy to direct you
more specifically (eg via irc).

Good luck.

If there are more questions that would work better interactively, feel
free to prod me on irc of course.

Ian.