Bug 974714 - Review Request: thermal_daemon - A close loop thermal monitoring and control daemon
Summary: Review Request: thermal_daemon - A close loop thermal monitoring and control ...
Keywords:
Status: CLOSED DUPLICATE of bug 1440406
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 974715 (view as bug list)
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2013-06-14 23:06 UTC by Srinivas Pandruvada
Modified: 2020-07-17 16:00 UTC (History)
11 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-07-16 21:41:30 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
New spec file, please review. (2.21 KB, text/plain)
2015-02-13 21:39 UTC, Eric Griffith
no flags Details

Description Srinivas Pandruvada 2013-06-14 23:06:22 UTC
Spec URL: http://spandruvada.fedorapeople.org/thermal_daemon.spec
SRPM URL: http://spandruvada.fedorapeople.org/thermal_daemon-1.02-1.fc19.src.rpm 
koji_build_results: http://spandruvada.fedorapeople.org/koji_build_results

Description: Thermal Daemon is a Linux daemon for monitoring and controlling platform temperatures. Once the system temperature reaches a certain threshold, the Linux daemon activates various cooling methods to try to cool the system. 

More details can be obtained from:
https://01.org/linux-thermal-daemon/documentation/introduction-thermal-daemon
http://www.linux.com/news/featured-blogs/200-libby-clark/721494-linux-thermal-daemon-monitors-and-controls-temperature-in-tablets-laptops
http://www.phoronix.com/scan.php?page=news_item&px=MTM2OTk

It operates in two modes:
Daemon acts to cool the system under the following two modes :
1. Zero Configuration Mode
2. User defined configuration mode

1. Zero Configuration Mode
For most users, this should be enough to bring the CPU temperature of the system under control. This uses DTS temperature sensor and uses Intel P state driver,
Power clamp driver, Running Average Power Limit control and cpufreq as cooling methods.
2. User defined configuration mode
This allows ACPI style configuration in a thermal XML configuration file. This can be used to fix the buggy ACPI configuration or fine tune by adding more sensors and cooling devices.
This is a first step in implementing a close loop thermal control in user mode and can be enhanced based on community feedback and suggestions.

This is my first package and seeking a sponsorship.

Fedora Account System Username: spandruvada

Comment 1 Srinivas Pandruvada 2013-06-14 23:20:42 UTC
*** Bug 974715 has been marked as a duplicate of this bug. ***

Comment 2 Marcelo Barbosa "firemanxbr" 2013-06-15 04:26:58 UTC
I'm not officially Fedora packager yet, here my informal review:

1) Remove "BuildRequires:  gcc-c++", this not require this it essential part for minimum build system.
 
2) Will your package build from EPEL5 ?
if no, remove "%defattr(-,root,root)" in %files, this option only EPEL5.

3) Adjust your %changelog in format like:
from:
"* Wed May 8 2013 Base version 1.0-1"
To something like:
"* Wed May 8 2013 Srinivas Pandruvada <srinivas.pandruvada.com> 1.0-1
- Initial package"

In next adjusts increment your release this package, example 1.0-2.

Best regards.

Marcelo Barbosa
Fedora Project Ambassador
firemanxbr

Comment 3 Antonio T. (sagitter) 2013-06-15 09:16:14 UTC
Hi Srinivas.

> 1) Remove "BuildRequires:  gcc-c++", this not require this it essential part
> for minimum build system.
>  
> 2) Will your package build from EPEL5 ?
> if no, remove "%defattr(-,root,root)" in %files, this option only EPEL5.
> 
> 3) Adjust your %changelog in format like:
> from:
> "* Wed May 8 2013 Base version 1.0-1"
> To something like:
> "* Wed May 8 2013 Srinivas Pandruvada <srinivas.pandruvada.com>
> 1.0-1
> - Initial package"
> 
> In next adjusts increment your release this package, example 1.0-2.
> 

Also,

- URL/Source* refer to project's website and source files' location respectively.
- Your package provides two .service files; please, refer to    http://fedoraproject.org/wiki/Packaging:Systemd#Packaging guidelines

Comment 4 Marcelo Barbosa "firemanxbr" 2013-06-15 14:55:16 UTC
Hi Srinivas Pandruvada, 

   Now i'm packager official. I go review your package, follow this comments in this ticket, questions: i'm available.

