Hi Daniel Ring, Noticed your ITP announcement on debian-devel and it seemed your package could be a useful addition. I might sponsor you if needed (but like I tell everyone I offer to sponsor please go through the regular RFS procedure and CC me just so others can also review your changes and sponsor your uploads when I'm too busy). I noticed Gunnar has already offered to review and sponsor but I figured the more the merrier, right? ;) Thus adding some quick comments from my perspective below in case they are useful.
On Sun, May 07, 2017 at 11:01:33AM +0000, Daniel Ring wrote: > May 2, 2017 9:58 AM, "Gunnar Wolf" <gw...@debian.org> wrote: > > Hi Daniel, > > > > I'm interested in looking at your package. When it's ready and when > > you need a sponsor, mail me! > > Hi Gunnar, > > I've finished putting together a preliminary version of the package, but I > have a few concerns about it. > > The largest one is that the build system is NodeJS-based, and requires a > version of npm newer than the one currently in Debian. Bugs #857986 and > #794890 > have some details about npm's issues. Installing nodejs from its official > repository works, as does building on Ubuntu. Hopefully also npm from experimental works? As you might be aware npm was removed from Stretch as requested by its maintainer, since not enough people where helping out getting it in good enough shape to be in a stable release. Unless the situation improves it's likely npm won't be in next stable (Buster) either (and I haven't seen anyone really offering to help yet)... In my view, maintaining a package also means you need to look at the health of your dependencies and get them in shape where needed. Are you interested in getting involved with packaging of npm itself or how do you view the current outlook of not being included in next stable release? If you're not targeting getting your package shipped in a stable release then it can still be useful, but you might also find peoples interest in reviewing and sponsoring to be less enthusiastic. > > Secondly, the build system has the usual issue with NodeJS packaging; it > downloads dependencies at runtime. Most of the packages don't exist in Debian > or are out of date, and I found several existing packages doing this while > looking for a better solution, so I'm not sure how much of an issue this is. Me neighter, but in general I think you should assume atleast some people will have problem with anything else than you packaging those dependencies as separate (reusable) packages. See the gitlab packaging efforts as an example. If you bundle unpackaged stuff that might be ok up until too many packages ship a bundeled version and security team eventually vetos not including your package. Thus if you bundle it's likely either way good to see this as flying under the radar and always being on the lookout for alternative ways to handle this so you're prepared for the day when you might have to handle it (or preferably even have it handled before that). > This only occurs at build-time, and nodejs isn't required to use the software. I guess you're already aware though that (atleast on official Debian buildds) there's no internet connectivity available at package build time... > > Finally, the upstream source contains several embedded libraries. I was able > to > swap a few of them for existing packages in Debian, but there are a few PHP > libraries that don't have existing packages. The JavaScript libraries are > amalgamated into a single file at build-time, and separating them out would be > a non-trivial amount of work for decreased performance. Again, I found several > existing packages doing this, so I'm not sure how much of a problem it is. Me neighter, but preferably the amalgation process should be a step in the package building. One reason is it's easier to fix any future found issues by patching the source and rebuilding rather than having to patch something generated. > Upstream provided sources for most, I added the few missing to satisfy > lintian. > > I've uploaded the package to mentors: > https://mentors.debian.net/package/rainloop (Thanks for the link. Full .dsc url usually preferred.) > Please review it when you have a chance, and let me know if there's anything I > need to fix! > I've quickly looked at the packaging and in general it looks well prepared. I made some notes about things that popped up on my mind attached below. One more general question I have is about security though. See for example roundcube which has had quite a few CVEs found and fixed during the years. Has rainloop taken any particular stance on development practises for security? How new is the project or how widely has it been deployed yet that might give it some kind of practical security track record? Anyway, my notes... debian/control: - "nodejs (>=6) | npm (>=3)" b-dep looks weird to me as these two are not functionally equivalents of each other. - please use (the more secure and preferred) https url for Vcs-Git - Mix of php(7) and php5 dependencies? Only php5 compatible? Will we ship php5 or will rloop soon be php7 compatible? For example the php-sabre-vobject dependency in turn seems to depend on php(7.x). Maybe I'm just uninformed and some php5-* packages are both 5 and 7 compatible?! debian/copyright: - You probably need to include the full AGPL-3 license text since it's not available in "common-licenses". - Please consider, since it might make your life easier, to license debian/* under the same license as upstream (consider any contributed patches that might end up under debian/patches/* which you'll likely want to upstream to lessen your own maintenance burden). debian/*postinst: - Have you considered excluding /var/lib/rainloop from dh_fixperms instead of using maintainer script? The www-data user is a statically allocated user which should have the same uid on all Debian systems. (See 'base-passwd' package.) Hope this helps. Regards, Andreas Henriksson