Hi On 2021-01-07 10:39:55 +0100, Ross Gammon wrote: > 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. The *.desktop and appdata files belong to the package that provides the executable (except for some corner cases). In particular, having the appdata file in hydrogen-data instructs the consumers of those files to install hydrogen-data if somebody wants to install hydrogen. They are non-functional if installing hydrogen-data without also installing hydrogen. So please move them to hydrogen. Cheers > > > > > 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 > -- Sebastian Ramacher
Attachment:
signature.asc
Description: PGP signature