Marcelo Barbosa
mr.marcelo.barbosa

Comment 5 Marcelo Barbosa "firemanxbr" 2013-06-18 20:05:33 UTC
Hi Srinivas Pandruvada, 

Anything new? i can review this package, but some errors where description in other comments, please adjust your package.

Thank you.

Marcelo Barbosa
Fedora Project Packager
Fedora Project Ambassador
firemanxbr

Comment 6 Srinivas Pandruvada 2013-06-18 20:15:05 UTC
Hi Marcelo,

I am working on your comments. Sorry, it is taking time as I am new to this package building and submission.

Thanks,
Srinivas

Comment 7 Marcelo Barbosa "firemanxbr" 2013-06-18 20:18:00 UTC
Srinivas,

   No problem, i can help you, questions send for me.
   
Thank you and good working.

Marcelo Barbosa
Fedora Project Packager
Fedora Project Ambassador
firemanxbr

Comment 8 Srinivas Pandruvada 2013-06-19 00:03:29 UTC
Updated:
http://spandruvada.fedorapeople.org/thermal_daemon.spec
http://spandruvada.fedorapeople.org/thermal_daemon-1.02-2.fc19.src.rpm

==============
diff --git a/rpms/thermal_daemon.spec b/rpms/thermal_daemon.spec
index f81a1b7..a55f2d5 100644
--- a/rpms/thermal_daemon.spec
+++ b/rpms/thermal_daemon.spec
@@ -1,26 +1,30 @@
 Name:           thermal_daemon
 Version:        1.02
-Release:        1%{?dist}
+Release:        2%{?dist}
 Summary:        The "Linux Thermal Daemon" program from 01.org
 
 License:        GPLv2+
-URL:            http://spandruvada.fedorapeople.org/
-Source0:        http://spandruvada.fedorapeople.org/%{name}-%{version}.tar.gz
+URL:            https://github.com/01org/thermal_daemon
+%global commit  78b9dd1343bf7350740eb76e3bf87dc207078d9d
+%global shortcommit %(c=%{commit}; echo ${c:0:7})
+Source0:        https://github.com/01org/thermal_daemon/archive/%{commit}/%{name}-v%{version}.tar.gz
 
+BuildRequires:  automake
+BuildRequires:  autoconf
 BuildRequires:  glib-devel
 BuildRequires:  dbus-glib-devel
 BuildRequires:  libxml2-devel
-BuildRequires:  automake
-BuildRequires:  autoconf
-BuildRequires:  gcc-c++
+BuildRequires:  systemd
 Requires:       libxml2
+Requires(post): systemd-units
+Requires(preun): systemd-units
+Requires(postun): systemd-units
 
 %description
 Thermal Daemon monitors and controls platform temperature.
 
 %prep
-%setup -q
-
+%setup -qn %{name}-%{version}
 
 %build
 autoreconf -f -i
@@ -32,9 +36,16 @@ make %{?_smp_mflags}
 rm -rf $RPM_BUILD_ROOT
 %make_install DESTDIR=$RPM_BUILD_ROOT
 
+%post
+%systemd_post thermald.service
+
+%preun
+%systemd_preun thermald.service
+
+%postun
+%systemd_postun_with_restart thermald.service 
 
 %files
-%defattr(-,root,root)
 %{_bindir}/thermald
 %config(noreplace) %{_sysconfdir}/dbus-1/system.d/org.freedesktop.thermald.conf 
 %{_datadir}/dbus-1/system-services/org.freedesktop.thermald.service
@@ -45,6 +56,7 @@ rm -rf $RPM_BUILD_ROOT
 %{_unitdir}/thermald.service
 
 %changelog
-* Wed May 8 2013 Base version 1.0-1
-- Base version
-- updated to vesrion 1.02
+* Tue Jun 18 2013 Srinivas Pandruvada <srinivas.pandruvada.com> 1.02-2
+- Update spec file after first review
+* Fri Jun 14 2013 Srinivas Pandruvada <srinivas.pandruvada.com> 1.02-1
+- Initial package

=============

Question: Is "Documentation=" field in the thermald.service is mandatory?

I hope this is better than the previous.

