[Bug 2305930] New: Review Request: btrfs-backup-ng - Intelligent incremental backups for btrfs
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Bug ID: 2305930 Summary: Review Request: btrfs-backup-ng - Intelligent incremental backups for btrfs Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: trismegustis@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe... SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe... 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
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value CC| |quantum.analyst@gmail.com
--- Comment #1 from Elliott Sales de Andrade quantum.analyst@gmail.com --- 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/%%7Bpypi_name%7D/btrfs_back...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fedora@lecris.me
--- Comment #2 from Cristian Le fedora@lecris.me --- 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`?
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #3 from Elliott Sales de Andrade quantum.analyst@gmail.com --- (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/#_applicati...
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #4 from Cristian Le fedora@lecris.me --- Ack, fair point. Then instead it should have a `%py_provides`
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #5 from Michael Berry trismegustis@gmail.com --- 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/fe... SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe... 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://github.com/berrym/b | |trfs-backup-ng Keywords| |AutomationTriaged
--- Comment #6 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7937707 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #7 from Cristian Le fedora@lecris.me --- 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.p... [2]: https://spdx.org/licenses/
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #8 from Michael Berry trismegustis@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #9 from Michael Berry trismegustis@gmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe... SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe...
Koji Scratch Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=123066821
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #10 from Michael Berry trismegustis@gmail.com --- (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.
python-srpm#_195 [2]: https://spdx.org/licenses/
Thabk out Christian
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #11 from Elliott Sales de Andrade quantum.analyst@gmail.com ---
%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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #12 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2045799 --> https://bugzilla.redhat.com/attachment.cgi?id=2045799&action=edit The .spec file difference from Copr build 7937707 to 7997562
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #13 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7997562 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #14 from Michael Berry trismegustis@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #15 from Michael Berry trismegustis@gmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe... SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe...
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #16 from Elliott Sales de Andrade quantum.analyst@gmail.com --- 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)
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #17 from Michael Berry trismegustis@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #18 from Michael Berry trismegustis@gmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe... SRPM URL: https://download.copr.fedorainfracloud.org/results/mberry/btrfs-backup-ng/fe...
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #19 from Michael Berry trismegustis@gmail.com --- 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!
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #20 from Michael Berry trismegustis@gmail.com --- [fedora-review-service-build]
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |177841 (FE-NEEDSPONSOR)
--- Comment #21 from Cristian Le fedora@lecris.me --- 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
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Cristian Le fedora@lecris.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR) Depends On|177841 (FE-NEEDSPONSOR) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |ngompa13@gmail.com Flags| |fedora-review? CC| |ngompa13@gmail.com
--- Comment #22 from Neal Gompa ngompa13@gmail.com --- Taking this review.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #23 from Neal Gompa ngompa13@gmail.com --- 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!
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|177841 (FE-NEEDSPONSOR) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #24 from Michael Berry trismegustis@gmail.com --- (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.
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!
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #25 from Michael Berry trismegustis@gmail.com --- (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!
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |RELEASE_PENDING
--- Comment #26 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/btrfs-backup-ng
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |ERRATA Status|RELEASE_PENDING |CLOSED Last Closed| |2024-09-28 01:08:42
--- Comment #27 from Elliott Sales de Andrade quantum.analyst@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2305930
--- Comment #28 from Michael Berry trismegustis@gmail.com ---
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.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com