Bug 1810902 - Review Request: rpkg-macros - Set of preproc macros for rpm packages
Summary: Review Request: rpkg-macros - Set of preproc macros for rpm packages
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: chedi toueiti
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-03-06 07:29 UTC by clime7
Modified: 2020-07-20 20:13 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-07-20 20:13:37 UTC
Type: ---
chedi.toueiti: fedora-review+


Attachments (Terms of Use)

Description clime7 2020-03-06 07:29:18 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/clime/rpkg-macros/fedora-31-x86_64/01285948-rpkg-macros/rpkg-macros.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/clime/rpkg-macros/fedora-31-x86_64/01285948-rpkg-macros/rpkg-macros-0.3-1.fc31.src.rpm
Description: 
Set of preproc macros to be used by for preprocessing rpm package sources. They are designed to dynamically generate certain parts of rpm spec files based e.g. on git metadata. You can use those macros also without
rpkg by:

   $ cat <file_with_the_macros> | preproc -s /usr/lib/rpkg.macros.d/all.bash

You can also source /usr/lib/rpkg.macros.d/all.bash into
your bash environment and then you can experiment with
them directly on your command-line. See content in
/usr/lib/rpkg.macros.d to discover available macros.

Fedora Account System Username: clime

Comment 1 chedi toueiti 2020-03-07 23:01:28 UTC
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- Permissions on files are set properly.
  Note: See rpmlint output
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_permissions


===== 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". 108 files have unknown license.
     Detailed output of licensecheck in
     /mnt/56298eef-80c6-43f5-8aee-172b1d73d266/Projects/Fedora/rpkg-macros/copr-build-1285948/review-rpkg-macros/licensecheck.txt
[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
[-]: Package uses nothing in %doc for runtime.
[!]: 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]: Package is not known to require an ExcludeArch tag.
[!]: Package complies to the Packaging Guidelines
[x]: Package installs properly.
[x]: 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]: Package requires other packages for directories it uses.
[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]: 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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[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).
[ ]: Package functions as described.
[ ]: Latest version is packaged.
[ ]: Package does not include license text files separate from upstream.
[ ]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[ ]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[ ]: 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]: 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: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: rpkg-macros-0.3-1.fc33.noarch.rpm
          rpkg-macros-0.3-1.fc33.src.rpm
rpkg-macros.noarch: W: spelling-error Summary(en_US) preproc -> procedure
rpkg-macros.noarch: W: spelling-error %description -l en_US preproc -> procedure
rpkg-macros.noarch: W: spelling-error %description -l en_US utilility -> utility, ductility
rpkg-macros.noarch: W: only-non-binary-in-usr-lib
rpkg-macros.noarch: W: no-documentation
rpkg-macros.noarch: E: non-standard-executable-perm /usr/bin/pack_sources 775
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/all.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/git.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/log.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/output.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/query.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/utils.bash 644 /bin/bash 
rpkg-macros.noarch: W: no-manual-page-for-binary pack_sources
rpkg-macros.src: W: spelling-error %description -l en_US utilility -> utility, ductility
rpkg-macros.src: W: spelling-error %description -l en_US usr -> use, us, user
rpkg-macros.src:3: E: hardcoded-library-path in /usr/lib
rpkg-macros.src:42: E: hardcoded-library-path in /usr/lib/rpkg.macros.d/all.bash
rpkg-macros.src:44: E: hardcoded-library-path in /usr/lib/rpkg.macros.d/all.bash
rpkg-macros.src:47: E: hardcoded-library-path in /usr/lib/rpkg.macros.d
rpkg-macros.src: W: no-%build-section
rpkg-macros.src: W: invalid-url Source0: rpkg-macros-0.3.tar.gz
2 packages and 0 specfiles checked; 11 errors, 10 warnings.




Rpmlint (installed packages)
----------------------------
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_CTYPE = "C.UTF-8",
	LANG = "en_US.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_CTYPE = "C.UTF-8",
	LANG = "en_US.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
rpkg-macros.noarch: W: spelling-error Summary(en_US) preproc -> procedure
rpkg-macros.noarch: W: spelling-error %description -l en_US preproc -> procedure
rpkg-macros.noarch: W: spelling-error %description -l en_US utilility -> utility, ductility
rpkg-macros.noarch: W: invalid-url URL: https://pagure.io/rpkg-util.git <urlopen error [Errno -2] Name or service not known>
rpkg-macros.noarch: W: only-non-binary-in-usr-lib
rpkg-macros.noarch: W: no-documentation
rpkg-macros.noarch: E: non-standard-executable-perm /usr/bin/pack_sources 775
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/all.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/git.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/log.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/output.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/query.bash 644 /bin/bash 
rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/utils.bash 644 /bin/bash 
rpkg-macros.noarch: W: no-manual-page-for-binary pack_sources
1 packages and 0 specfiles checked; 7 errors, 7 warnings.