Comment 9 Marcelo Barbosa "firemanxbr" 2013-06-19 06:05:18 UTC
Hello Srinivas,

   Your release is much better for this last version, but necessary see this it:

Issues:
=======

1) Sources used to build the package match the upstream source, as provided in
  the spec URL.

  Note: Upstream MD5sum check error, diff is in

  Only in /home/marcelo.barbosa/rpmbuild/SOURCES/reviews/974714-thermal_daemon/srpm-unpacked/thermal_daemon-v1.02.tar.gz-extract: thermal_daemon-1.02
  Only in /home/marcelo.barbosa/rpmbuild/SOURCES/reviews/974714-thermal_daemon/upstream-unpacked/Source0: thermal_daemon-78b9dd1343bf7350740eb76e3bf87dc207078d9d

  Source checksums
  ----------------
 https://github.com/01org/thermal_daemon/archive/78b9dd1343bf7350740eb76e3bf87dc207078d9d/thermal_daemon-v1.02.tar.gz :
  CHECKSUM(SHA256) this package     : d870160e9d0518e2cc0e0fdc948e9feb28112638de70f696f393683a913893c2
  CHECKSUM(SHA256) upstream package : 4c0beba121cb2769a6a423e2d861bbf1210d717c631d57d21e9001ad12205cd2
diff -r also reports differences

  Please see: http://fedoraproject.org/wiki/Packaging/SourceURL
  
  PS: You not use variable shortcommit in this line: 
Source0:        https://github.com/01org/thermal_daemon/archive/%{commit}/%{name}-v%{version}.tar.gz

2) Please remove this line, is not necessary:

   Requires:       libxml2

ERROR: thermal_daemon.x86_64: E: explicit-lib-dependency libxml2

3) About this question is mandatory, please see:
http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

Thank you and good working.

Marcelo Barbosa
Fedora Project Packager
Fedora Project Ambassador
firemanxbr

Comment 10 Srinivas Pandruvada 2013-06-19 22:38:10 UTC
Hi Marcelo,

I have uploaded new spec file and source rpm. I did a diff -r on the tar file from what is generated and what is obtained from gitweb. There is no difference, but checksum doesn't match.

I see at https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL a comment:
"
Keep in mind that github tarballs are generated on-demand, so their modification dates will vary and cause checksum tests to fail. Reviewers will need to use diff -r to verify the tarballs.
"


Also added documentation field in the systemd service file.


Thanks,
Srinivas

Comment 11 Christopher Meng 2013-06-20 00:13:36 UTC
Some suggestions:

1. In %build section,

prefix=/usr, please change it to %{_prefix}

2. In %install section, 

Please remove "rm -rf $RPM_BUILD_ROOT"

3. And, please replace $RPM_BUILD_ROOT by %{buildroot}

Comment 12 Srinivas Pandruvada 2013-06-20 16:08:02 UTC
Thanks. Did the suggested changed and submitted new spec file source rpm.
Let me know if there are any other suggestions.

Comment 13 Marcelo Barbosa "firemanxbr" 2013-06-20 16:14:50 UTC
Srinivas,

   Please post in commentary your new updates:

INFO: Processing bugzilla bug: 974714
INFO: Getting .spec and .srpm Urls from : 974714
INFO:   --> SRPM url: http://spandruvada.fedorapeople.org/thermal_daemon-1.02-2.fc19.src.rpm
INFO:   --> Spec url: http://spandruvada.fedorapeople.org/thermal_daemon.spec
INFO: Using review directory: /home/marcelo.barbosa/rpmbuild/SOURCES/reviews/974714-thermal_daemon
INFO: Downloading .spec and .srpm files
ERROR: 'Error 404 downloading http://spandruvada.fedorapeople.org/thermal_daemon-1.02-2.fc19.src.rpm'

   Example:

   url with .spec file
   url with src rpm file 

PS: not necessary diff command.

Marcelo Barbosa

Comment 14 Srinivas Pandruvada 2013-06-20 17:01:48 UTC
Hi Marcelo,

Spec URL: http://spandruvada.fedorapeople.org/thermal_daemon.spec
SRPM URL: http://spandruvada.fedorapeople.org/thermal_daemon-1.02-4.fc19.src.rpm

I didn't understand "post in commentary". I see you are firing some build from bugzilla, how do you do that?

