Many thanks Paul for your review - even though you do not wish to sponsor this, your review has been very valuable.
I've addressed your comments as discussed below. Looking on the mentors / mypackages webpage it says that the watch file I've included does not work. This is very strange because I ran a uscan and it correctly downloaded the upstream release file: uscan --force-download --verbose uscan info: uscan (version 2.16.2ubuntu3) See uscan(1) for help uscan info: Scan watch files in . uscan info: Check debian/watch and debian/changelog in . uscan info: package="budgie-desktop" version="10.2.5-1" (as seen in debian/changelog) uscan info: package="budgie-desktop" version="10.2.5" (no epoch/revision) uscan info: ./debian/changelog sets package="budgie-desktop" version="10.2.5" uscan info: Process ./debian/watch (package=budgie-desktop version=10.2.5) uscan info: Last orig.tar.* tarball version (from debian/changelog): 10.2.5 uscan info: Last orig.tar.* tarball version (dversionmangled): 10.2.5 uscan info: Requesting URL: https://github.com/solus-project/budgie-desktop/releases uscan info: Matching pattern: (?:(?:https://github.com )?\/solus\-project\/budgie\-desktop\/releases)?.*/?(\d{2}\.\d.\d)\.tar\.xz uscan info: Found the following matching hrefs on the web page (newest first): /solus-project/budgie-desktop/releases/download/v10.2.5/budgie-desktop-10.2.5.tar.xz (10.2.5) index=10.2.5-4 /solus-project/budgie-desktop/releases/download/v10.2.4/budgie-desktop-10.2.4.tar.xz (10.2.4) index=10.2.4-4 /solus-project/budgie-desktop/releases/download/v10.2.3/budgie-desktop-10.2.3.tar.xz (10.2.3) index=10.2.3-4 /solus-project/budgie-desktop/releases/download/v10.2.2/budgie-desktop-10.2.2.tar.xz (10.2.2) index=10.2.2-4 /solus-project/budgie-desktop/releases/download/v10.2.1/budgie-desktop-10.2.1.tar.xz (10.2.1) index=10.2.1-4 uscan info: Matching target for downloadurlmangle: https://github.com/solus-project/budgie-desktop/releases/download/v10.2.5/budgie-desktop-10.2.5.tar.xz uscan info: Upstream URL (downloadurlmangled): https://github.com/solus-project/budgie-desktop/releases/download/v10.2.5/budgie-desktop-10.2.5.tar.xz uscan info: Newest upstream tarball version selected for download (uversionmangled): 10.2.5 uscan info: Download filename (filenamemangled): budgie-desktop-10.2.5.tar.xz uscan: Newest version of budgie-desktop on remote site is 10.2.5, local version is 10.2.5 uscan info: => Package is up to date for from https://github.com/solus-project/budgie-desktop/releases/download/v10.2.5/budgie-desktop-10.2.5.tar.xz uscan info: => Forcing download as requested uscan info: Downloading upstream package: budgie-desktop-10.2.5.tar.xz uscan info: Requesting URL: https://github.com/solus-project/budgie-desktop/releases/download/v10.2.5/budgie-desktop-10.2.5.tar.xz uscan info: Successfully downloaded package: budgie-desktop-10.2.5.tar.xz uscan info: Start checking for common possible upstream OpenPGP signature files uscan info: End checking for common possible upstream OpenPGP signature files uscan info: Missing OpenPGP signature. uscan info: New orig.tar.* tarball version (oversionmangled): 10.2.5 uscan info: Executing internal command: mk-origtargz --package budgie-desktop --version 10.2.5 --compression gzip --directory .. --copyright-file debian/copyright ../budgie-desktop-10.2.5.tar.xz uscan info: New orig.tar.* tarball version (after mk-origtargz): 10.2.5 uscan info: Successfully symlinked ../budgie-desktop-10.2.5.tar.xz to ../budgie-desktop_10.2.5.orig.tar.xz. uscan info: Scan finished The remaining review comments and how I addressed them now follows: > Does upstream have an opinion on having older versions of Budgie in > Debian stable, which gets supported for 5 years now that we have LTS. I've asked this question upstream: https://github.com/solus-project/budgie-desktop/issues/446 In summary - users are requested to upgrade. Moving forward, the maintainer intends to branch the project at the next major release and will backport stuff where necessary (e.g. critical issues). This will be very useful for Debian to identify issues to include in updates. > I would suggest using diffoscope to compare the broken build with the > working one, you might discover the reason for this brokenness. This did not reveal any specific build issues. > The package fails to build because gtk+3.0 3.20.5-1 is not yet built in Debian: I presume this is a transition issue for Sid as it moves to GTK+3.20 <https://buildd.debian.org/status/package.php?p=gtk+3.0&suite=unstable> > There are many hardcoded library dependencies, they shouldn't be > needed as ${shlibs:Depends} will take care of them, unless these > libraries are loaded using dlopen instead of linking. If they are > loaded with dlopen, a ${dlopen:Depends} substvar and a script to > generate it would be better than hardcoding them. The dependencies are been cleaned up. No libraries are included. The minimal necessary dependencies have been left - these are required for the desktop system to start successfully > debian/copyright is missing some copyright holders. This has been substantially revised > I think the ftp-masters will want debian/copyright to be more specific > about which files are LGPL and which are GPL. The copyright now identifies LGPL vs GPL. I also asked upstream https://github.com/solus-project/budgie-desktop/issues/447 and upstream recommend the following: grep -R "Lesser General" | cut -d : -f 1 | uniq | grep -v LICENSE > I note that the upstream tarball contains generated files (*.c *.vapi > *.css *.png *.html). I personally think these need to be removed from > the upstream tarball and VCS if present in either of those and always > created at build time. If upstream doesn't want to do that an > acceptable workaround would be to remove these files in `debian/rules > clean` and in `debian/rules build` before autoreconf/configure are > run. Alternatively you could use the gitub-generated tarballs which > only contain what is in git. Looks like you will need to package some > more build-deps here though, like gulp-sass. I asked this upstream: https://github.com/solus-project/budgie-desktop/issues/448 In the debian/clean I've removed the build artifacts that upstream have recommended here https://github.com/solus-project/budgie-desktop/issues/446#issuecomment-221378660 > The imports/natray/ and gvc/ directories appear to be embedded code > copies from one of GNOME/cinnamon/MATE/cairo-dock-plug-ins/something. > They should be removed from all of these including budgie and packaged > separately. Until that happens the security team need to be notified > about the embedded code copy, which they track. > $ apt-file search -iIdsc na-tray > https://wiki.debian.org/EmbeddedCodeCopies <https://wiki.debian.org/EmbeddedCodeCopies> I asked about this via a hangout session with upstream. The maintainer pointed me at this https://bugzilla.redhat.com/show_bug.cgi?id=1170875 - TL;DR - copied code is allowed due to no stable API In summary " There's no system version -- gnome-control-center, gnome-settings-daemon and gnome-shell all bundle libgnome-volume-control using git submodules: https://mail.gnome.org/archives/commits-list/2012-November/msg06793.htmlhttps://bugzilla.gnome.org/show_bug.cgi?id=686488 (just checked and indeed there is no standalone libgnome-volume-control lib). Ah, OK, that's why I couldn't find it. According to [1] the submodule does not have a stable api and is supposed to be "linked" wherever it is needed. So including it in budgie-desktop is allowed by the packaging guidelines, as a "copy library". [1] https://mail.gnome.org/archives/gnomecc-list/2012-October/msg00003.html"; > Please add DEP-3 headers to the patches, particularly the > Origin/Forwarded headers should point at URLs. > The first line of nm-applet.diff looks a bit strange. This has been done. > The debian-watch-file-is-missing lintian tag should not be overridden > since upstream has a git repo with tags and tarballs that can be used > with uscan and debian/watch. override has been removed > Please file bugs on lintian about the > dep5-copyright-license-name-not-unique and postinst-must-call-ldconfig > false positives. This has been removed since the package has been reworked. > The binary-without-manpage lintian tag should not be overridden since > it is true. Just ignore it until a manual page exists. Override has been removed > It would be great if upstream could sign their commits, tags and > releases with OpenPGP: Upstream are already signing their commits. Tags/releases are not going to be signed. > Why do you disable the ibus systray icon? I've removed this gsettings override > I'm not sure the gnome-settings overrides are appropriate. This has been tidied - only one vital override exists - this is needed to display the GNOME appmenu correctly in the window decoration. > I wonder if the gsettings overrides should be renamed to > budgie-desktop.gsettings-override so it is only installed for one > package? This has been renamed > Usually in debian/control the version numbers in dependency relations > have a space before them: Space has been added. >I like to wrap-and-sort the debian/ directory using this command: > wrap-and-sort --short-indent --wrap-always --sort-binary-packages > --trailing-comma Done. > I'm not sure that Section: gnome is appropriate, could you explain > your reasoning here? I've moved to misc since I didnt see any other obvious Sid section available. Please advise if there is a better more appropriate section for GNOME/GTK+3 based desktop systems such as budgie-desktop > The Vcs-Browser field is reserved for the Debian packaging VCS, not > upstream. See below for a solution for upstream. Removed > I would suggest setting the Homepage here instead of to github: > https://solus-project.com/budgie/ <https://solus-project.com/budgie/> Home-page updated. > I note there are several stamp files that probably aren't meant to be > in the upstream tarball: This does not any material difference - these files are not installed - see the maintainers comments here: https://github.com/solus-project/budgie-desktop/issues/448 > There are some files in the upstream VCS that are missing from the > upstream tarball, I wonder if that is intentional. Apparently yes - according to the maintainer as linked above. > A typo in theme/README.md: obejct This is not installed - source only issue. > Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata Added upstream metadata >$ sudo apt-get install -t experimental check-all-the-things > $ check-all-the-things On the whole these findings were based on C issues - however this is due as far as I can gather due to the Vala compiler - so not something upstream can deal with. > # Not sure why but autodep8 doesn't like your debian/control: > $ autodep8 > grep-dctrl: debian/control:48: expected a colon. This seems to be fixed now. > # These should be created at build time instead > $ find -type f \( -iname '*.png' -o -iname '*.gif' -o -iname '*.jpg' > -o -iname '*.jpeg' \) -exec grep -iF inkscape {} + > <lots> Maintainer has indicated otherwise - see link above > $ codespell --quiet-level=3 > <lots> Vala to C compiler issues - not an upstream matter. On 22 May 2016 at 06:22, Paul Wise <p...@debian.org> wrote: > On Sun, May 22, 2016 at 6:38 AM, foss.freedom wrote: > > > > https://mentors.debian.net/debian/pool/main/b/budgie-desktop/budgie-desktop_10.2.5-1.dsc > > I've included a review below. > > > budgie-desktop is the flagship desktop system for Solus. Solus is an > tier 1 > > distro using its own packaging mechanism eopkg. Solus supports only its > own > > distro (naturally) in a 64bit intel based system only. The maintainer > does > > accept bug-reports for other distro's as long as it is reproducible in > Solus > > and/or the maintainer considers that the wider use of its desktop > > environment would be enhanced. > > Does upstream have an opinion on having older versions of Budgie in > Debian stable, which gets supported for 5 years now that we have LTS. > > > To produce a debian package that works in debian and ubuntu I have > used a > > more traditional rules based package rather than a simpler debhelper > > auto-build mechanism. I have had to do it this way because debhelper > does > > not produce binaries that actually work on a Ubuntu based platform - the > > desktop system fails to launch at logon. The failure is silent - there > is > > no obvious reason why debhelper autobuild fails to produce a working > > solution. > > I would suggest using diffoscope to compare the broken build with the > working one, you might discover the reason for this brokenness. > > > The patches incorporated are required for specifically Debian and Ubuntu > and > > are used in budgie-remix itself. > > I don't intend to sponsor this but here is a review: > > Things that I personally think should be fixed before this package can > be uploaded: > > The package fails to build because gtk+3.0 3.20.5-1 is not yet built in > Debian: > > libgtk-3-0 : Depends: libgtk-3-common (>= 3.20.5-1) which is a > virtual package and is not provided by any available package > https://buildd.debian.org/status/package.php?p=gtk+3.0&suite=unstable > > There are many hardcoded library dependencies, they shouldn't be > needed as ${shlibs:Depends} will take care of them, unless these > libraries are loaded using dlopen instead of linking. If they are > loaded with dlopen, a ${dlopen:Depends} substvar and a script to > generate it would be better than hardcoding them. > > debian/copyright is missing some copyright holders. > > I think the ftp-masters will want debian/copyright to be more specific > about which files are LGPL and which are GPL. > > I note that the upstream tarball contains generated files (*.c *.vapi > *.css *.png *.html). I personally think these need to be removed from > the upstream tarball and VCS if present in either of those and always > created at build time. If upstream doesn't want to do that an > acceptable workaround would be to remove these files in `debian/rules > clean` and in `debian/rules build` before autoreconf/configure are > run. Alternatively you could use the gitub-generated tarballs which > only contain what is in git. Looks like you will need to package some > more build-deps here though, like gulp-sass. > > The imports/natray/ and gvc/ directories appear to be embedded code > copies from one of GNOME/cinnamon/MATE/cairo-dock-plug-ins/something. > They should be removed from all of these including budgie and packaged > separately. Until that happens the security team need to be notified > about the embedded code copy, which they track. > > $ apt-file search -iIdsc na-tray > https://wiki.debian.org/EmbeddedCodeCopies > > Things that I think would be nice to fix: > > Please add DEP-3 headers to the patches, particularly the > Origin/Forwarded headers should point at URLs. > > http://dep.debian.net/deps/dep3/ > > The first line of nm-applet.diff looks a bit strange. > > The debian-watch-file-is-missing lintian tag should not be overridden > since upstream has a git repo with tags and tarballs that can be used > with uscan and debian/watch. > > https://github.com/solus-project/budgie-desktop/releases > https://wiki.debian.org/debian/watch#GitHub > > Please file bugs on lintian about the > dep5-copyright-license-name-not-unique and postinst-must-call-ldconfig > false positives. > > The binary-without-manpage lintian tag should not be overridden since > it is true. Just ignore it until a manual page exists. > > It would be great if upstream could sign their commits, tags and > releases with OpenPGP: > > https://wiki.debian.org/Creating%20signed%20GitHub%20releases > https://wiki.debian.org/debian/watch#Cryptographic_signature_verification > https://help.riseup.net/en/security/message-security/openpgp/best-practices > > I wonder what happens when you have both budgie and GNOME installed, > will the GNOME change? > > Why do you disable the ibus systray icon? > > I'm not sure the gnome-settings overrides are appropriate. > > I wonder if the gsettings overrides should be renamed to > budgie-desktop.gsettings-override so it is only installed for one > package? > > Usually in debian/control the version numbers in dependency relations > have a space before them: > > - libgnome-bluetooth-dev (>=3.16), > + libgnome-bluetooth-dev (>= 3.16), > > I like to wrap-and-sort the debian/ directory using this command: > > wrap-and-sort --short-indent --wrap-always --sort-binary-packages > --trailing-comma > > I'm not sure that Section: gnome is appropriate, could you explain > your reasoning here? > > The Vcs-Browser field is reserved for the Debian packaging VCS, not > upstream. See below for a solution for upstream. > > I would suggest setting the Homepage here instead of to github: > > https://solus-project.com/budgie/ > > You might want to test debian/compat 10: > > https://lists.debian.org/debian-devel/2016/04/msg00018.html > > I note there are several stamp files that probably aren't meant to be > in the upstream tarball: > > $ find -iname *.stamp | wc -l > 19 > > There are some files in the upstream VCS that are missing from the > upstream tarball, I wonder if that is intentional. > > A typo in theme/README.md: obejct > > Please add some upstream metadata: > https://wiki.debian.org/UpstreamMetadata > > Automatic checks: > > $ sudo apt-get install -t experimental check-all-the-things > $ check-all-the-things > > # Not sure why but autodep8 doesn't like your debian/control: > $ autodep8 > grep-dctrl: debian/control:48: expected a colon. > ... > > # This command checks style. While a consistent style > # is a good idea, people who have different style > # preferences will want to ignore some of the output. > # Do not bother adding non-upstreamable patches for this. > $ find -type f \( -iname '*.sh' -o -iname '*.bash' \) -exec bashate > --ignore E002,E003 {} + > E010: Do not on same line as for: 'for i in `cat $INDEX`' > - ./theme/render-assets.sh : L10 > E001: Trailing Whitespace: 'do ' > - ./theme/render-assets.sh : L11 > E001: Trailing Whitespace: ' && $OPTIPNG -o7 --quiet $ASSETS_DIR/$i.png > ' > - ./theme/render-assets.sh : L20 > E001: Trailing Whitespace: ' && $OPTIPNG -o7 --quiet > $ASSETS_DIR/$i...@2.png ' > - ./theme/render-assets.sh : L31 > 5 bashate error(s) found > > # These should be created at build time instead > $ find -type f \( -iname '*.png' -o -iname '*.gif' -o -iname '*.jpg' > -o -iname '*.jpeg' \) -exec grep -iF inkscape {} + > <lots> > > $ codespell --quiet-level=3 > <lots> > > # This command checks code complexity. While simple > # code is a good idea, complex code can be needed. > # Do not bother adding non-upstreamable patches for this. > $ find -type f -iname '*.c' -exec complexity {} + > <lots> > > $ debmake -k > I: set parameters > I: compare debian/copyright with the source > <lots> > > # Please check if these directories contain embedded code/data copies. > # Please remove any embedded copies from the upstream VCS and tarballs. > # https://wiki.debian.org/EmbeddedCodeCopies > $ find -type d -name 'vendor*' -o -name '*rd*party' -o -name contrib > -o -name imports > ./imports > > $ find \( -name .git -o -name .svn -o -name .bzr -o -name CVS -o -name > .hg -o -name _darcs -o -name _FOSSIL_ -o -name .sgdrawer \) -prune -o > -empty -print > ./docs/budgie-desktop-overrides.txt > > $ fdupes -q -r . | grep -vE > '/(\.(git|svn|bzr|hg|sgdrawer)|_(darcs|FOSSIL_)|CVS)(/|$)' | cat -s > <lots> > > $ flawfinder -Q -c . > <lots> > > $ find -type f \( -iname '*.po' -o -iname '*.pot' \) -exec POFileSpell {} + > <lots> > > # check if these can be switched to https:// > $ grep -rF http: . > <lots> > > $ find -type f \( -iname '*.po' -o -iname '*.pot' -o -iname '*.mo' -o > -iname '*.gmo' \) -exec i18nspector {} + > <lots> > > $ find -type f \( -iname '*.c' -o -iname '*.cc' -o -iname '*.cxx' -o > -iname '*.cpp' -o -iname '*.h' -o -iname '*.hh' -o -iname '*.hxx' -o > -iname '*.hpp' \) -exec include-what-you-use {} \; > <lots, probably lots of false positives> > > $ license-reconcile > <lots, some false positives> > > $ licensecheck --check=. --recursive --copyright . | grep --text -F > 'GENERATED FILE' > <lots> > > $ licensecheck --check=. --recursive --copyright . | grep --text -F > 'with incorrect FSF address' > <lots> > > $ find -type f \( -iname '*.sh' -o -iname '*.bash' -o -iname '*.zsh' > \) -exec shellcheck {} + > <lots> > > $ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname > .svn -o -iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o > -iname _darcs -o -iname .pc -o -iname .cabal-sandbox -o -iname .cdv -o > -iname .metadata -o -iname CMakeFiles -o -iname _build -o -iname > _sgbak -o -iname autom4te.cache -o -iname blib -o -iname cover_db -o > -iname node_modules -o -iname '~.dep' -o -iname '~.dot' -o -iname > '~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( -iname '*.bak' -o > -iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 'core.*' -o > -iname '*~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' -o > -iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o -iname > '*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o -iname > '*.css.min' \) -exec spellintian --picky {} + > <lots, some might be relevant> > > $ grep -riE 'fixme|todo|hack|xxx|broken' . > <lots, some useful> > > $ find -type f -iname '*.xml' -exec xmllint --noout --nonet {} + > <lots> > > -- > bye, > pabs > > https://wiki.debian.org/PaulWise >
