https://bugzilla.redhat.com/show_bug.cgi?id=2260849
Bug ID: 2260849 Summary: Review Request: stgit - Stack-based patch management for Git Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: felix@felix-maurer.de QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://fmaurer.fedorapeople.org/stgit/stgit.spec SRPM URL: https://fmaurer.fedorapeople.org/stgit/stgit-2.4.0-3.fc40.src.rpm
Description: Stacked Git, StGit for short, is an application for managing Git commits as a stack of patches.
With a patch stack workflow, multiple patches can be developed concurrently and efficiently, with each patch focused on a single concern, resulting in both a clean Git commit history and improved productivity.
Fedora Account System Username: fmaurer
Additional information: stgit has been in the Fedora repos up to f38, but has since been orphaned and retired. It got rewritten in Rust in the meantime. I'd like to get it unretired and maintain it in the future.
The current upstream version of stgit is 2.4.2 but this spec file only packages 2.4.0. This is due to the version of gix we have in Fedora (that is 0.55.2). stgit 2.4.0 depends on gix 0.54, starting from stgit 2.4.1 it depends on gix 0.56. Patching stgit 2.4.0 to use gix 0.55 is easier because that is only a dependency patch whereas patching stgit 2.4.1 (or .2) to use gix 0.55 also requires code changes. Once a new rust-gix is packaged, I'll of course update stgit.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |https://stacked-git.github. | |io/
--- Comment #1 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6970417 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Found issues:
- A package with this name already exists. Please check https://src.fedoraproject.org/rpms/stgit Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicti...
Please know that there can be false-positives.
--- 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=2260849
Paul Moore paul@paul-moore.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |paul@paul-moore.com Doc Type|--- |If docs needed, set a value
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #2 from Chuck Lever chuck.lever@oracle.com --- root@boudin:~# dnf install stgit Last metadata expiration check: 2:53:15 ago on Wed 14 Feb 2024 07:12:34 AM EST. No match for argument: stgit Error: Unable to find a match: stgit root@boudin:~# cat /etc/redhat-release Fedora release 39 (Thirty Nine) root@boudin:~#
I see that nobody is assigned to this bug. Will there be a stgit package for Fedora 39, and if so, when? If it will be a while, I can build it myself, but scraping it off the system to install the Fedora-packaged version when it shows up is always a challenge.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #3 from Paul Moore paul@paul-moore.com --- I just want to echo Chuck's comments above. I'd be happy to see stgit return to *any* Fedora release, even if it is just Rawhide at this point. The lack of stgit has been a large source of frustration for me and I'm at the point where I need to decide to start building stgit myself on my dev/test systems or move to another Linux distribution with a better development environment.
Please bring stgit back, you've got two Linux kernel maintainers asking for it :)
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #4 from Chuck Lever chuck.lever@oracle.com --- fmaurer, are you aware of : https://docs.fedoraproject.org/en-US/package-maintainers/Package_Orphaning_P... ?
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Flags| |fedora-review? CC| |decathorpe@gmail.com Assignee|nobody@fedoraproject.org |decathorpe@gmail.com
--- Comment #5 from Fabio Valentini decathorpe@gmail.com --- (In reply to Chuck Lever from comment #4)
fmaurer, are you aware of : https://docs.fedoraproject.org/en-US/package-maintainers/ Package_Orphaning_Process/#claiming_ownership_of_an_orphaned_package ?
This does not apply here, the package is *retired* (i.e. removed from Fedora), not only "orphaned" (without a maintainer).
I intended to review this package, I was just busy with other things lately. I'll do a first pass right away.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #6 from Fabio Valentini decathorpe@gmail.com --- Ok, first-pass review below.
1. You should replace the BuildRequires on "rust-packaging >= 21" with "cargo-rpm-macros". The former is ancient and no longer available, and only works for backwards compatibility.
2. If some BuildRequires are only required for running tests, then it is good form to also make them conditional:
``` %if %{with check} BuildRequires: procps-ng BuildRequires: git-core BuildRequires: git-email %endif ```
3. It is a bit hard to tell whether the build uses the correct flags because the Makefile invokes cargo with "--quiet". Would it be possible to drop that flag?
4. You're generating the license info / summary, but not using it in the spec file. The License tag is incorrect.
You could do something like this:
``` SourceLicense: GPL-2.0-only # paste license info from %cargo_license_summary here License: <construct complete license tag> ```
5. As it is currently set up, the package creates unowned directories:
``` %{_datadir}/emacs/site-lisp/stgit.el %{_datadir}/vim/vimfiles/ftdetect/stg.vim %{_datadir}/vim/vimfiles/syntax/stg*.vim ```
This results in the following directories without owners:
``` %{_datadir}/emacs/ %{_datadir}/emacs/site-lisp/ %{_datadir}/vim/ %{_datadir}/vim/vimfiles/ %{_datadir}/vim/vimfiles/ftdetect/ %{_datadir}/vim/vimfiles/syntax/ ```
You need to either depend on emacs-filesystem (which provides the first two directories) and vim-data / vim-filesystem (which provide the last four), or explicitly also own (co-own) these directories.
6. It looks like the Makefile target for running tests only runs the stgit tests, but not the actual cargo tests?
Maybe adding `%cargo_test` to the %check script would be a good idea to also run the unit tests?
7. There's a few warnings in the build.log that look like they might indicate a bug in the stgit code. Can you clarify with upstream whether this is actually the intended behaviour?
``` warning: use of deprecated method `indexmap::IndexSet::<T, S>::remove`: `remove` disrupts the set order -- use `swap_remove` or `shift_remove` for explicit behavior. --> src/stack/state.rs:256:28 | 256 | parent_set.remove(&prev_state.patches[patchname].commit.id); | ^^^^^^ | = note: `#[warn(deprecated)]` on by default
warning: use of deprecated method `indexmap::IndexSet::<T, S>::take`: `take` disrupts the set order -- use `swap_take` or `shift_take` for explicit behavior. --> src/stack/transaction/mod.rs:634:17 | 634 | old.take(pn) | ^^^^
warning: `stgit` (bin "stg") generated 2 warnings ```
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #7 from Felix Maurer felix@felix-maurer.de --- Thanks for the review! I have the following notes:
1. Will do. That was the result of an old rust2rpm.
2. Will do.
3. The actual build does not run with `--quiet`? Locally and in the copr logs I can see the rustc invocations with all the compiler flags. The output with `--quiet` is from `cargo run` for generation of completions and testing. But that always ends with "Fresh stgit", i.e., there is nothing being compiled. I can of course patch the `--quiet` out of the Makefile, but I think that adds patches and derivation from upstream for no benefit.
4. Will do.
5. I'm not sure I got the whole (un-)owned directory thing correctly. I don't think stgit should co-own these directories. emacs-filesystem is definitely missing. vim-filesystem is already Required and I only write to directories that belong to it (%{_datadir}/vim/vimfiles/ftdetect/ and %{_datadir}/vim/vimfiles/syntax/). To I still need to add vim-data for %{_datadir}/vim/ and %{_datadir}/vim/vimfiles/ which I don't directly use? Should it be my task to include that and not rather the task of vim-filesystem?
6. Will do. I don't think the cargo tests add something substantial over the stgit tests, but let's run them anyways.
7. This has been deprecated in indexmap (2.2.0) between when I wrote the spec file and now. The warnings have been fixed in upstream stgit in [1], part of 2.4.3, which we can not package right now because it needs a newer rust-gix. It's a small patch in stgit and it actually changes the semantics from swap_take/swap_remove to shift_take/shift_remove; not sure if this really fixes some bug. Should I include that patch?
[1]: https://github.com/stacked-git/stgit/commit/d20f35d0cdf3acda8570164c6e6371bd...
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #8 from Felix Maurer felix@felix-maurer.de --- Spec URL: https://fmaurer.fedorapeople.org/stgit-2.4.0-5/stgit.spec SRPM URL: https://fmaurer.fedorapeople.org/stgit-2.4.0-5/stgit-2.4.0-5.fc40.src.rpm
Some further notes: - While working on this, I noticed that all builds would print the invocations of rustc with flags, so I'm pretty sure your comment 3. doesn't really apply. - Just adding `%cargo_test` to run the unit tests would lead to stgit and a bunch of dependencies being build three times: once in the normal build, once for the unit tests (because RUST_BOOTSTRAP was different for the cargo macros and the makefile), and once again for the stgit tests (because RUST_BOOTSTRAP then switches back to the makefile state). I think I have solved the recompilation issues once and for all by passing `CARGO="%{__cargo}"` to make all the time. I don't see any recompilation now. - I included the patch from upstream to get rid of the deprecation warnings, just to be safe.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #9 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 2017284 --> https://bugzilla.redhat.com/attachment.cgi?id=2017284&action=edit The .spec file difference from Copr build 6970417 to 7027752
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #10 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/7027752 (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=2260849
--- Comment #11 from Fabio Valentini decathorpe@gmail.com --- Sorry for the delay here, I was distracted by $LIFE and urgent work stuff. The changes look good to me, and your explanation in the previous two comments makes sense. I'm running the full review now.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
Fabio Valentini decathorpe@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|fedora-review? |fedora-review+
--- Comment #12 from Fabio Valentini decathorpe@gmail.com --- Package looks good to me. Thank you!
================================================================================
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - Package does not use a name that already exists. Note: A package with this name already exists. Please check https://src.fedoraproject.org/rpms/stgit See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Naming/#_conflicting_package_names
This is OK, since this is a re-review to unretire this package.
===== MUST items =====
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [x]: Package requires other packages for directories it uses. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: The License field must be a valid SPDX expression. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [!]: Uses parallel make %{?_smp_mflags} macro. This is probably OK since the internal calls to "cargo" are themselves parallel. Other commands don't take much time and don't need to be parallelized. [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [!]: Latest version is packaged. This is OK. The packaged version is the latest version that is currently compatible with versions of dependencies that are available in Fedora. The package can be updated when the dependencies are updated. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [!]: Spec file according to URL is the same as in SRPM. Note: Spec file as given by url is not the same as in SRPM (see attached diff). This is expected for packages that use rpmautospec. [x]: Rpmlint is run on debuginfo package(s). [x]: Rpmlint is run on all installed packages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched.
Rpmlint ------- Checking: stgit-2.4.0-5.fc41.x86_64.rpm stgit-debuginfo-2.4.0-5.fc41.x86_64.rpm stgit-debugsource-2.4.0-5.fc41.x86_64.rpm stgit-2.4.0-5.fc41.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpn21f3qjm')] checks: 32, packages: 4
4 packages and 0 specfiles checked; 0 errors, 0 warnings, 16 filtered, 0 badness; has taken 0.7 s
Rpmlint (debuginfo) ------------------- Checking: stgit-debuginfo-2.4.0-5.fc41.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpleurnsxp')] checks: 32, packages: 1
1 packages and 0 specfiles checked; 0 errors, 0 warnings, 5 filtered, 0 badness; has taken 0.4 s
Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 3
stgit.x86_64: W: unused-direct-shlib-dependency /usr/bin/stg /lib64/libm.so.6 3 packages and 0 specfiles checked; 0 errors, 1 warnings, 13 filtered, 0 badness; has taken 0.4 s
Source checksums ---------------- https://github.com/stacked-git/stgit/releases/download/v2.4.0/stgit-2.4.0.ta... : CHECKSUM(SHA256) this package : 5836789617a3794f5626194e9670a24497ff5b3f9c779bd13decef3d4e1ee95d CHECKSUM(SHA256) upstream package : 5836789617a3794f5626194e9670a24497ff5b3f9c779bd13decef3d4e1ee95d
Requires -------- stgit (rpmlib, GLIBC filtered): emacs-filesystem git-core git-email ld-linux-x86-64.so.2()(64bit) libc.so.6()(64bit) libcurl.so.4()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3)(64bit) libgcc_s.so.1(GCC_4.2.0)(64bit) libm.so.6()(64bit) rtld(GNU_HASH) vim-filesystem
stgit-debuginfo (rpmlib, GLIBC filtered):
stgit-debugsource (rpmlib, GLIBC filtered):
Provides -------- stgit: stgit stgit(x86-64)
stgit-debuginfo: debuginfo(build-id) stgit-debuginfo stgit-debuginfo(x86-64)
stgit-debugsource: stgit-debugsource stgit-debugsource(x86-64)
Diff spec file in url and in SRPM --------------------------------- (diff irrelevant, caused by rpmautospec processing)
================================================================================
Recommended post-import rust-sig tasks:
- set up package on release-monitoring.org
This appears to have already been done for the previously existing package: https://release-monitoring.org/project/4892/
- add @rust-sig with "commit" access as package co-maintainer (optional)
This will cause the Rust SIG to be CC'd on all bugs for this package. Since this is a Rust package, it's probably what you want.
- set bugzilla assignee overrides to @rust-sig (optional)
For applications, you might want to *not* do this, since you're most qualified to decide about bugs and / or version updates. However, for Rust libraries, I recommend adding the @rust-sig override.
- track package in koschei for all built branches https://koschei.fedoraproject.org/search?q=stgit
I think this should resolve itself automatically once the package unretirement is processed.
================================================================================
You can proceed with filing the unretirement request with releng now. Let me know when you're ready to re-import and build the package. This package didn't use rpmautospec before it was retired, it might require special care to preserve old changelog entries correctly.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |MODIFIED
--- Comment #13 from Fedora Update System updates@fedoraproject.org --- FEDORA-2024-52b3f7c8e0 (stgit-2.4.0-1.fc41) has been submitted as an update to Fedora 41. https://bodhi.fedoraproject.org/updates/FEDORA-2024-52b3f7c8e0
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |ERRATA Status|MODIFIED |CLOSED Last Closed| |2024-03-04 15:59:23
--- Comment #14 from Fedora Update System updates@fedoraproject.org --- FEDORA-2024-52b3f7c8e0 (stgit-2.4.0-1.fc41) has been pushed to the Fedora 41 stable repository. If problem still persists, please make note of it in this bug report.
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #15 from Felix Maurer felix@felix-maurer.de --- Thank you Fabio for the reviews! stgit is now unretired and already available in rawhide.
I have created updates for [f39] and [f40] as well. @Paul, Chuck, and others: If you install stgit from the f39 update, please give feedback there so stgit makes it to stable faster. The f40 will not go to stable as long as the beta freeze lasts.
[f39]: https://bodhi.fedoraproject.org/updates/FEDORA-2024-7cdd296c3f [f40]: https://bodhi.fedoraproject.org/updates/FEDORA-2024-d38c8bed42
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #16 from Paul Moore paul@paul-moore.com --- A big thank you to Felix and Fabio, I really appreciate the work to get stgit back into Fedora - thank you!
Unfortunately the only Fedora systems I currently have installed are running Rawhide/F41 so I'm not sure I can be much help with F39 and F40, but I can say that the stgit-2.4.0-1.fc41.x86_64 package is working great so far :)
https://bugzilla.redhat.com/show_bug.cgi?id=2260849
--- Comment #17 from Chuck Lever chuck.lever@oracle.com --- Installed on F39, no bugs to report! Thank you for un-retiring this package!
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com