I can't attach Koji results as it will not work behind our firewall at work.

Thanks for your patience.

Srinivas

Comment 15 Marcelo Barbosa "firemanxbr" 2013-06-20 17:42:20 UTC
Srinivas,

   You execute new changes in your .spec file, do you have generate your packages again:

   $ rpmlint -bs .spec.file
  
   And:

   $ rpmlint -ba .spec.file

   Look this error again:

OUTUPUT:
  
Rpmlint
-------
Checking: thermal_daemon-1.02-4.fc18.x86_64.rpm
thermal_daemon.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint thermal_daemon
thermal_daemon.x86_64: E: explicit-lib-dependency libxml2
thermal_daemon.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 1 errors, 1 warnings.
# echo 'rpmlint-done:'

   Look this output cacater "E" = ERROR, but "W" = WARNING, in this package your "W" is false positive.

PS: Other commentary this output from fedora-review tool.

Marcelo Barbosa

Comment 16 Srinivas Pandruvada 2013-06-20 21:13:47 UTC
Hi Marcelo,
I did regenerate and upload. I don't see this error. This is what I did to test the uploaded SRPM:
- Emptied my ~/rpmbuild sub-folder contents. So no RPM or SPEC file, tar files exists
- wget http://spandruvada.fedorapeople.org/thermal_daemon-1.02-4.fc19.src.rpm
- rpm -ivh thermal_daemon-1.02-4.fc19.src.rpm
- This installed source in ~/rpmbuild/SOURCES and spec file in ~/rpmbuild/SPECS
- rpmbuild -bb /home/spandruvada/rpmbuild/SPECS/thermal_daemon.spec
- It generated thermal_daemon-1.02-4.fc19.x86_64.rpm and 
thermal_daemon-debuginfo-1.02-4.fc19.x86_64.rpm
- rpm -qa | grep thermal ,  shows the installed package
-  rpmlint /home/spandruvada/rpmbuild/SPECS/thermal_daemon.spec thermal_daemon-1.02-4.fc19.src.rpm
thermal_daemon.src: W: file-size-mismatch thermal_daemon-v1.02-121414e.tar.gz = 760874, https://github.com/01org/thermal_daemon/archive/121414ee4aef9cfea6817968780701531ebf814e/thermal_daemon-v1.02-121414e.tar.gz = 758973
1 packages and 1 specfiles checked; 0 errors, 1 warnings.

(As expected)

- rpmlint thermal_daemon
thermal_daemon.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

I don't see libxml2 error.

Comment 17 Marcelo Barbosa "firemanxbr" 2013-06-21 05:04:42 UTC
Srinivas, 

   Please add in this ticket one new block with: FE-NEEDSPONSOR
   With new block you get one sponsor for grant your for packager.
   Your package is almost finish, only two adjustments, see below this items with [!]:

====================================================================
- the rpmlint is false positive in: W: only-non-binary-in-usr-lib

Package Review
==============

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

===== MUST items =====
C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[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.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "GPL", "GPL (v2)", "Unknown or generated". 3 files have unknown license.
     Detailed output of licensecheck in
     /home/marcelo.barbosa/rpmbuild/SOURCES/reviews/974714-thermal_daemon/licensecheck.txt
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).

[!]: Package is named according to the Package Naming Guidelines.
please see: https://fedoraproject.org/wiki/Packaging:NamingGuidelines?rd=Packaging/NamingGuidelines

