Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07730364-btrfs-backup-ng/btrfs-backup-ng.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07730364-btrfs-backup-ng/btrfs-backup-ng-0.6.3-8.fc40.src.rpm Description: This project supports incremental backups for *btrfs* using *snapshots* and *send/receive* between filesystems. Think of it as a basic version of Time Machine. Backups can be stored locally and/or remotely (e.g. via SSH). Multi-target setups are supported as well as dealing with transmission failures (e.g. due to network outage). Fedora Account System Username: mberry This is the first package I am attempting to get into the official Fedora repositories, with the goal of joining the Fedora packager group, and I need a sponsor. I am the upstream maintainer of this project. Outside of the Copr builds I also completed a Koji Scratch Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122184608
It would be best to use the modern Python packaging macros, which automate many of the manual things here.: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ > %global pypi_version 0.6.3 This is mostly redundant; if you set `Version: 0.6.3`, it automatically generates a %{version} macro. > Source0: https://files.pythonhosted.org/packages/source/b/%{pypi_name}/btrfs_backup_ng-%{pypi_version}.tar.gz No need to number a single Source; can use the %pypi_source macro for the URL. > BuildRequires: python3dist(setuptools) Use %pyproject_buildrequires to autogenerate dependencies. > Summary: %{summary} Redundant. > Requires: python3dist(rich) > Requires: python3dist(setuptools) These are automatically determined by project metadata; no need to specify the manually. > %py3_build The modern macro is %pyproject_wheel. > %py3_install The modern macro is %pyproject_install. > %{python3_sitelib}/btrfs_backup_ng > %{python3_sitelib}/btrfs_backup_ng-%{pypi_version}-py%{python3_version}.egg-info You can use %pyproject_save_files to pick these up automatically. Are there any tests that could be run? Consider at least %pyproject_check_import. Also, consider using %autorelease and %autochangelog.
Not in the packaging guidelines, but please make sure there is a communication channel with upstream. I believe in this case it is your own fork, so consider opening issues. Some other notes: - name os the spec file/srpm should be python-btrfs-backup-ng - testing wise, understandibly this can be quite tricky, but maybe with `tmt` and `testing-farm` it would be possible to provision test environments with empty disk and then prepare the btrfs disk to do functional tests. Would be quite a task, not going to sugarcoat it. - are the Requires field sufficient? Don't you need a dependency on what provides `btrfs`?
(In reply to Cristian Le from comment #2) > - name os the spec file/srpm should be python-btrfs-backup-ng Not true if this is primarily an application: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming
Ack, fair point. Then instead it should have a `%py_provides`
Hello, thank you for the incredibly quick replies and suggestions. I've read the modern Fedora Python guidelines link and I believe implemented all the suggested spec file changes that were suggested as including adding btrfs-progs as a requirement (I believed this was automatically installed by default), as well as opened up issues on the projects github. Changes are as follows: Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07937697-btrfs-backup-ng/btrfs-backup-ng.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07937697-btrfs-backup-ng/btrfs-backup-ng-0.6.3-1.fc40.src.rpm Koji Scratch Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122331057 I'm hoping this is an improvement at least, any further feedback of any kind would be most welcome.
Copr build: https://copr.fedorainfracloud.org/coprs/build/7937707 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305930-btrfs-backup-ng/fedora-rawhide-x86_64/07937707-btrfs-backup-ng/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
A few other comments: - Preferably upload a spec file and srpm that did not expand `rpmautospec` - The `Summary:` after the `%py_provides` has no effect (py_provides inserts `Provides:` for the current package definition `btrfs-backup-ng` in this case, where `Summary:` is already defined. It does not create new packages [1]) Similarly `%description -n %{name} %_description` is redundant there. - Please use `%{pypi_source btrfs_backup_ng}`, the approach with `pypi_name` is deprecated - `%pyproject_save_files btrfs_backup_ng` is sufficient - About: "Check if the automatically generated License and its spelling is correct for Fedora" Yes it is correct. Search for the spdx (short) license identifier [2] when you need to. - (optional) Some people prefer using macros everywhere, like `%{name}`, others find it distracting as there is no guaranteed correlation between the values where it is expanded in. There is no clear templates or automation tools for python, unlike rust and go, so you can go with either approach until then. I cannot sponsor packagers, so I will leave the official review to whoever can sponsor you. [1]: https://src.fedoraproject.org/rpms/python-rpm-macros/blob/rawhide/f/macros.python-srpm#_195 [2]: https://spdx.org/licenses/
Thank you Christian Le for taking the time to make so many helpful suggestions. Every point you raised I was wondering about and you explained things very well. I will be uploading new links to my new Spec and SRPM. Cheers.
Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07991619-btrfs-backup-ng/btrfs-backup-ng.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07991619-btrfs-backup-ng/btrfs-backup-ng-0.6.3-9.fc40.src.rpm Koji Scratch Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=123066821
(In reply to Cristian Le from comment #7) > A few other comments: > - Preferably upload a spec file and srpm that did not expand `rpmautospec` > - The `Summary:` after the `%py_provides` has no effect (py_provides inserts > `Provides:` for the current package definition `btrfs-backup-ng` in this > case, where `Summary:` is already defined. It does not create new packages > [1]) > Similarly `%description -n %{name} %_description` is redundant there. > - Please use `%{pypi_source btrfs_backup_ng}`, the approach with `pypi_name` > is deprecated > - `%pyproject_save_files btrfs_backup_ng` is sufficient > - About: "Check if the automatically generated License and its spelling is > correct for Fedora" > Yes it is correct. Search for the spdx (short) license identifier [2] when > you need to. > - (optional) Some people prefer using macros everywhere, like `%{name}`, > others find it distracting as there is no guaranteed correlation between the > values where it is expanded in. There is no clear templates or automation > tools for python, unlike rust and go, so you can go with either approach > until then. > > I cannot sponsor packagers, so I will leave the official review to whoever > can sponsor you. > > [1]: > https://src.fedoraproject.org/rpms/python-rpm-macros/blob/rawhide/f/macros. > python-srpm#_195 > [2]: https://spdx.org/licenses/ Thabk out Christian
> %global _description %{expand:...} This variable is also only used in place (for `%description`), so you might as well inline it there as well. > %global pypi_name btrfs_backup_ng This variable is only used in one place (for `%py_provides`), so you might as well inline it there too, removing its definition, but see also below. > %py_provides %{pypi_name} > Requires: btrfs-progs These are both part of your `%description` and don't actually do anything where they are. Checking the built package: $ rpm -q --provides -p ./btrfs-backup-ng-0.6.3-9.fc42.noarch.rpm btrfs-backup-ng = 0.6.3-9.fc42 python3.13dist(btrfs-backup-ng) = 0.6.3 python3dist(btrfs-backup-ng) = 0.6.3 $ rpm -q --requires -p ./btrfs-backup-ng-0.6.3-9.fc42.noarch.rpm /usr/bin/python3 python(abi) = 3.13 python3.13dist(rich) rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests) <= 4.6.0-1 rpmlib(PartialHardlinkSets) <= 4.0.4-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 rpmlib(PayloadIsZstd) <= 5.4.18-1 So the `Provides` are already automatically detected and correct, but there are no `Requires` for `btrfs-progs`. You should move the `Requires` before the `%description`, and drop the `%py_provides` call. I would also remove the remaining comments; they're instructions for you, and you've already completed them. I also cannot sponsor you.
Created attachment 2045799 [details] The .spec file difference from Copr build 7937707 to 7997562
Copr build: https://copr.fedorainfracloud.org/coprs/build/7997562 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305930-btrfs-backup-ng/fedora-rawhide-x86_64/07997562-btrfs-backup-ng/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
(In reply to Elliott Sales de Andrade from comment #11) > > %global _description %{expand:...} > > This variable is also only used in place (for `%description`), so you might > as well inline it there as well. > > > %global pypi_name btrfs_backup_ng > > This variable is only used in one place (for `%py_provides`), so you might > as well inline it there too, removing its definition, but see also below. > > > %py_provides %{pypi_name} > > Requires: btrfs-progs > > These are both part of your `%description` and don't actually do anything > where they are. Checking the built package: > > $ rpm -q --provides -p ./btrfs-backup-ng-0.6.3-9.fc42.noarch.rpm > btrfs-backup-ng = 0.6.3-9.fc42 > python3.13dist(btrfs-backup-ng) = 0.6.3 > python3dist(btrfs-backup-ng) = 0.6.3 > > $ rpm -q --requires -p ./btrfs-backup-ng-0.6.3-9.fc42.noarch.rpm > /usr/bin/python3 > python(abi) = 3.13 > python3.13dist(rich) > rpmlib(CompressedFileNames) <= 3.0.4-1 > rpmlib(FileDigests) <= 4.6.0-1 > rpmlib(PartialHardlinkSets) <= 4.0.4-1 > rpmlib(PayloadFilesHavePrefix) <= 4.0-1 > rpmlib(PayloadIsZstd) <= 5.4.18-1 > > So the `Provides` are already automatically detected and correct, but there > are no `Requires` for `btrfs-progs`. You should move the `Requires` before > the `%description`, and drop the `%py_provides` call. > > I would also remove the remaining comments; they're instructions for you, > and you've already completed them. > > I also cannot sponsor you. Thank you for your review and comments, I see know where I had obviously messed things up and not manually checking the rpm for provides and requires was a serious mistake. I also was missing an essential require of python3-rich. I made the changes you suggested and made sure to check things out better this time. Your help is greatly appreciated.
Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07998488-btrfs-backup-ng/btrfs-backup-ng.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07998488-btrfs-backup-ng/btrfs-backup-ng-0.6.3-10.fc40.src.rpm
You won't need to specify `rich`; it's already found from the Python metadata: > $ rpm -q --requires -p ./btrfs-backup-ng-0.6.3-9.fc42.noarch.rpm > python3.13dist(rich)
(In reply to Elliott Sales de Andrade from comment #16) > You won't need to specify `rich`; it's already found from the Python > metadata: > > > $ rpm -q --requires -p ./btrfs-backup-ng-0.6.3-9.fc42.noarch.rpm > > python3.13dist(rich) Yes I overlooked the double dependency, I'll fix it, I did know better than that. Thanks again.
Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07998590-btrfs-backup-ng/btrfs-backup-ng.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fedora-40-x86_64/07998590-btrfs-backup-ng/btrfs-backup-ng-0.6.3-11.fc40.src.rpm
I know I made plenty of mistakes up to this point, but hopefully I've proven I'm willing to learn, do research on my own, and follow advice to make corrections. I feel like I've already learned a lot from this experience and hope to continue to do so and prove myself. I would really like some additional review to my latest commit with any kind of feedback, especially the kind that helps me improve, but good feedback would be welcome at this point too :). What I'm really curious about is as anyone can see by reading the comments in this bug report that I've received quite a bit of conflicting information. At this point my spec file seems simple enough that I'm not sure what would be wrong with it, any further reviews would be most helpful especially if it can provide certain evidence that any advice I'm following is indeed correct. Of course I'm still looking for a sponsor, I'm willing to be corrected, I'm willing to put in continual effort, and I'm patient and humble enough to just keep trying until I obtain my goals. Thank everyone for their help so far, it's been a great experience for me and I don't want it to stop, so again, any further review is most welcome!
[fedora-review-service-build]
You did great Michael, don't worry too much about it :). Package reviews are not exams, and it's more like we all learn from one another. A few final points: - you have an extra "}" in the %description at the end - the quotes in %pyproject_save_files do not do anything and since you are supposed to pass python modules I think it's better to drop them since those would never have spaces in them - consider using `autospec` macros [1,2]. Basically you change the `Release` to `%autorelease` and instead of writing the entries manually, put a `%autochangelog`. One thing to be aware is that all your commits will appear as entries in the changelog, so be mindful of what you write there ;). See [2] for more on how you control - please add a `-l` to the %pyproject_save_files. This makes sure that there is at least one file marked as %license retrieved through PEP639 (which will soon be finalized so not all backends have implemented it yet). If it fails to build, you may need to add `%license LICENSE` to %files and correct the `pyproject.toml` for a future release I can give you a few more tips for the project as an upstream developer. Just ping me (LecrisUT on Github) in an issue and I'll write you there. As for this package, I would mark it approved, but you still need sponsorship so it doesn't help much to do so. I will post your bug on fedora-devel matrix channel to see if anyone is free to go through your process. [1]: https://docs.pagure.org/fedora-infra.rpmautospec/autorelease.html [2]: https://docs.pagure.org/fedora-infra.rpmautospec/autochangelog.html
Taking this review.
Looking over this package and review, I see everything looks good here, so this package is APPROVED. Additionally, as I have the capability to do so, I am sponsoring you into the packager group. I also recommend that you join the Fedora Btrfs SIG matrix room as this tool is of interesting relevance to that group: https://matrix.to/#/#btrfs:fedora.im Congratulations and welcome to Fedora!
(In reply to Cristian Le from comment #21) > You did great Michael, don't worry too much about it :). Package reviews are > not exams, and it's more like we all learn from one another. > > A few final points: > - you have an extra "}" in the %description at the end > - the quotes in %pyproject_save_files do not do anything and since you are > supposed to pass python modules I think it's better to drop them since those > would never have spaces in them > - consider using `autospec` macros [1,2]. Basically you change the `Release` > to `%autorelease` and instead of writing the entries manually, put a > `%autochangelog`. One thing to be aware is that all your commits will appear > as entries in the changelog, so be mindful of what you write there ;). See > [2] for more on how you control > - please add a `-l` to the %pyproject_save_files. This makes sure that there > is at least one file marked as %license retrieved through PEP639 (which will > soon be finalized so not all backends have implemented it yet). If it fails > to build, you may need to add `%license LICENSE` to %files and correct the > `pyproject.toml` for a future release > > I can give you a few more tips for the project as an upstream developer. > Just ping me (LecrisUT on Github) in an issue and I'll write you there. > > As for this package, I would mark it approved, but you still need > sponsorship so it doesn't help much to do so. I will post your bug on > fedora-devel matrix channel to see if anyone is free to go through your > process. > > [1]: https://docs.pagure.org/fedora-infra.rpmautospec/autorelease.html > [2]: https://docs.pagure.org/fedora-infra.rpmautospec/autochangelog.html I noticed the trailing } 15 seconds after starting my last build but decided i was not going to do another build just then to fix a desciption typo. Once again I appreciate your advice you have been awesome! I'll make those new change suggestions and watch my language in my commits. You have been a tremendous help and have not only made this process far easier on me but actually rather enjoyable. Don't be surprised if I do hit you up at some point. Thanks again!
(In reply to Neal Gompa from comment #23) > Looking over this package and review, I see everything looks good here, so > this package is APPROVED. > > Additionally, as I have the capability to do so, I am sponsoring you into > the packager group. > > I also recommend that you join the Fedora Btrfs SIG matrix room as this tool > is of interesting relevance to that group: > https://matrix.to/#/#btrfs:fedora.im > > Congratulations and welcome to Fedora! Thanks! You have no idea how happy you have just made me, I've been dreaming of this since I was 10. I greatly appreciate you taking the time to review my package. I can't thank you enough for marking my package as APPROVED and then sponsoring me. I can't wait to keep learning and contributing to the Fedora Linux! I'll be following the Package Review Process docs for my next steps. Long live Fedora!
The Pagure repository was created at https://src.fedoraproject.org/rpms/btrfs-backup-ng
This appears to have been built in all releases; please link any future review requests with the initial Bodhi updates so that they close automatically. On a side note, it looks like you've been tagging your commits in dist-git, but there's no need to do so. We don't use tags for anything there, and if we ever start doing so, those might conflict in some way.
> This appears to have been built in all releases; please link any future > review requests with the initial Bodhi updates so that they close > automatically. > > On a side note, it looks like you've been tagging your commits in dist-git, > but there's no need to do so. We don't use tags for anything there, and if > we ever start doing so, those might conflict in some way. Ah yes, sorry about that, I realized after I had done it that it might had been improper based on some material I read. I didn't know about not needing to comment the dist-git repo, I apologize for my mistakes and will of course not repeat them in the future. Thank you for the information, very useful as always.