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