[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]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[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.
[-]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[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]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[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 %doc.
[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 do not use a name that already exist
[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
[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).

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.

[!]: %check is present and all tests pass.
test_pref.sh
[marcelo.barbosa@firelap test]$ pwd
/home/marcelo.barbosa/rpmbuild/SOURCES/reviews/974714-thermal_daemon/srpm-unpacked/thermal_daemon-v1.02-121414e.tar.gz-extract/thermal_daemon-121414ee4aef9cfea6817968780701531ebf814e/test

[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Uses parallel make.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[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: thermal_daemon-1.02-4.fc18.x86_64.rpm
thermal_daemon.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint thermal_daemon
thermal_daemon.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
thermal_daemon (rpmlib, GLIBC filtered):
    /bin/sh
    config(thermal_daemon)
    libc.so.6()(64bit)
    libdbus-1.so.3()(64bit)
    libdbus-glib-1.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgmodule-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgomp.so.1()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libxml2.so.2()(64bit)
    libxml2.so.2(LIBXML2_2.4.30)(64bit)
    libxml2.so.2(LIBXML2_2.6.0)(64bit)
    rtld(GNU_HASH)
    systemd-units



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



Source checksums
----------------
https://github.com/01org/thermal_daemon/archive/121414ee4aef9cfea6817968780701531ebf814e/thermal_daemon-v1.02-121414e.tar.gz :
  CHECKSUM(SHA256) this package     : 7be4249f07e151ab8d4e3140cbc09360456fada5455b30488baa589827398cac
  CHECKSUM(SHA256) upstream package : 73d8c2744f8d5fb653010b666489863b488cb49777b0a41e36d669c27241e061
However, diff -r shows no differences


Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-18-x86_64
Command line :/usr/bin/fedora-review -b 974714

====================================================================

Comment 18 Srinivas Pandruvada 2013-06-21 15:33:20 UTC
Hi Marcelo,
What is the failure "[!]: %check is present and all tests pass." means?

Thanks,
Srinivas

Comment 19 Srinivas Pandruvada 2013-06-21 15:44:20 UTC
Also
For the package naming guidlines:

- Common Character Set for Package Naming : Meets this requirment

When naming a package, the name should match the upstream tarball or project: Meets this requirment


When naming packages for Fedora, the maintainer must use the dash '-' as the delimiter for name parts. The maintainer must NOT use an underscore '_', a plus '+', or a period '.' as a delimiter. Version numbers used in compat libraries do not need to omit the dot '.' or change it into a dash '-':

This seems to be issue. The problem is that if remove underscore here, then it will not match the upstream tar ball name. What can be done about this?

Thanks for your help and patience.

Thanks,
Srinivas

Comment 20 Christopher Meng 2013-06-21 18:45:22 UTC
IMO If I package this program,  I'll name it to thermals. 

And I found the service file is also named thermald.

Underscore is bad, yes. 

So just change them, and define a new macro like:

%global pkgname thermal_daemon 

Then use this new macro in SourceURL and other places.

Comment 21 Marcelo Barbosa "firemanxbr" 2013-06-21 19:50:13 UTC
Srinivas, 

   Besides the name also confer the other problem(%check) your source have dir "/test" but your spec file no.

Marcelo Barbosa

Comment 22 Michael Schwendt 2013-06-21 19:53:33 UTC
Re: comment 19

Take a look at

  https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Separators

and notice what it says towards the bottom of that section:

| There are a few exceptions to the no underscore '_' rule. 
|
| [...]
| * packages where the upstream name naturally contains an underscore
| are excluded from this. Examples of these packages include:
|
| [...]
|
| If in doubt, ask on fedora-devel-list. 

# rpm -qa --qf %{n}\\n \*_\*|sort
libart_lgpl
libcom_err
libini_config
libipa_hbac
libnetfilter_conntrack
libpath_utils
libref_array
libsss_idmap
microcode_ctl
pam_krb5
pam_pkcs11
python-backports-ssl_match_hostname
sg3_utils-libs
tcp_wrappers
tcp_wrappers-libs
usb_modeswitch
usb_modeswitch-data
wpa_supplicant

Comment 23 Yohan Graterol 2013-06-22 07:09:35 UTC
Hi Marcelo,

Good job! But, you can not assigned this package. Only a sponsor can do formal review.

Comment 24 Srinivas Pandruvada 2013-06-24 15:48:59 UTC
David Woodhouse (dwmw2) is willing to sponsor me. So I will update spec and srpm files after I pass all fedora-review tests.

Thanks to Marcelo and Christopher to spend time and help me to learn submission process.

Comment 25 Srinivas Pandruvada 2013-06-24 23:47:54 UTC
Spec URL: http://spandruvada.fedorapeople.org/thermal-daemon.spec
SRPM URL: http://spandruvada.fedorapeople.org/thermal-daemon-1.02-5.fc19.src.rpm

Removed underscore in package name.
Added comment for %check as there is no self test exist in the upstream repo.

Comment 26 Srinivas Pandruvada 2013-10-01 18:31:56 UTC
Updated to thermal daemon version 1.03.
Spec URL: http://spandruvada.fedorapeople.org/thermal-daemon.spec
SRPM URL: http://spandruvada.fedorapeople.org/thermal-daemon-1.03-1.fc19.src.rpm

Comment 27 Piruthiviraj Natarajan 2014-04-27 14:28:01 UTC

@srinivas

I see updated package release from upstream. Do you mind submitting an updated version here?

Comment 28 Eric Griffith 2015-02-11 19:56:23 UTC
Hey guys, im tempted to try and take over / reviving packaging of thermal_daemon. It'd be my first attempt at packaging though so would anyone be willing to be kind of be my guide in the matter?

Comment 29 Srinivas Pandruvada 2015-02-11 21:08:43 UTC
Eric Griffith : Feel free to take over.

Comment 30 Eric Griffith 2015-02-13 19:38:49 UTC
Question for anyone keeping an eye on this. Srinivas I've got your spec file on hand as a reference but I've got a bit of a general question that I'd love to have answered beyond just this one .spec file. 

Currently it says:

Source0:        https://github.com/01org/thermal_daemon/archive/v%{version}.tar.gz

But when I do "rpmbuild -ba thermal_daemon.spec" (following Fedora Documentation http://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/index.html ) then rpmbuild complains about:

error: File /home/egriffith/rpmbuild/SOURCES/v1.30.tar.gz: No such file or directory

presumably because its just looking at "v%{version}.tar.gz". What is the appropriate fix here? The upstream source is in fact:

https://github.com/01org/thermal_daemon/archive/v%{version}.tar.gz (in this case v1.30.tar.gz) 

but the download link gives you "thermal_daemon-1.3.tar.gz" which is in my SOURCES directory. So do I rename thermal_daemon-1.3.tar.gz to be "v1.30.tar.gz" or do I modify "Source0" to not be 100% faithful to the actual URL? Or is there an override I can use to specify that the downloaded files have a different name than the upstream source URL?

Comment 31 Eric Griffith 2015-02-13 21:39:26 UTC
Created attachment 991607 [details]
New spec file, please review.

Disclaimer: First time writing a spec file, tried to follow all available guidelines. This builds, installs and works fine so i know this isn't broken just want to make it accurate as per guidelines & assumptions.  

A handful of questions in regards to the spec file format: 

1) rpmlint complains about source0 not being a valid URL, but it rpmbuild can't find the source if I have it correct... see my above post in regards to that situation. 

2) %build section. The original spec file had 'autoreconf' the compilation instructions had ./autogen.sh. Which is the correct one to use? Current spec uses autogen.sh

3) Is a "BuildRequires: systemd" necessary or is it just redundant? 

4) Can someone link or explain the Requires(post)/(preun)/(postun), same with %post, %preun, %postun

5) %files does every single file need to be listed here or just 'important ones? I left out /usr/share/src, /usr/lib and a couple others.

6) Upstream name is "thermal_daemon" but I see it packaged elsewhere as "thermald" and the binary / .service is thermald. Preference goes to...?

Comment 32 jeremy9856 2017-01-28 09:19:16 UTC
Is there still something that block the package to be add in the repo ?

Comment 33 Srinivas Pandruvada 2017-01-30 02:32:36 UTC
Someone needs to be maintainer who can gets a sponsor or already a maintainer.

I tried 2 and half years back, but I can't be maintainer for this package for Fedora anymore because of my schedule.

So anybody wants to own it, please feel free to take over.

Comment 34 jeremy9856 2017-06-25 06:43:39 UTC
I'm surprised that nobody want to take care of this package because it's a fairly important one.

Comment 36 jeremy9856 2017-06-25 09:20:26 UTC
Well, it seem someone took care of it :)
I guess that can be closed now.

Thanks !

Comment 37 Petr Menšík 2020-07-16 21:41:30 UTC
(In reply to Jens Lody from comment #35)
> https://bugzilla.redhat.com/show_bug.cgi?id=1440406

I am closing this review, because thermald package seems to contain Srinivas Pandruvada's commits on upstream. I take that as proof this package is indeed [1].
In case I am mistaken, please reopen the review.

1. https://github.com/intel/thermal_daemon

*** This bug has been marked as a duplicate of bug 1440406 ***


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