Hi Nicholas, On 06/01/2021 18:12, Nicholas D Steeves wrote: > Hi Ross, > > Sorry for the delay, so tired and busy here! > Me too! > Ross Gammon <ross@the-gammons.net> writes: > >> Please note, I was not able to do a test installation to check hydrogen >> is working. I hope you can confirm that. >> > > I can confirm it works for everything I use it for, but unfortunately I > don't have any midi gear to test midi-related functionality. > >> Blocking upload: >> 1. The package fails to build twice in a row (I used the --twice option >> in pdebuild). It appears some translation files are not being cleaned >> after the first build. >> > > Thank you for spotting this. I've activated a build hook to catch cases > like this in the future. Fixed, and fixed an assumption I had made > about the translations. > >> Minor things: >> 1. We could enable Salsa CI by adding a debian/salsa-ci.yml file. > > We could, but I don't think Hydrogen's test is significant enough to > make it worthwhile to spin up an instance every time someone pushes to > the repo (cost of resources, and cost of carbon footprint). No problems. But you also get other tests like reproducibility and piuparts tests for free, which you should otherwise do manually to avoid issue with an upload. :-) > >> 2. I think the copyright-hints & check files can be removed as they were >> used by cdbs? > > Done, queued for the source-only -2 upload > >> 3. The github issues page could be added to the upstream metadata >> file. > > Is this user facing? I have been specifically omitting this from my > packages, because I worry that it will make it more convenient for the > user to click and report upstream, despite "Don't file bugs upstream" (https://www.debian.org/Bugs/Reporting). Yes, there are a few public facing tools that use this information. And I always think a report in the wrong place is better than no report :-) > >> 4. I am not sure what is going on with the icon. There is a lot going on >> in d/rules and also a patch. This is probably something that should be >> sorted out upstream. As a minimum we should probably forward our patch >> upstream or tag the patch as "Forwarded: not-needed" if it is not an >> upstream issue. I note that in Ubuntu, they install an svg into the >> pixmaps folder. > > Yeah, it's confusing to me too, and yes, ought to be solved upstream. > The png (erroneously named as a bmp) is used internally for something, > and I'm not sure using the svg in that context would succeed, but it > seems like it ought to be. The SVGs we install are: > > usr/share/hydrogen/data/img/gray/h2-icon.svg > usr/share/hydrogen/data/img/gray/icon.svg > usr/share/hydrogen/data/img/gray/warning.svg > usr/share/icons/hicolor/scalable/apps/org.hydrogenmusic.Hydrogen.svg > >> 5. Patch 2001 should probably be tagged as "Forwarded: not-needed" as it >> seems Debian specific. > > Done. Queued for -2. > >> 6. I see that even with all hardening options enabled in d/rules, we >> still get the "hardening no fortify" lintian error. Are there some flags >> not being passed to the build system? > > Possibly. I will investigate and plan to solve this for -2. > >> 7. I see the large number of commits to add library packages and upload >> pre-release versions has resulted in some churn in the changelog. I >> think some entries can be removed now that we have agreed to upload a >> simpler structure and because it is a proper release (e.g. disabling the >> documentation)? >> > > I believe I cut the bin:lib-pkg ones, but I see I missed the docs ones. > Fixed. > >> Suggestions/Considerations: >> 1. It is worth taking a look at the version of hydrogen uploaded in >> Ubuntu by Erich Eickmeyer. By adding ladspa-sdk and libtar as build >> dependencies (and a patch), it appears that the hydrogen builds with >> extra options. > > I considered enabling new features, but chose to change the package as > little as possible, because we're so late in the bullseye cycle. I'm of > course open to enabling them if someone else will thoroughly test the > features. > >> Erich also switched the jack dependency to jack2. > > When building with libjack-dev, debhelper should indirectly generate a > dependency on libjack-jackd2-0 | libjack-0.125. My rational for not > switching is that I'm aware of users of older Firewire interfaces who > prefer (or need) jack and not jack2. I also asked #debian-multimedia > for confirmation that sticking with jack was consistent with team policy > and received a firm statement to not switch to building against jack2-dev. > >> 2. I see we have added a lintian override for the desktop and appdata >> files not being in the same package as the executable. Is there a reason >> for not just installing the desktop/appdata files with the executable >> instead of in -data? >> > > If I remember correctly this was one of the previous maintainers' > decisions, and I think it had to with putting everything is arch:all > into the -data package--personally I think that's a reasonable design > decision. bin:hydrogen Depends on bin:hydrogen-data, and > bin:hydrogen-data Recommends bin:hydrogen, so they'll almost always both > be installed. Do you know if appdata files should always go in the same > package as the executable? It is the logical place for them. That is why there is a lintian warning. It is really large images and sound files etc. that should be moved out to a separate data package. But as you say, there is a hard dependency here so it doesn't really matter in this case. Lets keep it as it was. > > Thanks again for taking the time to review and sponsor! > Nicholas > Thanks for the clear responses! I will check and upload after I finish work today. -- Regards, Ross Gammon FBEE 0190 904F 1EA0 BA6A 300E 53FE 7BBD A689 10FC
Attachment:
signature.asc
Description: OpenPGP digital signature