https://bugzilla.redhat.com/show_bug.cgi?id=2338778 --- Comment #3 from Benson Muite <benson_muite@xxxxxxxxxxxxx> --- (In reply to Ben Beasley from comment #2) > > ===== Issues ===== > > - Since one file is under an MIT license, the text of that license is also > required, > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > LicensingGuidelines/#_license_text. > > I opened an upstream PR to amend the upstream LICENSE file. Since the > copyright notices are included in the SPDX fields in the source file > header, > and the license text is taken from the URL cited in the source file, it > should be safe enough to patch this in downstream while awaiting upstream > review. > Done. > - The format of > > VCS: https://github.com/blogc/blogc > > is not consistent with the documentation at > > https://rpm-software-management.github.io/rpm/manual/tags.html > > which says it should be > > (Public) upstream source code VCS location. Format <vcs>:<address> with > <vcs> being the VCS command used (e.g. git, svn, hg, …) and <address> > being > the location of the repository as used by the VCS tool to clone/checkout > the repository (e.g. https://github.com/rpm-software-management/rpm.git). > > Therefore, please change this to > > VCS: git:https://github.com/blogc/blogc.git Fixed thanks. > > - The getsource.sh script does not work. Change > > tar -cvf blogc > > to > > tar -cvf blogc.tar blogc Fixed thanks. > > - I was going to question the use of a "getsource.sh" script, as you normally > don’t need one to package a git snapshot from GitHub. You would be able to > change > > Source0: blogc.tar.gz > # Need Git files for build version from a commit > Source1: getsource.sh > > to > > Source: > https://github.com/blogc/blogc/archive/%{commit}/blogc-%{commit}.tar.gz > > and > > %autosetup -n blogc -p 1 > > to > > %autosetup -n blogc-%{commit} -p 1 > > or maybe something like > > %autosetup -n blogc-%{commit} -p 1 -S git > git tag v%{version} > > if this weren’t a snapshot… > > but then I saw how the version number actually includes the number of git > commits since the last release tag in the version number ("29"): > > 0.20.1.29-b1ae > > This is hard to figure out by hand and hard-code, and it absolutely > requires > the real upstream .git directory to do it automatically. > > Unfortunately, this means the source archive technically includes not just > everything in a git checkout, but everything that has *ever* been committed > to the upstream git repository, which is probably fine but really expands > the > scope of review. > > I think what you’re doing is probably the best you can do while still > generating the right version number, but please consider changing > > git clone https://github.com/blogc/blogc > > in getsource.sh to > > git clone https://github.com/blogc/blogc --shallow-since=2021-01-01 > > which still goes back far enough to catch the last release tag on > 2021-01-02, > without including all of the project’s ancient history. > Done. > - Please consider encouraging upstream to make a new release. > Done: https://github.com/blogc/blogc/issues/19 > - While the changelog is correctly formatted, I don’t think it makes sense to > include changelog entries from the upstream spec file, before the package > was > included in Fedora. The author has a copr: https://copr.fedoraproject.org/coprs/rafaelmartins/blogc/ and it is mentioned on the main webpage https://blogc.rgm.io/ probably helpful to have these if any people transition from copr to the package when it is in Fedora. > > - As demonstrated in > > > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_requiring_base_package > > the dependencies among subpackages of the same RPM should be arch-specific. > > Requires: %{name} = %{version}-%{release} > Requires: %{name}-make = %{version}-%{release} > Requires: %{name}-runserver = %{version}-%{release} > > (in various places) should become, respectively: > > Requires: %{name}%{?_isa} = %{version}-%{release} > Requires: %{name}-make%{?_isa} = %{version}-%{release} > Requires: %{name}-runserver%{?_isa} = %{version}-%{release} > Done. > - While this isn’t strictly required, it can be nicer to write > > # https://github.com/blogc/blogc/pull/17 > Patch: getaddrinfo.patch > > as > > # https://github.com/blogc/blogc/pull/17 > Patch: https://github.com/blogc/blogc/pull/17.patch > > or, if you want to preserve the patch name, > > # https://github.com/blogc/blogc/pull/17 > Patch: https://github.com/blogc/blogc/pull/17.patch#/getaddrinfo.patch > Done. > - Since this will be a new leaf package, consider adding: > > # https://fedoraproject.org/wiki/Changes/EncourageI686LeafRemoval > ExcludeArch: %{ix86} > > This package builds fine on i686, but there is no value in building it > there. Still have a running and almost indestructible i686 machine. > > - I was able to determine experimentally that > > BuildRequires: git > > can and should be replaced with > > BuildRequires: git-core > > Consider trying to determine if > > Requires: git > > can be replaced with > > Requires: git-core > > too, based on the particular git commands that are actually used. > > See: https://fedoraproject.org/wiki/Changes/ChangeToGitCore This seems ok. Main commands used are git, git-shell, git_archive_cmd, git-receive-pack, git-upload-pack spec: https://fed500.fedorapeople.org/blogc.spec srpm: https://fed500.fedorapeople.org/blogc-0.20.1^20240602.b1ae9c9-1.fc42.src.rpm -- 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=2338778 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202338778%23c3 -- _______________________________________________ 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
