Spec URL: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/09847020-lesspipe/lesspipe.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/09847020-lesspipe/lesspipe-2.21-1.el9.src.rpm Description: lesspipe.sh is an input filter for the pager less. It is able to process a wide variety of file formats. It enables users to deeply inspect archives and to display the contents of files in archives without having to unpack them before. That means file contents can be properly interpreted even if the files are compressed and contained in a hierarchy of archives (often found in RPM or DEB archives containing source tarballs). The filter is easily extensible for new formats. The input filter is a bash script, but works as well as a zsh script. For zsh and bash tab completion mechanisms for archive contents are provided. Fedora Account System Username: ahaupt Successful build in COPR: https://copr.fedorainfracloud.org/coprs/ahaupt/lesspipe/build/9847020/ That's my very first package in Fedora. Following the advise at https://docs.fedoraproject.org/en-US/package-maintainers/New_Package_Process_for_New_Contributors/ I'm asking for a sponsor. I'm in close contact to the main author of the package, Wolfgang Friebel.
Copr build: https://copr.fedorainfracloud.org/coprs/build/9849530 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/09849530-lesspipe/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.
> %{bindir}/lesspipe.sh This will cause conflict, as file are shipped in less package already: $ rpm -qf /usr/bin/lesspipe.sh less-590-6.el9.x86_64 and a package in EPEL9 can't conflict nor replace files in the base distribution.
(In reply to Terje Rosten from comment #2) > > %{bindir}/lesspipe.sh > > This will cause conflict, as file are shipped in less package already: > > $ rpm -qf /usr/bin/lesspipe.sh > less-590-6.el9.x86_64 > > and a package in EPEL9 can't conflict nor replace files in the base > distribution. No, it won't conflict as bindir is defined as: %define bindir %{_prefix}/libexec/%{name} I can rename the macro if this is confusing or breaks other assumptions.
Ok, make convert those %define to %global and move them to the top of file.
(In reply to Terje Rosten from comment #4) > Ok, make convert those %define to %global and move them to the top of file. Next try, %defines are %global now: spec: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/09849759-lesspipe/lesspipe.spec srpm: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/09849759-lesspipe/lesspipe-2.21-2.el9.src.rpm
Created attachment 2116646 [details] The .spec file difference from Copr build 9849530 to 9849767
Copr build: https://copr.fedorainfracloud.org/coprs/build/9849767 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/09849767-lesspipe/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.
Anything else I should do?
version 2.22 spec: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/09926755-lesspipe/lesspipe.spec srpm: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/09926755-lesspipe/lesspipe-2.22-1.el9.src.rpm
Created attachment 2119068 [details] The .spec file difference from Copr build 9849767 to 9927143
Copr build: https://copr.fedorainfracloud.org/coprs/build/9927143 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/09927143-lesspipe/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.
Hi Andreas, thank you for introduction mail pointing us to this review. This packaging approach might not be the correct approach for this. You can avoid the conflict with the libexec, but it brings a bad user experience having to manually overwrite which lesspipe to be used. Could you open up another mail thread on devel.org to discuss the details of this plan, how to do it in Fedora and how to do it in Epel, if you can make an epel-only package that deprecates the previous one, etc. You can CC carl as the Epel expert to comment on what can be done and less-maintainers to figure out if lesspipe could be split out from this.
Hi Christan, thanks for your reply! Yes, I wasn't sure how to avoid the naming conflict best. The main idea was, users who want to profit from the the more powerful lesspipe solution install the package - and the others just don't install it ;-) But yes, this approach might be a bit naive especially in multi-user environments.
I think you need to add: BuildRequires: perl-generators Otherwise, I think you will experience this same problem that the less packaging has: $ podman run --pull=always -it fedora:rawhide Trying to pull registry.fedoraproject.org/fedora:rawhide... Getting image source signatures Copying blob 2c0edcb66eac skipped: already exists Copying config 28630f0db8 done | Writing manifest to image destination [root@3e508d0437c7 /]# dnf install -qy less-color … [root@3e508d0437c7 /]# vimcolor Can't locate File/Copy.pm in @INC (you may need to install the File::Copy module) (@INC entries checked: /usr/local/lib64/perl5/5.42 /usr/local/share/perl5/5.42 /usr/lib64/perl5/vendor_perl /usr/share/perl5/vendor_perl /usr/lib64/perl5 /usr/share/perl5) at /usr/bin/vimcolor line 19. BEGIN failed--compilation aborted at /usr/bin/vimcolor line 19. See https://docs.fedoraproject.org/en-US/packaging-guidelines/Perl/#_build_dependencies for more details.
So this package is built from https://lesspipe.org/archive/lesspipe-2.22.tar.gz where as the existing less package has: https://src.fedoraproject.org/rpms/less/blob/rawhide/f/less.spec https://github.com/wofr06/lesspipe/archive/refs/tags/v%{lesspipe_version}.tar.gz#/lesspipe-%{lesspipe_version}.tar.gz Which is the same thing. Its the same lesspipe.sh I totally think less pipe should be a separate package in a prefect world since its a different source but today it isn't. Not obvious to me why this does not contradict. If the idea is to bring a newer version back to EPEL release? You are into https://docs.fedoraproject.org/en-US/packaging-guidelines/ReviewGuidelines/#_package_review_exceptions and https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#multiple So package name would be lesspipe2.22 for instance and be declared now in here to be an EPEL only package. For a first package this is going to be awkward one .... Depends on what the intentions are.
Hi Steve, thanks for looking at this! less package contains lesspipe for newest Fedora only, atm. I agree with you, it shouldn't actually contain it. My intention was to get lesspipe into EPEL (mainly). The less versions on RHEL do not contain lesspipe, yet - just a rather trivial script with, unfortunately, the same name. I just made another copr build splitting the /etc/profile.d stuff into a separate sub package as suggested by Christian Le: https://copr.fedorainfracloud.org/coprs/ahaupt/lesspipe/build/10148623/ It also contains BuildRequires: perl-generators in order to pull in the obvious helper script dependencies. Does this make sense now? Next step would obviously be to contact less-package maintainers in order to align future less packages with this rpm. Do you agree? Thanks, Andreas
You should have a %check section in which you run test.pl. (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites) Many of the tests will fail unless some external command is installed, so you'll need to add those to BuildRequires. The remainder of the tests will be skipped unless some external command is installed. You should add those to BuildRequires if they're packaged for Fedora/EPEL (with the Fedora-only ones, if any, in an %if block). It could be argued that the package should Recommend all of those things too, but that feels like it might be too much to me. The /etc/profile.d/less.sh and less.csh provided by the less package only modify the LESSOPEN environment variable if it is not already set. I think this package should do the same. On EPEL, so that this lesspipe takes precedence over the less package's lesspipe, I would suggest doing this in files called, say, /etc/profile.d/50-lesspipe.sh and 50-lesspipe.csh, so that they run first. For the vimcolor script to work, we need: Requires: (vim-enhanced or neovim) However, vimcolor is only tried if none of the commands nvimpager, bat, batcat, pygmentize, source-highlight, are installed. Further, code2color is only tried if none of those and vim or nvim installed. Perhaps it makes sense to split each of vimcolor and code2color out into separate subpackages, then Recommend one of any of the colourizers, Suggesting whichever one we feel is best/smallest/least intrusive/highest precedence/whatever: Recommends: (bat or pygmentize or source-highlight or vimcolor or code2color) Suggests: bat (I don't know if bat is actually a good choice; this is just an example)
Thanks Peter, I tried to implement your suggestions and made a new build: https://copr.fedorainfracloud.org/coprs/ahaupt/lesspipe/build/10182892/ It contains a more current lesspipe with a new shell-based test script. Will also be in the next official lesspipe verson. It breaks on RHEL10 interestingly. According to the author Wolfgang Friebel it's a problem with the iconv release in EL10 (version 2.39). Just wonder how to deal with it. Write a patch so that these tests does not run on RHEL-10 builds?
(In reply to Andreas Haupt from comment #18) > According to the author Wolfgang Friebel it's a problem with the iconv release in EL10 (version 2.39). I doubt this, since tests 57 and 58 also fail in a fresh Rawhide container. Seems more likely that there's some unknown requirement that's, by chance, getting pulled in to to Fedora builds, but is missing from the EPEL 10 build.
(In reply to Peter Oliver from comment #19) > (In reply to Andreas Haupt from comment #18) > > > According to the author Wolfgang Friebel it's a problem with the iconv release in EL10 (version 2.39). > > I doubt this, since tests 57 and 58 also fail in a fresh Rawhide container. > Seems more likely that there's some unknown requirement that's, by chance, > getting pulled in to to Fedora builds, but is missing from the EPEL 10 build. OK, the problem was found and worked around. Looks like it was affected by this: https://github.com/gwsw/less/issues/568 Now, with the new test.sh all tests run fine (except those being ignore, e.g. because mock does not provide a color terminal ;-)) https://copr.fedorainfracloud.org/coprs/ahaupt/lesspipe/build/10198104/ Anything else? I wrote a mail to less-maintainers some days ago in order to make them aware of this review. Did not get an answer, though. I would open a ticket against less package now making them aware of this review. Maybe they change their mind again and just 'Recommend:' this lesspipe rpm in future less package releases. Thanks, Andreas
Opened https://bugzilla.redhat.com/show_bug.cgi?id=2445448
Many of the tests are still being skipping for missing dependencies that we have in Fedora. We could run more tests by BuildRequiring /usr/bin/cabextract, /usr/bin/7za, /usr/bin/bsdtar, etc. Some of the tests are skipped because archive_color and vimcolor could not be found. This package provides them itself, so perhaps %{bindir} needs adding to $PATH before running the tests? Instead of "%{_prefix}/libexec", you should write "%{_libexecdir}". I still think it would be better to automatically modify the LESSOPEN environment variable only if it is not already set. See the less package for examples of the shell code that can do this: - https://src.fedoraproject.org/rpms/less/blob/rawhide/f/less.csh - https://src.fedoraproject.org/rpms/less/blob/rawhide/f/less.sh I think the files you drop into /etc/profile.d to do this should have "lesspipe" in their names, since that's the name of the package that they belong to, and the name of the tool they are acting for.
Hi Peter, thanks for your comments! New build https://copr.fedorainfracloud.org/coprs/ahaupt/lesspipe/build/10203967/ which includes: * "%{_libexecdir}" is used now. * profile files are named /etc/profile.d/lesspipe.{sh,csh} * profile files check for empty $LESSOPEN before setting env * all rpms I could find in Fedora which are used in test.sh are now a "BuildRequires:" On rawhide this makes 94 tests succeed: 94/35/0 tests passed/ignored/failed in 7 seconds The color tests only work in a terminal supporting colors. test.sh checks if it is running in such a terminal, otherwise it ignores these tests.
Hi Andreas, a quick note about Fedora review process, please continue to use the format with `spec` and `srpm` as you did in the first few comments. That triggers the Fedora review bot to submit its own copr builds including the review.txt. Will leave the proper review for Pter since he seems to have better grasp of the package. Just noting some generic package review points: - Please add some comments on why some of the non-obvious dependencies including weak ones are being added. bat, pandoc, and libreoffice are very weird to be on that list - Similarly would be good for the BuildRequires with maybe some hints on where they were found - Heredoc files in the spec file can be quite bug prone. I would recommend a separate source file and using a `sed` to expand the macros instead - `%dir %{bindir}` is unnecessary, that would always be owned by something at the very core of the system. Generally if a hard dependency owns the directories already, co-owning the folder does not do anything for you - And the inverse issue with `%{bash_completion}` should be more explicitly a `%dir` (to avoid any surprises that a file became a directory and vice-versa) or packaging the files in it more explicitly - Using the git archives is generally preferred. You know the libxz fiasco and all that right? - For the GPL-2.0-only license, have you confirmed with upstream that that is their intent? Usually it is not clear from the license text alone if that is the case - Licensecheck for this would be fun. See this gem [1] in the vimcolor script [1]: https://github.com/wofr06/lesspipe/blob/f64d679cae63e86563802363a671cea14493d2c0/vimcolor#L12-L13
(In reply to Andreas Haupt from comment #23) > * profile files are named /etc/profile.d/lesspipe.{sh,csh} If we want these to take precedence over the settings provided by the less package (and I think we do, else these files are useless), the filenames should sort before /etc/profile.d/less.{sh,csh} (i.e., be prefixed with a number). > * all rpms I could find in Fedora which are used in test.sh are now a > "BuildRequires:" We can run even more tests with: # For %%check: BuildRequires: /usr/bin/djvutxt BuildRequires: /usr/bin/dvi2tty BuildRequires: /usr/bin/ffprobe BuildRequires: /usr/bin/h5dump BuildRequires: /usr/bin/matdump BuildRequires: /usr/bin/odt2txt BuildRequires: /usr/bin/plistutil BuildRequires: /usr/bin/ps2ascii
Thanks for all the comments! new build: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/fedora-rawhide-x86_64/10207168-lesspipe/lesspipe-2.22-8.fc45.src.rpm (In reply to Cristian Le from comment #24) > Hi Andreas, a quick note about Fedora review process, please continue to use > the format with `spec` and `srpm` as you did in the first few comments. That > triggers the Fedora review bot to submit its own copr builds including the > review.txt. Ah! Thanks for the hint! Now it hopefully works again. > Will leave the proper review for Pter since he seems to have better grasp of > the package. Just noting some generic package review points: > - Please add some comments on why some of the non-obvious dependencies > including weak ones are being added. bat, pandoc, and libreoffice are very > weird to be on that list Those were mentioned by the author Wolfgang Friebel as "nice to have" for most users. I can change that if desired. > - Similarly would be good for the BuildRequires with maybe some hints on > where they were found added a comment that these BuildRequires are needed for test.sh > - Heredoc files in the spec file can be quite bug prone. I would recommend a > separate source file and using a `sed` to expand the macros instead Put into %{SOURCE{0,1}} > - `%dir %{bindir}` is unnecessary, that would always be owned by something > at the very core of the system. Generally if a hard dependency owns the > directories already, co-owning the folder does not do anything for you %{bindir} is /usr/libexec/lesspipe atm. This is the reason I put the directory into the rpm. I want this directory to be removed in case the rpm is uninstalled. > - And the inverse issue with `%{bash_completion}` should be more explicitly > a `%dir` (to avoid any surprises that a file became a directory and > vice-versa) or packaging the files in it more explicitly Done. %dir %{bash_completion} as well as %{bash_completion}/* are now used. > - Using the git archives is generally preferred. You know the libxz fiasco > and all that right? I'm still in contact with the author Wolfgang Friebel to sort out issues. The current lesspipe-2.22 used is actually a "git archive" snapshot taken two days ago. In case all is fine with this review, I would ask the author to create a new release (2.23 most likely). But what you are saying is: I should change the %Source: from https://lesspipe.org/archive/lesspipe-%{version}.tar.gz to https://github.com/wofr06/lesspipe/archive/refs/tags/v%{version}.tar.gz right? > - For the GPL-2.0-only license, have you confirmed with upstream that that > is their intent? It is taken from: https://github.com/wofr06/lesspipe/blob/lesspipe/LICENSE > Usually it is not clear from the license text alone if that > is the case > - Licensecheck for this would be fun. See this gem [1] in the vimcolor script > > [1]: > https://github.com/wofr06/lesspipe/blob/ > f64d679cae63e86563802363a671cea14493d2c0/vimcolor#L12-L13 OK, will get back to the author to make this clear, thanks!
Created attachment 2132785 [details] The .spec file difference from Copr build 9927143 to 10207290
Copr build: https://copr.fedorainfracloud.org/coprs/build/10207290 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/10207290-lesspipe/fedora-review/review.txt Found issues: - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/lesspipe/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ 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.
> > Hi Andreas, a quick note about Fedora review process, please continue to use > > the format with `spec` and `srpm` as you did in the first few comments. That > > triggers the Fedora review bot to submit its own copr builds including the > > review.txt. > > Ah! Thanks for the hint! Now it hopefully works again. Lol, I meant more explicitly the format > Spec URL: ... > SRPM URL: ... there is a bit of freedom in the format, but I have not dig into the explicit format. And clearly @jkadlcik clearly did some black magic in there also :). > Those were mentioned by the author Wolfgang Friebel as "nice to have" for most users. I can change that if desired. Yes please do. Fedora has different $OPINIONS ;). It does raise the question, are all of those required in the tests also :-? > %{bindir} is /usr/libexec/lesspipe atm. This is the reason I put the directory into the rpm. I want this directory to be removed in case the rpm is uninstalled. Ah, I did not notice that it is `%{bindir}` and not `%{_bindir}`. Yes it is correct then. But please use a different name to avoid this confusion, particularly avoid pre-defined path macros [1]. Something like `%{exec_dir}` or `%{lesspipe_exec_dir}` is available. > https://github.com/wofr06/lesspipe/archive/refs/tags/v%{version}.tar.gz right? Yes. [1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/#macros_installation
I noticed a missing dependency: Requires and BuildRequires /usr/bin/ps. We should probably run the colour tests, given that users are most commonly going to be using lesspipe with a colour terminal: BuildRequires: bat BuildRequires: /usr/bin/dtc BuildRequires: /usr/bin/pandoc BuildRequires: /usr/bin/pygmentize BuildRequires: /usr/bin/source-highlight ... %check env TERM=xterm-256color ./test.sh %{buildroot}%{bindir}/lesspipe.sh
SRPM url: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/10216028-lesspipe/lesspipe-2.22-9.el9.src.rpm Hope I have considered all comments in this build. One word to color tests: I've not actively enabled them for the time being. One test was failing and according to the author it is planned to change these tests in the next days anyway: https://github.com/wofr06/lesspipe/pull/190#issuecomment-4032056639 However, I've prepared everything to run these tests in a future release.
Created attachment 2133102 [details] The .spec file difference from Copr build 10207290 to 10218156
Copr build: https://copr.fedorainfracloud.org/coprs/build/10218156 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/10218156-lesspipe/fedora-review/review.txt Found issues: - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/lesspipe/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ 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.
new SRPM url: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/10249849-lesspipe/lesspipe-2.22-10.el9.src.rpm now with enabled color tests which succeed. Problem was just a missing BuildRequires: /usr/bin/vim There will be a new release 2.23 soon. Then I take sources directly from Github again
Created attachment 2134406 [details] The .spec file difference from Copr build 10218156 to 10249871
Copr build: https://copr.fedorainfracloud.org/coprs/build/10249871 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/10249871-lesspipe/fedora-review/review.txt Found issues: - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/lesspipe/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ 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.
Release 2.23: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/10254224-lesspipe/lesspipe-2.23-1.el9.src.rpm In my opinion this version should be more or less final. Anything else to change from your side? I'd like to ask for your ok to create a new package repo as stated in: https://docs.fedoraproject.org/en-US/package-maintainers/New_Package_Process_for_New_Contributors/#add_package_to_source_code_management_scm_system_and_set_owner
Created attachment 2134561 [details] The .spec file difference from Copr build 10249871 to 10254245
Copr build: https://copr.fedorainfracloud.org/coprs/build/10254245 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/10254245-lesspipe/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.
Hi. > My intention was to get lesspipe into EPEL (mainly). I think this can only be an EPEL-only package. Only that allows duplicate versions of something
(In reply to Steve Traylen from comment #40) > Hi. > > > My intention was to get lesspipe into EPEL (mainly). > > I think this can only be an EPEL-only package. Only that allows duplicate > versions of something My hope is that the `lesspipe` in the `less` package can be extracted out. There were similar opinions voiced when that was included in `less`, e.g. @awilliam mentioned that in the ML thread last time. I tried to contact @mhlavink internally to get him to respond to the proposal here, but apart from his disapproval, I did not get any more information on the why that I could forward you. I see that he responded to https://bugzilla.redhat.com/show_bug.cgi?id=2445448, but I disagree with his opinion. `less` requiring `lesspipe` to function for the user is *orthogonal* to how `lesspipe` is being packaged. That is why we have `Requires`. I want to request from Michal a more detailed explanation on why the `lesspipe` should not be packaged separately and added in the `Requires` of `less`. As the spec file indicates this version has much more testing and dependencies that it can and should run. --- @Andreas, the Recommends/Suggests that the author provided are also orthogonal with whether they should be in the spec file. Unless these actively affect the execution of `lesspipe`, please remove them, otherwise please explain in more detail how they are linked to `lesspipe`. `libreoffice`, `pandoc` etc. do not seem relevant. Also please avoid using the `Requires: /usr/bin/*` and instead depend on the actual packages that provide them. This would avoid issues when multiple packages provide the same named binary, changes in the filestructure, etc.
(In reply to Cristian Le from comment #41) > @Andreas, the Recommends/Suggests that the author provided are also > orthogonal with whether they should be in the spec file. Unless these > actively affect the execution of `lesspipe`, please remove them, otherwise > please explain in more detail how they are linked to `lesspipe`. > `libreoffice`, `pandoc` etc. do not seem relevant. Those have been removed as `Suggests:` already. They are still there as `BuildRequires:` as it was stated here that most of the available tests from test.sh should also run/succeed instead of just being ignored due to missing binaries. I would still recommend bat, as it's the best choice for colored output. Would you agree with this? ``` # needed for colored output Recommends: bat ``` I will remove the current `Suggests:` which are actually needed for pdf display and vimcolor, if you insist ;-) > Also please avoid using the `Requires: /usr/bin/*` and instead depend on the > actual packages that provide them. This would avoid issues when multiple > packages provide the same named binary, changes in the filestructure, etc. OK, will do this: ``` Requires: procps-ng ``` instead of: ``` Requires: /usr/bin/ps ``` Is this all? Thanks for your time, Andreas
(In reply to Andreas Haupt from comment #42) > I would still recommend bat, as it's the best choice for colored output. > Would you agree with this? > > ``` > # needed for colored output > Recommends: bat > ``` No, that is a decision for Fedora to make via ChangeProposals, and because `lesspipe` has the possibility of being installed on all systems depending on how this review goes, you **must** keep it to the bare minimum for now. You can propose adding `bat` to the systems in a separate proposal, but please not in here. > I will remove the current `Suggests:` which are actually needed for pdf > display and vimcolor, if you insist ;-) Comment above applies to this. > Those have been removed as `Suggests:` already. They are still there as `BuildRequires:` as it was stated here that most of the available tests from test.sh should also run/succeed instead of just being ignored due to missing binaries. It seems weird at first glance, but looking at the implementation it is indeed required. Some comments on where to find the usage can be helpful for others who would come to help with the maintenance if there is any cleanup needed. You would also have to consider balancing the dependencies that may make this package rebuild when those are bumped. In Fedora infra we have the `tmt` test backend where you could move some of the tests, and afaict this package would be quite easy to do so. > Is this all? From my side, yes. But I am not familiar enough with the project to know what could have been missed. But there is still the compatibility with `less` that we either need the collaboration or the epel-only packaging. And the reviewer here, which I did not take since I think either Peter or Steve would want to see it through the final review as they had some open concerns.
(In reply to Cristian Le from comment #43) > (In reply to Andreas Haupt from comment #42) > > I would still recommend bat, as it's the best choice for colored output. > > Would you agree with this? > > > > ``` > > # needed for colored output > > Recommends: bat > > ``` > > No, that is a decision for Fedora to make via ChangeProposals, and because > `lesspipe` has the possibility of being installed on all systems depending > on how this review goes, you **must** keep it to the bare minimum for now. > You can propose adding `bat` to the systems in a separate proposal, but > please not in here. > > > I will remove the current `Suggests:` which are actually needed for pdf > > display and vimcolor, if you insist ;-) > > Comment above applies to this. New srpm with `Suggests:` & `Recommends:` removed: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/10254859-lesspipe/lesspipe-2.23-2.el9.src.rpm
Created attachment 2134574 [details] The .spec file difference from Copr build 10254245 to 10254866
Copr build: https://copr.fedorainfracloud.org/coprs/build/10254866 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/10254866-lesspipe/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 Cristian Le from comment #41) > I tried to contact @mhlavink internally ... but I disagree with his > opinion. yes, we had a private discussion and I'm well aware of the fact that you disagree with me, but that's not a reason to have this argument all over again.
(In reply to Cristian Le from comment #41) > My hope is that the `lesspipe` in the `less` package can be extracted out. > There were similar opinions voiced when that was included in `less`, e.g. > @awilliam mentioned that in the ML thread last time. > > I tried to contact @mhlavink internally to get him to respond to > the proposal here, but apart from his disapproval, I did not get any more > information on the why that I could forward you. I see that he responded to > https://bugzilla.redhat.com/show_bug.cgi?id=2445448, but I disagree with his > opinion. `less` requiring `lesspipe` to function for the user is > *orthogonal* to how `lesspipe` is being packaged. That is why we have > `Requires`. I made that same argument over at https://src.fedoraproject.org/rpms/less/pull-request/15#comment-310680 along with the following additional points, but evidently mhlavink did not find it compelling. > less and lesspipe are independent projects with different developers, > licences, bug trackers, release cycles, version numbers, etc. By > putting them into the same source package, we lose a lot: the version > number of lesspipe is obscured, release monitoring is broken, we can't > release updates independently, etc. Meanwhile, I don't think we gain > anything (other than bypassing the package review process). > Also please avoid using the `Requires: /usr/bin/*` and instead depend on the > actual packages that provide them. This would avoid issues when multiple > packages provide the same named binary, changes in the filestructure, etc. Just FYI, different packagers take different views on this, since using the path rather than package name projects you against files moving between packages, and because naming the executable is more explicit about what functionality is being required. Ultimately, I’m relaxed about which style we end up with. (In reply to Cristian Le from comment #43) > that is a decision for Fedora to make via ChangeProposals, I wonder if switching our traditional lesspipe.sh for the one from lesspipe.org deserved a change proposal itself. I imagine that some people will be surprised by it when they upgrade to Fedora 44.
(In reply to Michal Hlavinka from comment #47) > but that's not a reason to have this argument all over again. I am obviously of a different opinion as I have been trying to understand your point of view and the technical aspects that you are concerned. We are taking all of your comments into consideration and are trying to address the issues you have, it would be polite if you did the same with our concerns so we can find an amicable common ground, instead of a silent treatment. Even being explicit that you are not interested in further discussion is a better response than no response. (In reply to Peter Oliver from comment #48) > > Just FYI, different packagers take different views on this, since using the > path rather than package name projects you against files moving between > packages, and because naming the executable is more explicit about what > functionality is being required. Ultimately, I’m relaxed about which style > we end up with. Indeed, I am equally fine with either approach if the packager knows the difference and potential issues between them. Using the package names gives you better control of the upgrade path of the dependencies, e.g. if one of the package gets converted to a compat package, that can often be forgotten. > (In reply to Cristian Le from comment #43) > > that is a decision for Fedora to make via ChangeProposals, > > I wonder if switching our traditional lesspipe.sh for the one from > lesspipe.org deserved a change proposal itself. I imagine that some people > will be surprised by it when they upgrade to Fedora 44. I am not familiar enough with the differences to judge, but from the vast change in test dependencies and support, I am leaning towards a yes. It does not seem like a simple change of sources and this change seemingly circumvented the package review process which as we can see here has uncovered quite a lot of differences in the packaging process compared to the bundled one.
- lesspipe-profile should "Require: setup", for directory /etc/profile.d. - lesspipe-profile should "Require: lesspipe" of an appropriate version. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package We need to Require all of the commands that the script runs: - lesspipe should "Require: /usr/bin/file" - lesspipe should "Require: /usr/bin/head" - lesspipe should "Require: /usr/bin/iconv" - lesspipe should "Require: /usr/bin/locale" - I don’t think less is a hard requirement, so it would be better to Recommend it than Require it. - There is no need to install INSTALL; those are instructions for us (the packagers) not the end users. - Files LICENSE and vimcolor contain outdated postal addresses for the Free Software Foundation. Could you submit an upstream pull request to correct them? https://www.gnu.org/licenses/old-licenses/gpl-2.0.html#SEC1 - For completeness, I think it makes sense to pass the --libexecdir=%{lesspipe_exec_dir} option to ./configure. - It would be better to list files explicitly in the %files section, instead of relying on wildcards. In particular, packaging guideline https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists would be breached if %{lesspipe_exec_dir} was ever changed in the future to %{_bindir}. - For the vimcolor script to work, we need "Requires: (vim-enhanced or neovim); Suggests: neovim". I still think it would be sensible to split vimcolor out into a separate subpackage, since it pulls in lots of dependencies, and it only gets used if none of the other colourizers are installed. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages [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. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU General Public License, Version 2", "*No copyright* GNU General Public License, Version 2", "GNU General Public License v2.0 or later". 19 files have unknown license. Detailed output of licensecheck in /var/lib/copr- rpmbuild/results/lesspipe/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: Package must own all directories that it creates. Note: Directories without known owners: /etc/profile.d [x]: Package contains no bundled libraries or specifies bundled libraries with Provides: bundled(<libname>) if unbundling is not possible. [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. [!]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [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. Note: Documentation size is 61707 bytes in 4 files. [!]: Package complies to the Packaging Guidelines [x]: Package installs properly. [!]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [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 requires other packages for directories it uses. [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 does not use a name that already exists. [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: [x]: Reviewer should test that the package builds in mock. [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [!]: Final provides and requires are sane (see attachments). [!]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in lesspipe-profile [!]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [?]: 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]: 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]: 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: [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. Rpmlint ------- Checking: lesspipe-2.23-2.fc45.noarch.rpm lesspipe-profile-2.23-2.fc45.noarch.rpm lesspipe-2.23-2.fc45.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.8.0 configuration: /usr/lib/python3.14/site-packages/rpmlint/configdefaults.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/tmp4hzghkna')] checks: 32, packages: 3 lesspipe.src: E: spelling-error ('zsh', '%description -l en_US zsh -> sh, ssh, ash') lesspipe-profile.noarch: E: spelling-error ('zsh', '%description -l en_US zsh -> sh, ssh, ash') lesspipe-profile.noarch: W: non-conffile-in-etc /etc/profile.d/50-lesspipe.csh lesspipe-profile.noarch: W: non-conffile-in-etc /etc/profile.d/50-lesspipe.sh lesspipe-profile.noarch: W: no-documentation lesspipe.noarch: W: install-file-in-docs /usr/share/doc/lesspipe/INSTALL lesspipe.noarch: E: incorrect-fsf-address /usr/libexec/lesspipe/vimcolor lesspipe.noarch: E: incorrect-fsf-address /usr/share/licenses/lesspipe/LICENSE lesspipe.spec:98: W: configure-without-libdir-spec 3 packages and 0 specfiles checked; 4 errors, 5 warnings, 10 filtered, 4 badness; has taken 0.3 s Rpmlint (installed packages) ---------------------------- (none): E: there is no installed rpm "lesspipe-profile". ============================ rpmlint session starts ============================ rpmlint: 2.8.0 configuration: /usr/lib/python3.14/site-packages/rpmlint/configdefaults.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: 2 0 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 filtered, 0 badness; has taken 0.0 s (none): E: there is no installed rpm "lesspipe". There are no files to process nor additional arguments. Nothing to do, aborting. Source checksums ---------------- https://github.com/wofr06/lesspipe/archive/refs/tags/v2.23.tar.gz : CHECKSUM(SHA256) this package : 89b255a20a2f9a9b0145a297006487eec1a08ca68c073d21e6e5b9179ec86d16 CHECKSUM(SHA256) upstream package : 89b255a20a2f9a9b0145a297006487eec1a08ca68c073d21e6e5b9179ec86d16 Requires -------- lesspipe (rpmlib, GLIBC filtered): /usr/bin/bash /usr/bin/perl less perl(:VERSION) perl(File::Copy) perl(File::Temp) perl(Getopt::Std) perl(IO::File) perl(IPC::Open3) perl(Term::ANSIColor) perl(strict) perl(warnings) procps-ng lesspipe-profile (rpmlib, GLIBC filtered): Provides -------- lesspipe: lesspipe lesspipe-profile: lesspipe-profile Generated by fedora-review 0.11.0 (05c5b26) last change: 2025-11-29 Command line :/bin/fedora-review --no-colors --prebuilt --rpm-spec --name lesspipe --mock-config /var/lib/copr-rpmbuild/results/configs/child.cfg Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Generic Disabled plugins: PHP, Haskell, Perl, C/C++, R, fonts, Python, Java, Ocaml, SugarActivity Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH
(In reply to Andreas Haupt from comment #37) > I'd like to ask for your ok to create a new package repo To get access to do that, you'll need to find a sponsor. See https://docs.fedoraproject.org/en-US/package-maintainers/How_to_Get_Sponsored_into_the_Packager_Group/#how_to_find_a_sponsor and https://docs.fedoraproject.org/en-US/fesco/Packager_sponsor_policy/
Hi Peter, new SRPM: https://download.copr.fedorainfracloud.org/results/ahaupt/lesspipe/epel-9-x86_64/10257200-lesspipe/lesspipe-2.23-3.el9.src.rpm ... containing fixes for your suggestions. Additionally, License info should have been fixed now via https://github.com/wofr06/lesspipe/pull/195
Created attachment 2134674 [details] The .spec file difference from Copr build 10254866 to 10257234
Copr build: https://copr.fedorainfracloud.org/coprs/build/10257234 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2417806-lesspipe/fedora-rawhide-x86_64/10257234-lesspipe/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.
note that using @ in bugzilla doesn't work like it does in other systems; it doesn't just notify someone, it sets a NEEDINFO on them. Please use it sparingly.
One last thing: colour terminal detection is somewhat dependent on /usr/bin/tput, so I think the package should recommend that. Other than that, the package is approved. Thanks for sticking with it.
Dear Andreas, With this and the other unofficial reviewes in other packages you have done I am happy to sponsor you into the packages group. I have of course known you for years and years. I will press the various buttons in next couple of days. Awesome review everyone. Steve.
The Pagure repository was created at https://src.fedoraproject.org/rpms/lesspipe
FEDORA-2026-8c8b77460a (lesspipe-2.23-1.fc45) has been submitted as an update to Fedora 45. https://bodhi.fedoraproject.org/updates/FEDORA-2026-8c8b77460a
FEDORA-2026-8c8b77460a (lesspipe-2.23-1.fc45) has been pushed to the Fedora 45 stable repository. If problem still persists, please make note of it in this bug report.
I thought we had established this package cannot go in Fedora. It can only be an epel package.
(In reply to Steve Traylen from comment #61) > I thought we had established this package cannot go in Fedora. > > It can only be an epel package. Sorry, my bad. Was not really aware when exactly the switch to epel-only has to take place. "rawhide" branch has been retired, "epel10" branch was created.