Bug 1872867 - Review Request: stalld - thread stall detector
Summary: Review Request: stalld - thread stall detector
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jaroslav Škarvada
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-08-26 18:53 UTC by Clark Williams
Modified: 2020-09-01 19:29 UTC (History)
8 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2020-09-01 19:29:07 UTC
Type: ---
Embargoed:
jskarvad: fedora-review+


Attachments (Terms of Use)

Description Clark Williams 2020-08-26 18:53:39 UTC
Spec URL: https://jcwillia.fedorapeople.org/stalld.spec
SRPM URL: https://jcwillia.fedorapeople.org/stalld-1.0-1.fc32.src.rpm
Description: stalld is a daemon that monitors linux threads and detects when those threads are stalled due to cpu-starvation. The stalled thread is given a temporary boost using the SCHED_DEADLINE policy and is returned to its original policy after the boost.
Fedora Account System Username: jcwillia

Comment 1 Clark Williams 2020-08-27 23:17:10 UTC
rpmlint run on SRPM:
-----------------------------------------------------------------------------------
$ rpmlint -i redhat/SRPMS/stalld-1.0-1.fc32.src.rpm 
stalld.src: E: specfile-error error: line 2: Empty tag: Version:
This error occurred when rpmlint used rpm to query the specfile.  The error is
output by rpm and the message should contain more information.

stalld.src: E: specfile-error error: query of specfile /tmp/rpmlint.stalld-1.0-1.fc32.src.rpm.b2pufp89/stalld.spec failed, can't parse
This error occurred when rpmlint used rpm to query the specfile.  The error is
output by rpm and the message should contain more information.

1 packages and 0 specfiles checked; 2 errors, 0 warnings.
-----------------------------------------------------------------------------------

I'm not sure if the above is acceptable or not. The issue is that my Version looks like
this:

Version:	%(grep ^VERSION ../Makefile | awk '{print $3}')

meaning that I pull the version number from the main Makefile, so I don't have to maintain
it in multiple places.

Comment 2 Jaroslav Škarvada 2020-08-28 07:36:31 UTC
I took a first look:

- Version:	%(grep ^VERSION ../Makefile | awk '{print $3}')
This is not acceptable because there is no ../Makefile before the sources are downloaded and unpacked. The version tag has to be evaluable all the time even without sources.

- Source0:	%{name}-%{version}.tar.xz
This has to be full URL parseable by e.g. the 'spectool -g stalld.spec', for github you can e.g. use:
Source0:        %{url}/archive/v%{version}/%{name}-%{version}.tar.gz

This will work if there is upstream github release with the tag 'v%{version}'.

If there is no upstream github release you can still package the snapshot, but it's bit more tricky, example:

> # git hash of the snapshot
> %global git_commit f3905d3510dfb3851f946f097a9e2ddaa5fb333b
> # date when the snapshot was checked out / added to the spec
> %global git_date 20200828
> 
> %global git_short_commit %(echo %{git_commit} | cut -c -8)
> %global git_suffix %{git_date}git%{git_short_commit}
> 
> ...
> 
> # for pre-release and no upstream version, use 0
> Version: 0
> # for pre-releases use 0.1, 0.2, ....
> Release: 0.1.%{git_suffix}%{?dist}
> ...
> URL:		https://github.com/bristot/stalld
> Source0:      %{url}/archive/%{git_commit}/%{name}-%{git_suffix}.tar.gz

Again 'spectool -g stalld.spec' has to handle it.

- BuildRequires needs to have 'all' requirements, e.g. the following are missing:
gcc, make

- you shouldn't use 'rm -rf $RPM_BUILD_ROOT' just drop the line

- make DESTDIR=$RPM_BUILD_ROOT install
  make DESTDIR=$RPM_BUILD_ROOT -C redhat install

you should use:
%make_install
%make_install -C redhat

- I think you should support systemd presets (even if not used by your service) [1], i.e. add the following to the spec:
BuildRequires: systemd-rpm-macros

...
%post
%systemd_post %{name}.service

%preun
%systemd_preun %{name}.service

