Sign me up Login

Details about package rgbds

Name: rgbds
Uploader: Eldred HABERT <me@eldred.fr> (Debian QA page)
Description: rgbds - Game Boy ASM programming tools

Package uploads

Upload #4

Information

Version: 0.4.2-1
Uploaded: 2021-03-16 08:35
Source package: rgbds_0.4.2-1.dsc
Distribution: unstable
Section: devel
Priority: optional
Homepage: https://rgbds.gbdev.io
Vcs-Browser: https://github.com/ISSOtm/rgbds-deb
Vcs-Git: https://github.com/ISSOtm/rgbds-deb.git
Closes bugs: #984927

Changelog

 rgbds (0.4.2-1) unstable; urgency=medium
 .
   * Initial release (Closes: #984927)

QA information

Comments

No comments

Upload #3

Information

Version: 0.4.2-1
Uploaded: 2021-03-13 23:50
Source package: rgbds_0.4.2-1.dsc
Distribution: unstable
Section: devel
Priority: optional
Homepage: https://rgbds.gbdev.io
Vcs-Browser: https://github.com/ISSOtm/rgbds-deb
Vcs-Git: https://github.com/ISSOtm/rgbds-deb.git
Closes bugs: #984927

Changelog

 rgbds (0.4.2-1) unstable; urgency=medium
 .
   * Initial release (Closes: #984927)

QA information

Comments

  1. Hi Eldred, thanks again for working on this packaging.
    
    (To anyone else reading: I'm an occasional rgbds user and would love to see this in Debian. I'm only a DM, not a DD, so I can't sponsor the upload.)
    
    The package looks pretty good, just a few comments. Apologies in advance, if you've already fixed locally anything I comment on here.
    
    debian/source/local-options exists in your git repository (not in the .dsc, per its name). What is its purpose? You don't have any patches, and even if you did, AFAIK unapply-patches is the default behaviour.
    
    debian/rules:
    
    ifneq (,$(filter nostrip,$(DEB_BUILD_OPTIONS)))
    	STRIP :=
    else
    	STRIP := -s
    endif
    
    I don't think this is doing anything. It's not exported, and not referenced otherwise. Note that debhelper is overriding the strip command anyway, with or without nostrip; from the build log:
    
       dh_auto_build -O--buildsystem=cmake\+makefile -O-Bbuild
            cd build && make -j4 "INSTALL=install --strip-program=true" VERBOSE=1
    
    AFAIK, packages should always build with debug symbols (not stripped). dh_strip needs them to create the automatic debug packages, and respects nostrip on its own.
    
    ifneq (,$(filter terse,$(DEB_BUILD_OPTIONS)))
    	Q := @
    else
    	Q :=
    endif
    
    Same comment here: it doesn't seem to have any effect. Actually, I suspect that by default we should explicitly invoke "make Q="; the upstream Makefile sets it with := and AFAIK that wouldn't be overridden just by exporting it in the environment. (But I question whether we actually want to use the Makefile; see below.)
    
    Also, it looks like you're exporting DH_VERBOSE regardless even when terse is set. Personally (this is just an opinion) I would not put DH_VERBOSE in debian/rules, only set it locally when debugging something.
    
    ifneq (,$(filter parallel=%,$(DEB_BUILD_OPTIONS)))
        PARALLEL = -j$(patsubst parallel=%,%,$(filter parallel=%,$(DEB_BUILD_OPTIONS)))
    endif
    
    Same again: doesn't seem to have any effect. Actually DEB_BUILD_OPTIONS is already automatically respected by most tools. I don't see PARALLEL anywhere in the upstream build system, so I'm not sure what this is supposed to do.
    
    %:
    	dh $@ --buildsystem=cmake+makefile -B build
    
    What is "+makefile" gaining us? I tried it with just --buildsystem=cmake and it seems to work fine? I probably missed something.
    
    execute_before_dh_auto_clean:
            rm -f build/src/rgbasm build/src/rgblink build/src/rgbfix build/src/rgbgfx
    
    It looks to me like "dh_auto_clean" automatically removes the build/ directory? But, this should probably clean up the copies made in . for the tests. There are some other untracked files generated during the build, too. Maybe we should run "make clean" in addition to the auto clean.
    
    debian/copyright: I'm not really experienced enough to comment on this but I'm wondering if it's detailed enough. Several of the upstream source files mention other authors in their license headers, and the README mentions former copyright holders too.
    
    https://salsa.debian.org/games-team/mgba/-/blob/master/debian/copyright is an example of a more detailed copyright file that I'm familiar with. Note, I don't know for sure what level of detail is actually required.
    
    Hope this helps.
    Ready Ryan Tandy at March 16, 2021, 3:53 a.m.
  2. Hello! Thanks again for the review.
    
    I initially created the `local-options` file while I was using in-tree builds (from the Makefile), but as you said, it's no longer needed, so I removed it.
    
    The DEB_BUILD_OPTIONS in `rules` are holdovers from when I was using the Makefile; they are no longer needed, so I got rid of them as well.
    
    > Also, it looks like you're exporting DH_VERBOSE regardless even when terse is set.
    
    My mistake, I meant not to commit that, but I forgot. I removed it.
    
    > What is "+makefile" gaining us? I tried it with just --buildsystem=cmake and it seems to work fine?
    
    I got the list of build systems from `dh_auto_build -l`, which mentioned both `cmake+makefile` and `cmake+ninja`, but not just `cmake`. In the interest of avoiding superfluous dependencies, I went with the former; but since the latter should be simpler and possibly more flexible, I switched to just `cmake` as you indicated.
    
    > But, this should probably clean up the copies made in . for the tests.
    
    Correct, and this was the intention, modulo a stupid copy/paste mistake. My bad.
    
    > There are some other untracked files generated during the build, too. Maybe we should run "make clean" in addition to the auto clean.
    
    It's worth noting that building using `pbuilder` keeps the source tree clean no matter what, so I only added that rule out of precaution. Maybe it's entirely unnecessary?
    
    > Several of the upstream source files mention other authors in their license headers, and the README mentions former copyright holders too.
    
    I checked again, and I noticed that I did forget to mention the vendor code in src/extern and include/extern; this has been corrected. As for the different names in the source files, they are mostly here to record who originally wrote the code, but the codebase as a whole is licensed by all RGBDS contributors, which those names are part of.
    
    There has been a point where copyright of the codebase was much more hazy, but that was settled when the codebase was relicensed as a whole under the MIT license; it's been several years, so while IANAL either, I think this should be risk-free.
    
    All of these have been incorporated into the new upload.
    
    Thank you very much for the feedback, and apologies for the stupid mistakes!
    Eldred HABERT at March 16, 2021, 8:30 a.m.

Upload #2

Information

Version: 0.4.2-1
Uploaded: 2021-03-13 22:35
Distribution: experimental
Section: devel
Priority: optional
Homepage: https://rgbds.gbdev.io
Vcs-Browser: https://github.com/ISSOtm/rgbds-deb
Vcs-Git: https://github.com/ISSOtm/rgbds-deb.git
Closes bugs: #984927

Changelog

 rgbds (0.4.2-1) experimental; urgency=medium
 .
   * Initial release (Closes: #984927)

QA information

Comments

No comments

Upload #1

Information

Version: 0.4.2-1
Uploaded: 2021-03-11 23:05
Distribution: experimental
Section: devel
Priority: optional
Homepage: https://rgbds.gbdev.io
Vcs-Browser: https://github.com/gbdev/rgbds
Vcs-Git: https://github.com/gbdev/rgbds.git
Closes bugs: #984927

Changelog

 rgbds (0.4.2-1) experimental; urgency=medium
 .
   * Initial release (Closes: #984927)

QA information

Comments

  1. The package looks mostly ready, but I have a few select suggestions.
    
    It looks like a package with the debugging symbols is not automatically generated, probably because the binaries lack debugging information. This might be due to the Makefile buildsystem which may not be as robust as CMake. Furthermore, CMake is more likely to support cross compilation without other tweaks. It is my opinion that the CMake build system is likely better suited for packaging. If it doesn't have feature parity yet, please ask upstream to fix that.
    
    The control file has
    Vcs-Browser: https://github.com/gbdev/rgbds
    Vcs-Git: https://github.com/gbdev/rgbds.git
    
    but these fields should specify the VCS for the Debian package, not upstream. It doesn't appear there's any Debian packaging there yet (perhaps you need to push your commits?), but if you plan to keep your Debianization there upstream, that's alright.
    
    There's an empty file in debian/ called 'rgbds-docs.docs'.
    
    There's a makefile provided by dpkg at /usr/share/dpkg/buildopts.mk for your convenience (and actually several more) that can make debian/rules cleaner, particularly when it comes to setting up the parallel build (but again, if you use CMake this won't be necessary). You could make a change like this to make debian/rules cleaner:
    
    --- rules.orig	2021-03-10 20:47:42.000000000 -0500
    +++ rules	2021-03-11 19:04:36.063794003 -0500
    @@ -1,5 +1,8 @@
     #!/usr/bin/make -f
     # See debhelper(7) (uncomment to enable)
    +include /usr/share/dpkg/buildopts.mk
    +DEB_BUILD_OPTION_PARALLEL ?= 1
    +
     # output every command that modifies files on the build system.
     export DH_VERBOSE = 1
     
    @@ -15,10 +18,6 @@
     	Q :=
     endif
     
    -ifneq (,$(filter parallel=%,$(DEB_BUILD_OPTIONS)))
    -    PARALLEL = -j$(patsubst parallel=%,%,$(filter parallel=%,$(DEB_BUILD_OPTIONS)))
    -endif
    -
     export DEB_BUILD_MAINT_OPTIONS=hardening=+all
     
     
    @@ -29,7 +28,7 @@
     # The Makefile doesn't have a separate preprocessing step, and otherwise doesn't consider CPPFLAGS.
     # Further, enable extra warnings.
     override_dh_auto_build:
    -	$(MAKE) $(PARALLEL) Q=$Q CFLAGS+="$(CPPFLAGS)" WARNFLAGS="-Wall -Wextra"
    +	$(MAKE) -j$(DEB_BUILD_OPTION_PARALLEL) Q=$Q CFLAGS+="$(CPPFLAGS)" WARNFLAGS="-Wall -Wextra"
     
     # Do not run the full test suite, as it pulls non-free codebases to run tests on.
     override_dh_auto_test:
    @@ -37,4 +36,4 @@
     	cd test/link && ./test.sh
     
     override_dh_auto_install:
    -	$(MAKE) $(PARALLEL) install DESTDIR="$(CURDIR)"/debian/rgbds PREFIX=/usr STRIP=$(STRIP) Q=$Q
    +	$(MAKE) -j$(DEB_BUILD_OPTION_PARALLEL) install DESTDIR="$(CURDIR)"/debian/rgbds PREFIX=/usr STRIP=$(STRIP) Q=$Q
    
    You should look into the Debian Games Team if you haven't already; they may be interested in co-maintaining this package with you.
    
    I'm not a Debian Developer and can't sponsor, but hope this helps the package. Since none of these issues are blockers, I feel "Ready" is more appropriate than saying your package needs work.
    Ready John Scott at March 12, 2021, 12:10 a.m.
  2. Thank you very much for your feedback!
    
    > It looks like a package with the debugging symbols is not automatically generated, probably because the binaries lack debugging information.
    
    I saw -g being passed to the compiler invocations while building, so I don't know why the binaries wouldn't have any debug info.
    
    > It is my opinion that the CMake build system is likely better suited for packaging. If it doesn't have feature parity yet, please ask upstream to fix that.
    
    It should be worth noting that I am upstream -- RGBDS' main maintainer for the time being -- and as far as I'm aware, the CMakeLists should have at least feature parity with the Makefile. I opted for the Makefile instead to avoid adding an extra build dependency, but it seems that CMake would be preferred instead, so I'll look into that.
    
    > these fields should specify the VCS for the Debian package, not upstream.
    
    I read in some documentation that it is a bad idea for upstream to provide their own debian/, so I committed to a separate repo (https://github.com/ISSOtm/rgbds-deb). Should I link to that one, or should I try to integrate the debian packaging upstream, perhaps on a separate branch?
    
    > There's an empty file in debian/ called 'rgbds-docs.docs'.
    
    My bad, it's a leftover. I removed it locally, though I'll wait for more feedback before pushing that.
    
    (Skipping the diff suggestion, since I'll switch to CMake.)
    
    > You should look into the Debian Games Team if you haven't already; they may be interested in co-maintaining this package with you.
    
    Thanks for the suggestion! I'll look into that.
    
    > Since none of these issues are blockers, I feel "Ready" is more appropriate than saying your package needs work.
    
    Especially since the package shouldn't make it into bullseye, I'll try to incorporate these suggestions anyway.
    
    I'm looking to improve the code's quality; most of the warnings present in the packaged release have been fixed in upstream, for example. (Note that the next stable release is planned for early April, so I preferred packaging the current latest stable, rather than the fresh 0.5.0-rc1.)
    Eldred HABERT at March 12, 2021, 12:52 a.m.