[Bug 2143827] New: Review Request: alsa-ucm-asahi - ALSA UCM configuration fcr Asahi Linux
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
Bug ID: 2143827 Summary: Review Request: alsa-ucm-asahi - ALSA UCM configuration fcr Asahi Linux Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: leif.liddy@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://leifliddy.com/.spec/alsa-ucm-asahi.spec SRPM URL: https://leifliddy.com/asahi-linux/37/source/SRPMS/alsa-ucm-asahi-0.2-1.fc37.... Description: ALSA Use Case Manager configuration (and topologies) for Apple silicon devices Fedora Account System Username: leifliddy
Notes: This package is based off of the Asahi-Linux (Arch) reference distro package https://github.com/AsahiLinux/PKGBUILDs/blob/main/alsa-ucm-conf-asahi/PKGBUI...
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
Leif Liddy leif.liddy@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|medium |low
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR) CC| |ngompa13@gmail.com Doc Type|--- |If docs needed, set a value
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
Davide Cavalca davide@cavalca.name changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |davide@cavalca.name Assignee|nobody@fedoraproject.org |davide@cavalca.name Flags| |fedora-review? Status|NEW |ASSIGNED
--- Comment #1 from Davide Cavalca davide@cavalca.name --- Taking this review, and I'm happy to sponsor you.
Blockers: - the license tag is MIT, but the upstream project doesn't seem to have an explicit license and doesn't include a license file (which is a requirement for most licenses, and for MIT in particular); please ask upstream to clarify the project license and to ideally include a LICENSE file, and then add it to the package with %license in the files section - as you're packaging a git snapshot, the version needs to be set accordingly per https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ ; look at https://src.fedoraproject.org/rpms/crosswords-puzzle-sets-gnome for an example
Please fix: - URL should point to the actual URL of the project, i.e. https://github.com/povik/alsa-ucm-conf-asahi - you'll want to install the README.asahi using %doc in the files section
Nits and suggestions: - the description should be a full sentence and end with a period - you can drop the %{_builddir}/%{git_name}-%{_commit_id} in the %install section -- that's already your current directory - it's not obvious why you require coreutils, I'd recommend adding a comment - the changelog entry is valid, but it deviates from the convention sightly: you have a double space between the date and the name, and you're missing a dash between email and version
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #2 from Leif Liddy leif.liddy@gmail.com --- Hi David, I've made the necessary changes
The license is now being included in the git repo: https://github.com/povik/alsa-ucm-conf-asahi/issues/2
And so I modified the spec file to pull from the latest commit.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #3 from Neal Gompa ngompa13@gmail.com ---
License: BSD
We're starting to use SPDX identifiers now, so BSD-3-Clause is the correct identifier here.
%global _commit_id 461b4fe8853fc876c6b2f92414efa9d63f6aa213
You should ask upstream to tag releases if there's a version scheme. That way the spec file can be adjusted to make it easy to update to new versions in the future.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #4 from Leif Liddy leif.liddy@gmail.com --- Ok, so Martin agreed to tag a release and I sorted out the license identifier. Can you please do another pass?
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #5 from Neal Gompa ngompa13@gmail.com ---
URL: https://github.com/povik/alsa-ucm-conf-asahi Source: https://github.com/povik/alsa-ucm-conf-asahi/archive/refs/tags/v%%7Bversion%...
You can simplify things for "Source:" like so:
Source: %{url}/archive/v%{version}/alsa-ucm-conf-asahi-%{version}.tar.gz
Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_ta...
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #6 from Neal Gompa ngompa13@gmail.com ---
%global git_name alsa-ucm-conf-asahi
You only use this macro in one place, you can choose to eliminate it and swap out the macro for the string in the "%setup" macro.
%setup -n %{git_name}-%{version}
You should probably use "%autosetup -n alsa-ucm-conf-asahi-%{version}" here.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #7 from Neal Gompa ngompa13@gmail.com ---
%{_datadir}/alsa/ucm2/conf.d/macaudio
Please add a trailing slash here so RPM knows it must be a directory.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #8 from Leif Liddy leif.liddy@gmail.com ---
%global git_name alsa-ucm-conf-asahi
I was using that in the Source, but removed it in the last iteration. I'll just leave it out and go with your suggestion.
It should be good now.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #9 from Leif Liddy leif.liddy@gmail.com --- This project has recently moved to a new home: https://github.com/AsahiLinux/alsa-ucm-conf-asahi
The SPEC file has been updated to reflect that.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
Davide Cavalca davide@cavalca.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #10 from Davide Cavalca davide@cavalca.name --- fedora-review is complaining that it can't find the srpm (not sure why), but this package is simple enough that we can do a manual review.
- package builds and installs without errors on rawhide - test suite is run and all unit tests pass (there are no tests) - latest version of is packaged - license matches upstream specification and is acceptable for Fedora - license file is included with %license in %files - package complies with Packaging Guidelines
I'd recommend following Neal's recommendation above and using
Source: %{url}/archive/v%{version}/alsa-ucm-conf-asahi-%{version}.tar.gz
for Source, but that's not a blocker.
This is APPROVED. I will also sponsor you into the packager group shortly.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #11 from Davide Cavalca davide@cavalca.name --- I have just sponsored you into the packager group, welcome! The next step here is to use fedpkg request-repo to have a repository created for the package. You will then be able to import it and submit builds. I'd also encourage you to send an email to the Fedora Devel mailing list (https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/) and introduce yourself.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #12 from Leif Liddy leif.liddy@gmail.com --- Thanks Davide. I just changed the name of the SOURCE. The new spec and SRPM files can be found here.
Spec URL: https://www.leifliddy.com/.rpmdev/alsa-ucm-asahi.spec SRPM URL: https://www.leifliddy.com/.rpmdev/alsa-ucm-asahi-1-1.fc37.src.rpm
I'll create a new repo later and will introduce myself to the mailing list. Thanks.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #13 from Kevin Fenzi kevin@scrye.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/alsa-ucm-asahi
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #14 from Leif Liddy leif.liddy@gmail.com ---
and you're missing a dash between email and version
If I specify this in the spec file %changelog %autochangelog
And then commit git commit -m "Initial version"
This is what shows up in the changelog
rpm -q --changelog noarch/alsa-ucm-asahi-1-1.fc38.noarch.rpm * Fri Nov 25 2022 Leif Liddy leif.liddy@gmail.com 1-1 - Initial version
Shouldn't that feature be inserting a dash between the email and version?
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #15 from Davide Cavalca davide@cavalca.name --- Oh interesting, I never noticed that but you're totally right. Looking at the policy: https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs both formats are explicitly listed as allowed, so I think you're good either way.
https://bugzilla.redhat.com/show_bug.cgi?id=2143827
--- Comment #16 from Neal Gompa ngompa13@gmail.com --- If you're using %autochangelog, you MUST use %autorelease for your Release: field, because otherwise the changelog will not make sense.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com