Bug#976113: RFS Review for Hydrogen
Hi Sebastian, On 07/01/2021 11:06, Sebastian Ramacher wrote: > 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. Good point - well made. Don't worry Nicholas. I can take care of that to avoid another mentors upload round trip. -- Regards, Ross Gammon FBEE 0190 904F 1EA0 BA6A 300E 53FE 7BBD A689 10FC signature.asc Description: OpenPGP digital signature
Bug#976113: RFS Review for Hydrogen
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 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
Bug#976113: RFS Review for Hydrogen
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 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
Bug#976113: RFS Review for Hydrogen
Hi Ross, Sorry for the delay, so tired and busy here! Ross Gammon 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). > 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). > 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? Thanks again for taking the time to review and sponsor! Nicholas signature.asc Description: PGP signature
Bug#976113: RFS Review for Hydrogen
Hi Nicholas, Good stuff. I think we are just about ready to upload. Here are the comments from my review. Please note, I was not able to do a test installation to check hydrogen is working. I hope you can confirm that. 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. Minor things: 1. We could enable Salsa CI by adding a debian/salsa-ci.yml file. 2. I think the copyright-hints & check files can be removed as they were used by cdbs? 3. The github issues page could be added to the upstream metadata file. 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. 5. Patch 2001 should probably be tagged as "Forwarded: not-needed" as it seems Debian specific. 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? 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)? 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. Erich also switched the jack dependency to jack2. 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? -- Regards, Ross Gammon FBEE 0190 904F 1EA0 BA6A 300E 53FE 7BBD A689 10FC signature.asc Description: OpenPGP digital signature
