Bug 2143827
Summary: | Review Request: alsa-ucm-asahi - ALSA UCM configuration fcr Asahi Linux | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Leif Liddy <leif.liddy> |
Component: | Package Review | Assignee: | Davide Cavalca <davide> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | low | Docs Contact: | |
Priority: | unspecified | ||
Version: | rawhide | CC: | davide, ngompa13, package-review |
Target Milestone: | --- | Flags: | davide:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2023-11-25 17:06:47 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 177841 |
Description
Leif Liddy
2022-11-18 02:06:23 UTC
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 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. > 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. Ok, so Martin agreed to tag a release and I sorted out the license identifier. Can you please do another pass? > 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 > %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. > %{_datadir}/alsa/ucm2/conf.d/macaudio
Please add a trailing slash here so RPM knows it must be a directory.
> %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.
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. 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.
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. 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. (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/alsa-ucm-asahi >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> 1-1
- Initial version
Shouldn't that feature be inserting a dash between the email and version?
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. If you're using %autochangelog, you MUST use %autorelease for your Release: field, because otherwise the changelog will not make sense. Package is now in repositories, closing review. |