%postun
%systemd_postun_with_restart %{name}.service


[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_systemd

Comment 3 Clark Williams 2020-08-28 12:58:44 UTC
(In reply to Jaroslav Škarvada from comment #2)
> I took a first look:
> 
> - Version:	%(grep ^VERSION ../Makefile | awk '{print $3}')
> This is not acceptable because there is no ../Makefile before the sources
> are downloaded and unpacked. The version tag has to be evaluable all the
> time even without sources.

Ugh. I'll hard-code it to a versions string now. Still want to find a way to only maintain
version info in one place, without being Fedora-specific (has to run on Debian as well).

> 
> - Source0:	%{name}-%{version}.tar.xz
> This has to be full URL parseable by e.g. the 'spectool -g stalld.spec', for
> github you can e.g. use:
> Source0:        %{url}/archive/v%{version}/%{name}-%{version}.tar.gz
> 
> This will work if there is upstream github release with the tag
> 'v%{version}'.
> 
> If there is no upstream github release you can still package the snapshot,
> but it's bit more tricky, example:
> 

I'm about to move the cannonical location to git.kernel.org, so I'll fix this today
after that's successfull.


> > # git hash of the snapshot
> > %global git_commit f3905d3510dfb3851f946f097a9e2ddaa5fb333b
> > # date when the snapshot was checked out / added to the spec
> > %global git_date 20200828
> > 
> > %global git_short_commit %(echo %{git_commit} | cut -c -8)
> > %global git_suffix %{git_date}git%{git_short_commit}
> > 
> > ...
> > 
> > # for pre-release and no upstream version, use 0
> > Version: 0
> > # for pre-releases use 0.1, 0.2, ....
> > Release: 0.1.%{git_suffix}%{?dist}
> > ...
> > URL:		https://github.com/bristot/stalld
> > Source0:      %{url}/archive/%{git_commit}/%{name}-%{git_suffix}.tar.gz
> 
> Again 'spectool -g stalld.spec' has to handle it.
> 
> - BuildRequires needs to have 'all' requirements, e.g. the following are
> missing:
> gcc, make
> 
> - you shouldn't use 'rm -rf $RPM_BUILD_ROOT' just drop the line
> 
> - make DESTDIR=$RPM_BUILD_ROOT install
>   make DESTDIR=$RPM_BUILD_ROOT -C redhat install
> 
> you should use:
> %make_install
> %make_install -C redhat
> 
> - I think you should support systemd presets (even if not used by your
> service) [1], i.e. add the following to the spec:
> BuildRequires: systemd-rpm-macros
> 
> ...
> %post
> %systemd_post %{name}.service
> 
> %preun
> %systemd_preun %{name}.service
> 
> %postun
> %systemd_postun_with_restart %{name}.service
> 
> 
> [1]
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/
> #_systemd

Thanks Jaroslav, I'll push another round this weekend.

Comment 4 Clark Williams 2020-08-28 19:18:05 UTC
updated specfile with above concerns addressed

SOURCE0 is temporarily pointing at my fedorapeople.org page. Will be updated when we work out the tarball generation logic

Comment 5 Jaroslav Škarvada 2020-08-31 07:47:37 UTC
Package Review
==============

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


Issues:
=======
- Sources used to build the package match the upstream source, as provided
  in the spec URL.
  Note: Upstream MD5sum check error
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/

Not a blocker, please next time make the files in sync. In the review it's maybe better to bump the release and reupload the spec and srpm.

- systemd_post is invoked in %post, systemd_preun in %preun, and
  systemd_postun in %postun for Systemd service files.
  Note: Systemd service file(s) in stalld
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/Scriptlets/#_scriptlets

Probably false positive.

- Package is not compiled with the distribution CFLAGS/LDFLAGS

Please add CFLAGS/LDFLAGS variables to the Makefile.

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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.
[-]: 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]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated". 9 files have unknown license. Detailed
     output of licensecheck in /home/yarda/git-
     fedora/stalld/1872867-stalld/licensecheck.txt
[-]: License file installed when any subpackage combination is installed.

[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/stalld-1.0
     See next comment for possible solution.

[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/stalld-1.0

[!]: %build honors applicable compiler flags or justifies otherwise.
     This is serious, please add CFLAGS/LDFLAGS to the Makefile

[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.
[x]: 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.
[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.
     Note: There are rpmlint messages (see attachment).
[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]: %config files are marked noreplace or the reason is justified.
[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]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[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:
[!]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
     Details will follow in the next comment

[x]: Final provides and requires are sane (see attachments).

[?]: Package functions as described.
     Not tested

[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.
[-]: 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.
[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:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: stalld-1.0-1.fc34.x86_64.rpm
          stalld-debuginfo-1.0-1.fc34.x86_64.rpm
          stalld-debugsource-1.0-1.fc34.x86_64.rpm
          stalld-1.0-1.fc34.src.rpm
stalld.src: W: file-size-mismatch stalld-1.0.tar.xz = 20048, https://jcwillia.fedorapeople.org/stalld-1.0.tar.xz = 9856
4 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (debuginfo)
-------------------
Checking: stalld-debuginfo-1.0-1.fc34.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
(none): E: no installed packages by name stalld
(none): E: no installed packages by name stalld-debuginfo
(none): E: no installed packages by name stalld-debugsource
0 packages and 0 specfiles checked; 0 errors, 0 warnings.



Source checksums
----------------
https://jcwillia.fedorapeople.org/stalld-1.0.tar.xz :
  CHECKSUM(SHA256) this package     : ae7226b414ee43a60d6cacc5ee3546504d8d5aa5cbfada43b161a38a82f1ba48
  CHECKSUM(SHA256) upstream package : 5ed619fe366b6c66d29fd666089af9ad991ed9d92a377745ebe1345a13329311
diff -r also reports differences


Requires
--------
stalld (rpmlib, GLIBC filtered):
    /bin/sh
    config(stalld)
    libc.so.6()(64bit)
    libpthread.so.0()(64bit)
    rtld(GNU_HASH)
    systemd

stalld-debuginfo (rpmlib, GLIBC filtered):

stalld-debugsource (rpmlib, GLIBC filtered):



Provides
--------
stalld:
    config(stalld)
    stalld
    stalld(x86-64)

stalld-debuginfo:
    debuginfo(build-id)
    stalld-debuginfo
    stalld-debuginfo(x86-64)

stalld-debugsource:
    stalld-debugsource
    stalld-debugsource(x86-64)



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

Comment 6 Jaroslav Škarvada 2020-08-31 07:54:18 UTC
Despite the CFLAGS/LDFLAGS problem mentioned in the previous comment, there are few more things:

- Please make consistent use of the spaces and tabs. You used tabs as a separator for the "Name", "Version" and others tags, but spaces for the "Requires" and "BuildRequires". Please be consistent. This is not a blocker, but nice to have.

- I think the systemd configuration file should be installed into the /etc/sysconfig, i.e. /etc/sysconfig/stalld (without the .conf), not to the /etc/systemd [1]. Also please change the unit file accordingly.

- Please use just (this is to also get rid of the unowned /usr/share/stalld-1.0 directory):
%doc README.md
to install the README file to the right location (i.e. /usr/share/doc/stalld), the /usr/share/stalld-1.0 seems strange. In such case the README shouldn't be installed in the Makefile.
You could also change the Makefile to use the DOCDIR variable and then change the spec:
%install
%make_install DOCDIR=%{buildroot}%{_docdir}

%files %{_docdir}/README.md

- Nice to have (not a blocker): consider adding license file to the upstream project and installing it in the spec by the %license tag, e.g.:
%license LICENSE.txt

- You needn't list manual pages as a doc, but this is not a blocker [2]

[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#_environmentfiles_and_support_for_etcsysconfig_files
[2] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

Comment 7 Jaroslav Škarvada 2020-08-31 08:08:44 UTC
Also in the comment 5 I wasn't correct, it doesn't build on the aarch64, ppc64le and s390x:

src/stalld.c: In function 'sched_setattr':
src/stalld.c:88:17: error: '__NR_sched_setattr' undeclared (first use in this function); did you mean 'sched_setattr'?
   88 |  return syscall(__NR_sched_setattr, pid, attr, flags);
      |                 ^~~~~~~~~~~~~~~~~~
      |                 sched_setattr
src/stalld.c:88:17: note: each undeclared identifier is reported only once for each function it appears in
src/stalld.c: In function 'sched_getattr':
src/stalld.c:94:18: error: '__NR_sched_getattr' undeclared (first use in this function); did you mean 'sched_getattr'?
   94 |  return syscall (__NR_sched_getattr, pid , attr, size, flags);
      |                  ^~~~~~~~~~~~~~~~~~
      |                  sched_getattr
src/stalld.c: In function 'sched_setattr':
src/stalld.c:89:1: warning: control reaches end of non-void function [-Wreturn-type]
   89 | }
      | ^
src/stalld.c: In function 'sched_getattr':
src/stalld.c:95:1: warning: control reaches end of non-void function [-Wreturn-type]
   95 | }
      | ^
make: *** [<builtin>: src/stalld.o] Error 1

It needs to be fixed to build on all supported architectures or it nees ExcludeArch.

Comment 8 Jaroslav Škarvada 2020-08-31 08:17:20 UTC
(In reply to Jaroslav Škarvada from comment #6)
> - You needn't list manual pages as a doc, but this is not a blocker [2]
> 
> [1]
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/
> #_environmentfiles_and_support_for_etcsysconfig_files
> [2] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

But according to the [2] it MUST have wildcard (and probably also the mandir macro), the text:
> When installing man pages, note that RPM will re-compress them into its preferred format. So the %files section MUST reference manpages with a pattern that takes this into account

i.e.:
%{_mandir}/man8/stalld.8*

Comment 9 Clark Williams 2020-08-31 12:14:20 UTC
(In reply to Jaroslav Škarvada from comment #6)
> Despite the CFLAGS/LDFLAGS problem mentioned in the previous comment, there
> are few more things:
> 
> - Please make consistent use of the spaces and tabs. You used tabs as a
> separator for the "Name", "Version" and others tags, but spaces for the
> "Requires" and "BuildRequires". Please be consistent. This is not a blocker,
> but nice to have.

I use emacs so that's somewhat transparent to me. I'll go through and re-justify
the specfile and hopefully get a more consistent usage. 

> 
> - I think the systemd configuration file should be installed into the
> /etc/sysconfig, i.e. /etc/sysconfig/stalld (without the .conf), not to the
> /etc/systemd [1]. Also please change the unit file accordingly.
> 

Not sure I agree with this. The config file is strictly for the use of the unit
file and I got the impression that the customary place for these paramter files
was in /etc/systemd. Let me dig a little deeper here.


> - Please use just (this is to also get rid of the unowned
> /usr/share/stalld-1.0 directory):
> %doc README.md
> to install the README file to the right location (i.e.
> /usr/share/doc/stalld), the /usr/share/stalld-1.0 seems strange. In such
> case the README shouldn't be installed in the Makefile.
> You could also change the Makefile to use the DOCDIR variable and then
> change the spec:
> %install
> %make_install DOCDIR=%{buildroot}%{_docdir}
> 
> %files %{_docdir}/README.md

Ok

> 
> - Nice to have (not a blocker): consider adding license file to the upstream
> project and installing it in the spec by the %license tag, e.g.:
> %license LICENSE.txt

I added an SPDX tag to the source specifically so we didn't have to carry a
licence file. Do you know if there's any effort in Fedora to move the packaging
requirements to using SPDX?

Comment 10 Clark Williams 2020-08-31 16:39:01 UTC
(In reply to Clark Williams from comment #9)
> (In reply to Jaroslav Škarvada from comment #6)
> > 
> > - I think the systemd configuration file should be installed into the
> > /etc/sysconfig, i.e. /etc/sysconfig/stalld (without the .conf), not to the
> > /etc/systemd [1]. Also please change the unit file accordingly.
> > 
> 
> Not sure I agree with this. The config file is strictly for the use of the
> unit
> file and I got the impression that the customary place for these paramter
> files
> was in /etc/systemd. Let me dig a little deeper here.

Ok re-read this:  

https://fedoraproject.org/wiki/Packaging:Systemd#EnvironmentFiles_and_support_for_.2Fetc.2Fsysconfig_files

and saw that I missed the /etc/sysconfig section. Will move the config file
over to /etc/sysconfig.

Comment 11 Jaroslav Škarvada 2020-08-31 16:47:15 UTC
(In reply to Clark Williams from comment #9)
> 
> Not sure I agree with this. The config file is strictly for the use of the
> unit
> file and I got the impression that the customary place for these paramter
> files
> was in /etc/systemd. Let me dig a little deeper here.
> 
IMHO environment files are usually installed under the /etc/sysconfig, it's sysvinit legacy, but it's not IMHO explicitly written in the guidelines. I think /etc/systemd is really bad option, e.g.:
$ dnf repoquery --whatprovides '/etc/systemd/*'
Fedora Modular 31 - x86_64 - Updates                                                                    25 kB/s |  22 kB     00:00    
Fedora 31 - x86_64 - Updates                                                                           207 kB/s |  22 kB     00:00    
RPM Fusion for Fedora 31 - Free tainted                                                                 19 kB/s | 8.8 kB     00:00    
systemd-0:243-4.gitef67743.fc31.i686
systemd-0:243-4.gitef67743.fc31.x86_64
systemd-0:243.8-1.fc31.i686
systemd-0:243.8-1.fc31.x86_64

but:
$ dnf repoquery --whatprovides '/etc/sysconfig/*' | wc -l
Poslední kontrola metadat: před 0:03:29, Po 31. srpna 2020, 18:40:00 CEST.
466
 
> > 
> > - Nice to have (not a blocker): consider adding license file to the upstream
> > project and installing it in the spec by the %license tag, e.g.:
> > %license LICENSE.txt
> 
> I added an SPDX tag to the source specifically so we didn't have to carry a
> licence file. Do you know if there's any effort in Fedora to move the
> packaging
> requirements to using SPDX?

It's just optional. Regarding the SPDX I think it's not explicitly supported, but maybe better to ask on fedora-devel mailing list.

Comment 12 Clark Williams 2020-08-31 18:56:08 UTC
(In reply to Jaroslav Škarvada from comment #11)
> (In reply to Clark Williams from comment #9)
> > 
> > Not sure I agree with this. The config file is strictly for the use of the
> > unit
> > file and I got the impression that the customary place for these paramter
> > files
> > was in /etc/systemd. Let me dig a little deeper here.
> > 
> IMHO environment files are usually installed under the /etc/sysconfig, it's
> sysvinit legacy, but it's not IMHO explicitly written in the guidelines. I
> think /etc/systemd is really bad option, e.g.:
> $ dnf repoquery --whatprovides '/etc/systemd/*'
> Fedora Modular 31 - x86_64 - Updates                                        
> 25 kB/s |  22 kB     00:00    
> Fedora 31 - x86_64 - Updates                                                
> 207 kB/s |  22 kB     00:00    
> RPM Fusion for Fedora 31 - Free tainted                                     
> 19 kB/s | 8.8 kB     00:00    
> systemd-0:243-4.gitef67743.fc31.i686
> systemd-0:243-4.gitef67743.fc31.x86_64
> systemd-0:243.8-1.fc31.i686
> systemd-0:243.8-1.fc31.x86_64
> 
> but:
> $ dnf repoquery --whatprovides '/etc/sysconfig/*' | wc -l
> Poslední kontrola metadat: před 0:03:29, Po 31. srpna 2020, 18:40:00 CEST.
> 466
>  

yeah, I re-read the systemd packaging guidelines (link in c#10) and they to mention
using /etc/sysconfig. So I changed to use that. 

> > > 
> > > - Nice to have (not a blocker): consider adding license file to the upstream
> > > project and installing it in the spec by the %license tag, e.g.:
> > > %license LICENSE.txt
> > 
> > I added an SPDX tag to the source specifically so we didn't have to carry a
> > licence file. Do you know if there's any effort in Fedora to move the
> > packaging
> > requirements to using SPDX?
> 
> It's just optional. Regarding the SPDX I think it's not explicitly
> supported, but maybe better to ask on fedora-devel mailing list.

I'll talk to Daniel about adding a License file to the upstream project. Still
trying to get the tarball situation straight so our URLs can be fully kernel.org.


I pushed new spec/SRPM/tarball to jcwillia.fedorapeople.org.

Comment 13 Clark Williams 2020-08-31 19:37:16 UTC
Added license file (gpl-2.0.txt) and logic to install it properly.

Comment 14 Clark Williams 2020-09-01 00:16:23 UTC
Updated Makefile CFLAGS/LDFLAGS and fixed tab vs space issues

pushed latest specfile/SRPM to jcwillia.fedorapeople.org

Comment 15 Jaroslav Škarvada 2020-09-01 08:10:58 UTC
Thanks, there are two more minor things, please fix them before including the package, but I am approving the package now:

- %config(noreplace) /etc/sysconfig/stalld
You should use macro, e.g.:
%config(noreplace) %{_sysconfdir}/sysconfig/stalld

- it seems you used commas instead of periods in the changelog, e.g. (redhat,com, instad of redhat.com):
* Mon Aug 31 2020 williams@redhat,com - 1.0-2
- use _docdir macro for README.md
- use _mandir macro for stalld.8 manpage
- use tabs for spacing
- added push Makefile target to copy latest to upstream URL

* Tue Aug 25 2020 williams@redhat,com - 1.0-1
- rename project to stalld
- set version to 1.0
- clean up rpmlint complaints

Comment 16 Clark Williams 2020-09-01 12:03:09 UTC
Done. Pushed new spec/srpm to fedorapeople.org and have PR out on main source repo

Comment 17 Jaroslav Škarvada 2020-09-01 12:25:39 UTC
(In reply to Clark Williams from comment #16)
> Done. Pushed new spec/srpm to fedorapeople.org and have PR out on main
> source repo

Do you need Fedora sponsorship?

Comment 18 Neal Gompa 2020-09-01 12:48:34 UTC
Please fix the changelogs to use "Name <email>" for authors.

Comment 19 Neal Gompa 2020-09-01 12:49:46 UTC
> BuildRequires:	glibc-devel gcc make systemd-rpm-macros

In general, you should put each dependency on its own line, as that makes it easier for diff management as they change in git.

Comment 20 Jaroslav Škarvada 2020-09-01 13:11:20 UTC
I also noticed that the release CFLAGS are still not used.

Comment 21 Jaroslav Škarvada 2020-09-01 13:15:01 UTC
(In reply to Jaroslav Škarvada from comment #20)
> I also noticed that the release CFLAGS are still not used.

I think it's because you didn't used %configure macro, so you should edit the spec the following way:
%make_build CFLAGS="%{build_cflags}" LDFLAGS="%{build_ldflags}"

Comment 22 Clark Williams 2020-09-01 14:26:52 UTC
Updated specfile/Makefile to actually use CFLAGS/LDFLAGS
Fixed some compiler warnings for src/stalld.c
Fixed specfile changelog annotations as per #c19

Pushed stalld-1.0-3.fc32.src.rpm and specfile to fedorapeople.org

I don't *think* I need sponsorship since I used to maintain a Fedora package (it has been a while). 
Once I hear from you and Neal that I've addressed your concerns, I'll request a branch and see if
I can import the SRPM.

Comment 23 Neal Gompa 2020-09-01 17:24:11 UTC
Looks fine to me now.

Comment 24 Jaroslav Škarvada 2020-09-01 18:02:56 UTC
Thanks, LGTM.

Comment 25 Clark Williams 2020-09-01 18:30:47 UTC
Ok, On to requesting a branch!

Thank you both for your reviews!

Comment 26 Gwyn Ciesla 2020-09-01 18:52:59 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/stalld

Comment 27 Clark Williams 2020-09-01 19:29:07 UTC
Rawhide build done, working on f32 build now. 

Closing the BZ


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