https://bugzilla.redhat.com/show_bug.cgi?id=2332914 --- Comment #11 from Alex Haydock <alex@xxxxxxxxxxxxxxxxx> --- (In reply to Fabio Valentini from comment #10) > > Spec URL: > > https://download.copr.fedorainfracloud.org/results/alexhaydock/quickemu/ > > fedora-42-x86_64/08832097-quickemu/quickemu.spec > > SRPM URL: > > https://download.copr.fedorainfracloud.org/results/alexhaydock/quickemu/ > > fedora-42-x86_64/08832097-quickemu/quickemu-4.9.7-5.fc42.patched.src.rpm > > Thanks for the update - I'll take another look now! Thanks, I appreciate the time you've taken on this one. > > > I rebuilt this since I posted, using the latest upstream 4.9.7 code. It also > > builds (tested and working nicely on a daily basis) on F42. > > Great! > > > There are a few things I know probably need resolving before this could be > > accepted though. > > * Patch1 is based on upstream (merged) PR which fixes an active bug in F41+ > > but hasn't made it to a release yet: > > https://github.com/quickemu-project/quickemu/pull/1565 > > That's fine, just put a quick comment + a link to this PR (or the commit > corresponding to the merged PR) alongside Patch1. Done > > > * After writing the SPEC above I found in the patching guidelines that I > > need to add a comment referencing where the patch exists upstream > > * With the above in mind, I probably can't include Patch2, 3, or 4. They're > > all based on this PR I authored, but it hasn't been accepted upstream yet: > > https://github.com/quickemu-project/quickemu/pull/1579 > > That's fine too. A pull request that has been sent to upstream is a > perfectly fine reference for local patches. > Just document what those patches do and / or why they are required, and > include a link to the PR itself in the spec file. Also done > > > * I probably also need to reset the package versioning to 4.9.7-1 as the > > changelog includes a bunch of my own experimentation while learning how to > > properly patch SPEC files. > > This will happen automatically when you import the package, assuming you > don't import your whole git history too :) Easy enough :) thanks! > > ============================================================================= > === > > There's some other issues with the package I see: > > 0. I would recommend to consistently indent tag values, I usually use 16 > spaces because that leaves enough room for all common tags names. > You're currently using a mix of 12 spaces / three tabs and no indentation at > all, which looks a bit strange, but this is a matter of "style" (or > "taste"), so you there is no hard requirement for this. Ah this is definitely a casualty of piecing together various tutorials from around the web to make this one SPEC. Your 16-space style makes a lot of sense to me so I've adopted that now. > > 1. You will need to drop the ".patched" suffix for %autorelease at some > point, but at the very latest before importing the package to Fedora. Done for now, but can revisit when importing the package anyway > > 2. The "Group" tag is obsolete and SHOULD NOT be used: > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections > You can just drop it. Removed > > 3. The Source0 URL doesn't use the recommended format for GitHub tarballs. > They are documented here: > https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > #_git_tags > > It would look like this in your case: > Source: %{url}/archive/%{version}/%{name}-%{version}.tar.gz Fixed. I see you've used %{version} there where the docs page uses %{gittag} so I guess it relies on the assumption that the upstream project will continue to use release tags that match version numbers but if they ever stop then I suppose it'll be obvious quite quickly. > > 4. Since all your patches are upstream commits and / or PRs, you could just > reference them as such: > > Patch: https://github.com/quickemu-project/quickemu/pull/1565.patch > Patch: https://github.com/quickemu-project/quickemu/pull/1579.patch > > This way their "upstream status" is immediately documented, and you just > need to add some comments why you include them. > > (GitHub provides actual .patch files under those URLs if you just attach > ".patch" to a Pull Request URL, you can download them with "spectool -g" as > usual.) > This is super useful, thanks! I had forgotten about this feature of GitHub. > 5. I don't understand what the sha256 sum thing here is about: > > > # Define upstream SHA256SUM for the .tar.gz matching this release version > > # curl -fsSL https://github.com/quickemu-project/quickemu/archive/refs/tags/4.9.7.tar.gz -o - 2>/dev/null | sha256sum > > %define SHA256SUM0 38a93301a2b233bc458c62d1228d310a9c29c63c10d008c2905029ca66188606 > > The checksum of source archives will be determined and checked before > uploading them to Fedora servers, and every time they are used for builds - > so there is no need to do this manually. You can drop the manual checksum > creation and checking entirely. > This makes sense. I think what was confusing me here is that in the Copr method there's not really anything in between the upstream GitHub and final package to do that validation, so in theory the release could be changed upstream and future package builds would just blindly incorporate any of those changes. Sounds like that's all provided for when a package is imported into Fedora proper. > 6. All Requires and Recommends MUST be resolvable from Fedora repositories: > > > # Based on: https://github.com/quickemu-project/quickemu/wiki/01-Installation#install-requirements-on-fedora > > # The `zsync` package is listed under Recommends as it is no longer packaged in mainline Fedora > > So you will need to drop "Recommends: zsync" if it is no longer available > from Fedora: > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_package_dependencies That's fair. It's in RPMFusion so I left it but removed now. Is there a process for documenting where an upstream dependency can't be satisfied in the Fedora package? zsync in this project is really only used as part of the download tool that simplifies downloading images to boot VMs with. It's likely only be useful to people who regularly download the diffs of Ubuntu daily build ISOs (so probably a very tiny subset of users). But it still feels like maybe I should document it somewhere? Alternatively I could just make it clear this is the case when I update the upstream docs after the package is imported into Fedora. > > 7. "%global debug_package %{nil}" > > This should not be necessary. Your package contains no ELF objects with > debuginfo, so it's obvious that there *can* be no debuginfo. > > Add this instead (somewhere between the "Patch" lines and "Requires": > > ``` > BuildArch: noarch > ``` > > This means that your package has only architecture-independent content and > metadata, and causes generation of debuginfo to be skipped. Makes sense! Removed, thanks > > 8. Use best practices for %prep scriptlet. > > > %prep > > # Validate our checksum > > echo "%SHA256SUM0 %SOURCE0" | sha256sum -c - > > %setup -q > > > > %autopatch > > This looks very strange. You should be able to just replace this with: > > ``` > %prep > %autosetup -p1 > ``` > > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_autosetup > > (It's a bit strange that this page mentions %autosetup as an "alternative" - > it really should be the default for new packages nowadays.) Also fixed, thanks Thanks again for reviewing! -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2332914 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202332914%23c11 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
