Bug 2143827 - Review Request: alsa-ucm-asahi - ALSA UCM configuration fcr Asahi Linux
Summary: Review Request: alsa-ucm-asahi - ALSA UCM configuration fcr Asahi Linux
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Davide Cavalca
QA Contact: Fedora Extras Quality Assurance
Depends On:
TreeView+ depends on / blocked
Reported: 2022-11-18 02:06 UTC by Leif Liddy
Modified: 2023-11-25 17:06 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Last Closed: 2023-11-25 17:06:47 UTC
Type: ---
davide: fedora-review+

Attachments (Terms of Use)

Description Leif Liddy 2022-11-18 02:06:23 UTC
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.src.rpm
Description: ALSA Use Case Manager configuration (and topologies) for Apple silicon devices
Fedora Account System Username: leifliddy

This package is based off of the Asahi-Linux (Arch) reference distro package

Comment 1 Davide Cavalca 2022-11-18 03:02:13 UTC
Taking this review, and I'm happy to sponsor you.

- 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

Comment 2 Leif Liddy 2022-11-19 09:39:48 UTC
Hi David, I've made the necessary changes 

The license is now being included in the git repo:

And so I modified the spec file to pull from the latest commit.

Comment 3 Neal Gompa 2022-11-19 13:14:16 UTC
> 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.

Comment 4 Leif Liddy 2022-11-21 00:31:55 UTC
Ok, so Martin agreed to tag a release and I sorted out the license identifier.
Can you please do another pass?

Comment 5 Neal Gompa 2022-11-21 01:05:45 UTC
> URL:            https://github.com/povik/alsa-ucm-conf-asahi
> Source:         https://github.com/povik/alsa-ucm-conf-asahi/archive/refs/tags/v%{version}.tar.gz

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_tags

Comment 6 Neal Gompa 2022-11-21 01:08:10 UTC
> %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.

Comment 7 Neal Gompa 2022-11-21 01:09:24 UTC
> %{_datadir}/alsa/ucm2/conf.d/macaudio

Please add a trailing slash here so RPM knows it must be a directory.

Comment 8 Leif Liddy 2022-11-21 01:17:05 UTC
> %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.

Comment 9 Leif Liddy 2022-11-21 22:37:05 UTC
This project has recently moved to a new home:

The SPEC file has been updated to reflect that.

Comment 10 Davide Cavalca 2022-11-22 15:10:59 UTC
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.

Comment 11 Davide Cavalca 2022-11-22 15:17:43 UTC
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.

Comment 12 Leif Liddy 2022-11-22 23:22:27 UTC
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.

Comment 13 Kevin Fenzi 2022-11-24 23:26:10 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/alsa-ucm-asahi

Comment 14 Leif Liddy 2022-11-25 00:25:09 UTC
>and you're missing a dash between email and version

If I specify this in the spec file

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> 1-1
- Initial version

Shouldn't that feature be inserting a dash between the email and version?

Comment 15 Davide Cavalca 2022-11-25 02:38:35 UTC
Oh interesting, I never noticed that but you're totally right. Looking at the policy:
both formats are explicitly listed as allowed, so I think you're good either way.

Comment 16 Neal Gompa 2022-11-25 03:43:22 UTC
If you're using %autochangelog, you MUST use %autorelease for your Release: field, because otherwise the changelog will not make sense.

Comment 17 Package Review 2023-11-25 17:06:47 UTC
Package is now in repositories, closing review.

Note You need to log in before you can comment on or make changes to this bug.