[Bug 1872867] New: Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

Bug ID: 1872867
           Summary: Review Request: stalld - thread stall detector
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

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

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

Jaroslav Škarvada  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
           Assignee|[e-mail]    |[e-mail]

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

Jaroslav Škarvada  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #1 from Clark Williams  ---

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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #2 from Jaroslav Škarvada  ---
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/#_syste...

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #3 from Clark Williams  ---
(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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #4 from Clark Williams  ---
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

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #5 from Jaroslav Škarvada  ---
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

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #6 from Jaroslav Škarvada  ---
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/#_environm...
[2] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #7 from Jaroslav Škarvada  ---
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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #8 from Jaroslav Škarvada  ---
(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*

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #9 from Clark Williams  ---
(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?

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #10 from Clark Williams  ---
(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_suppor...

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

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #11 from Jaroslav Škarvada  ---
(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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #12 from Clark Williams  ---
(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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #13 from Clark Williams  ---
Added license file (gpl-2.0.txt) and logic to install it properly.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #14 from Clark Williams  ---
Updated Makefile CFLAGS/LDFLAGS and fixed tab vs space issues

pushed latest specfile/SRPM to jcwillia.fedorapeople.org

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

Jaroslav Škarvada  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |fedora-review+

--- Comment #15 from Jaroslav Škarvada  ---
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

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #16 from Clark Williams  ---
Done. Pushed new spec/srpm to fedorapeople.org and have PR out on main source
repo

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #17 from Jaroslav Škarvada  ---
(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?

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

Neal Gompa  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #18 from Neal Gompa  ---
Please fix the changelogs to use "Name <email>" for authors.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #19 from Neal Gompa  ---
> 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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #20 from Jaroslav Škarvada  ---
I also noticed that the release CFLAGS are still not used.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #21 from Jaroslav Škarvada  ---
(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}"

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #22 from Clark Williams  ---
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.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #23 from Neal Gompa  ---
Looks fine to me now.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #24 from Jaroslav Škarvada  ---
Thanks, LGTM.

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #25 from Clark Williams  ---
Ok, On to requesting a branch!

Thank you both for your reviews!

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

--- Comment #26 from Gwyn Ciesla  ---
(fedscm-admin):  The Pagure repository was created at
https://src.fedoraproject.org/rpms/stalld

[Bug 1872867] Review Request: stalld - thread stall detector

https://bugzilla.redhat.com/show_bug.cgi?id=1872867

Clark Williams  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|---                         |RAWHIDE
        Last Closed|                            |2020-09-01 19:29:07

--- Comment #27 from Clark Williams  ---
Rawhide build done, working on f32 build now.

Closing the BZ