Requires
--------
rpkg-macros (rpmlib, GLIBC filtered):
    /usr/bin/bash
    bash
    coreutils
    findutils
    git



Provides
--------
rpkg-macros:
    rpkg-macros



Generated by fedora-review 0.7.5 (5fa5b7e) last change: 2020-02-16
Command line :/usr/bin/fedora-review --copr-build 1285948
Buildroot used: fedora-rawhide-x86_64
Active plugins: Shell-api, Generic
Disabled plugins: R, Python, Java, Ocaml, C/C++, SugarActivity, Perl, PHP, Haskell, fonts
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 2 Robert-André Mauchin 🐧 2020-03-08 19:43:36 UTC
(In reply to chedi toueiti from comment #1)
> Package Review
> ==============
>

Could you add comments about what needs fixing, it is not clear from your post. The perms issue is a false positive from rpmlint.

Comment 3 chedi toueiti 2020-03-08 20:15:26 UTC
for the permission part "E: non-executable-script" I could be wrong but I think this is due to the presence of the shebang in the bash files.
As you will be sourcing those in your environment the shebang are unecessary and could safely be removed. 
In any other use case the correct permission have to be set (https://fedoraproject.org/wiki/Packaging_tricks#Exec_permission). 

the second set of errors are related to E: hardcoded-library-path in /usr/lib it should be %{_libdir}

Comment 4 clime7 2020-03-09 01:10:44 UTC
@chedi

> for the permission part "E: non-executable-script" I could be wrong but I think this is due to the presence of the shebang in the bash files.
As you will be sourcing those in your environment the shebang are unecessary and could safely be removed. 

You are right that they are unnecessary. However, e.g. https://www.shellcheck.net/ will give you a warning if they are not included so I just
included them to satisfy the linting tool and I don't think they are harmful there, although you are right that they are not technically needed,
so idk, it's your call, I can remove them if you tell me but I would personally like them to stay if possible.

The permission error I can fix easily, there should really rather be 755 to make it the same as the other /usr/bin content

The /usr/lib thing I think I do correctly because %{_libdir} expands to on /usr/lib64 on my machine but I would like to specifically place the
library under /usr/lib because it is a noarch package. But it's true that according to this thread https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject.org/thread/AWM3ND26Q26M5OVDONQCWZBAN6AKV3MD/
I could be using %{_prefix}/lib instead of /usr/lib. I will change it too.

Comment 6 chedi toueiti 2020-03-09 09:15:05 UTC
@clime

Your last build fixed the hard coded path but not the permissions, just modify the %install section as follow and we will be good

%install
install -d %{buildroot}%{libdir}
install -d %{buildroot}%{libdir}/rpkg.macros.d
cp -ar macros.d/* %{buildroot}%{libdir}/rpkg.macros.d

install -d %{buildroot}%{_bindir}
install -p -m 755 bin/pack_sources %{buildroot}%{_bindir}/pack_sources
# fix for rpmlint permission errors
chmod a+x %{buildroot}%{libdir}/rpkg.macros.d/*.bash
chmod a+x %{buildroot}%{libdir}/rpkg.macros.d/library/*.bash

Comment 7 clime7 2020-03-09 12:03:46 UTC
Ah, I only thought about fixing bin/pack_sources permissions which were 775 and i changed it to 755.

# fix for rpmlint permission errors
chmod a+x %{buildroot}%{libdir}/rpkg.macros.d/*.bash
chmod a+x %{buildroot}%{libdir}/rpkg.macros.d/library/*.bash

I am not so sure about this cause, as you said, these files are only intended to be sourced - not executed (as they only contain functions and not a direct bash code). They contain #!/bin/bash but that's just because the linter i use doesn't like when it is missing and I think it is also not so bad bad practice to put the shebang everywhere (even into files which will be just sourced).

Can you, please, take a look again if the executable permissions are really needed on the files you mention? I think the rpmlint tool might not be 100% to the point there.

I am trying a little bit of negotiation here :).

Thank you
clime

Comment 8 chedi toueiti 2020-03-09 12:55:15 UTC
@clime,

I feel your pain, I'm not trying to be over zealous but according to https://fedoraproject.org/wiki/Packaging:ReviewGuidelines#Things_To_Check_On_Review 
"MUST: Permissions on files must be set properly. Executables should be set with executable permissions"

===================
MUST Items
Items marked as MUST are things that the package (or reviewer) MUST do. If a package fails a MUST item, that is considered a blocker. 
No package with blockers can be approved on a review. Those items must be fixed before approval can be given.
===================

I think even if I let it slide, the automated systems or official reviewers will reject it. 
I'll be posting a question in the packaging mailing list to get a second opinion.

Comment 9 chedi toueiti 2020-03-09 13:21:32 UTC
The response I got from Neal Gompa ngompa13@gmail.com on the packaging list


On Mon, Mar 9, 2020 at 8:54 AM chedi toueiti <chedi.toueiti@gmail.com> wrote:
>
> Hi,
>
> I'm reviewing a new package
> https://bugzilla.redhat.com/show_bug.cgi?id=1810902
>
> this package have bash scripts that are under /usr/lib and starting with a shebang, rpmlint is detecting the error
>
> E: non-executable-script
>
> rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/all.bash 644 /bin/bash
> rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/git.bash 644 /bin/bash
> rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/log.bash 644 /bin/bash
> rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/output.bash 644 /bin/bash
> rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/query.bash 644 /bin/bash
> rpkg-macros.noarch: E: non-executable-script /usr/lib/rpkg.macros.d/library/utils.bash 644 /bin/bash
>
>
> The author desire to keep the shebang and not set the execution bit as the files are not meant to be executable.
>
> Should I consider the package valid or not in this case.
>

In the current case, this is not acceptable. These are the two choices
I would offer:

* Those should be in /usr/share if they are architecture-independent
scripts that aren't executed (shebangs should be purged)
* Those should be in /usr/libexec in any other case

Comment 10 clime7 2020-03-09 13:47:41 UTC
@chedi

Ok, if i remove the shebangs, do the "E: non-executable-script" errors disappear? I would do it then. What do you think?

I wouldn't like to move it to entirely different path because I have some tooling that references the /usr/lib path and I think the /usr/lib path is ok (although if there is something in the Fedora packaging guidelines forbidding this, I would need to change it as well).

Comment 11 chedi toueiti 2020-03-09 14:35:38 UTC
@clime

To my knowledge /usr/lib is meant for 32bit architecture but you can store noarch scripts in it. So there is nothing forbidding it as long as they are libraries or executable scripts

for example the rpm package have scripts under it

/usr/lib/rpm/find-debuginfo.sh
/usr/lib/rpm/find-lang.sh
/usr/lib/rpm/libtooldeps.sh
/usr/lib/rpm/ocaml-find-provides.sh
/usr/lib/rpm/ocaml-find-requires.sh
/usr/lib/rpm/pkgconfigdeps.sh

but all those are standalone executables. (which is not your case)


On the other hand /usr/libexec is architecture agnostic, and should only be used as the directory for executable programs that are designed primarily to be run by other programs rather than by users (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_libexecdir), which is very close to the your use case but in the same time is sourcing really executing ??(you'll have to add the exec permission)

and finnally there is /usr/share but you'll have to remove the shebang.

I think you should move your files to either /usr/share or /usr/libexec, it's up to you to decide between removing the shebang or adding the exec bit.

Comment 12 clime7 2020-03-09 15:04:08 UTC
@chedi

if you look at e.g. /usr/lib/rpm/macros.d, there are plenty non-executable files or all the files under /usr/lib/python3.7/site-packages.

The files that rpkg-macros provide are libraries so /usr/libexec is not a good place imho.

/usr/share I recognize as a place for man pages and various assets like fonts, icons etc.

Imho, /usr/lib is a place for noarch libraries that are needed by executables under /usr/bin, which is a case here.

https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04s06.html

Comment 14 chedi toueiti 2020-03-09 15:34:16 UTC
@clime 

this is good to go, I'll be waiting for your review here https://bugzilla.redhat.com/show_bug.cgi?id=1808023

PS:
to finish your review of https://bugzilla.redhat.com/show_bug.cgi?id=1807945
you have to:
- take the bug
- change the flag to +
- change the bug status to POST

Comment 15 Gwyn Ciesla 2020-03-09 20:20:01 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rpkg-macros


Note You need to log in before you can comment on or make changes to this bug.