Bug 904843 - Review Request: acpica-tools - ACPICA tools for the development and debug of ACPI tables
Summary: Review Request: acpica-tools - ACPICA tools for the development and debug of ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Peter Robinson
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 924442
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-01-27 20:17 UTC by Al Stone (Old account - use ahs3@redhat.com)
Modified: 2013-09-21 16:50 UTC (History)
7 users (show)

Fixed In Version: acpica-tools-20130823-2.fc19
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-09-07 01:27:23 UTC
Type: ---
Embargoed:
pbrobinson: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Al Stone (Old account - use ahs3@redhat.com) 2013-01-27 20:17:23 UTC
Reason for Review: this is my very first Fedora package and I am seeking
sponsorship

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130123-1.f18.src.rpm

Description:
The ACPI Component Architecture (ACPICA) project provides an OS-independent
reference implementation of the Advanced Configuration and Power Interface
Specification (ACPI).  ACPICA code contains those portions of ACPI meant to
be directly integrated into the host OS as a kernel-resident subsystem, and
a small set of tools to assist in developing and debugging ACPI tables.

This package contains only the user-space tools needed for ACPI table
development, not the kernel implementation of ACPI.  The following commands
are installed:
   -- iasl: compiles ASL (ACPI Source Language) into AML (ACPI Machine
      Language), suitable for inclusion as a DSDT in system firmware.
      It also can disassemble AML, for debugging purposes.
   -- acpibin: performs basic operations on binary AML files (e.g.,
      comparison, data extraction)
   -- acpiexec: simulate AML execution in order to debug method definitions
   -- acpihelp: display help messages describing ASL keywords and op-codes
   -- acpinames: display complete ACPI name space from input AML
   -- acpisrc: manipulate the ACPICA source tree and format source files
      for specific environments
   -- acpixtract: extract binary ACPI tables from acpidump output (see
      also the pmtools package)


Fedora Account System Username: ahs3

NB: the intent would be for this package to supercede the iasl package which
(a) only ships the iasl tool mentioned above from the same upstream source,
and (b) is a little bit dated.  This would be the more complete version of
the ACPICA tools.

NB: this package does work on x86_64; it may or may not work properly on
i386 or ARM.  Part of getting this package into Fedora is to provide for such
additional testing and/or porting as needed.

NB: the pmtools package contains an another version of acpixtract, also
quite dated.  The intent would be to either supercede it, or as shown in
the spec file, have the version from this package as an alternative.

The x86_64 version does have a clean scratch build (task #4906409).

Comment 1 Antonio T. (sagitter) 2013-01-27 21:20:21 UTC
Hi Al.

>NB: this package does work on x86_64; it may or may not work properly on
>i386 or ARM.  Part of getting this package into Fedora is to provide for such
>additional testing and/or porting as needed.

About this, http://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support

Comment 2 Antonio T. (sagitter) 2013-01-27 22:27:09 UTC
Just few other comments.

>NB: the intent would be for this package to supercede the iasl package which
>(a) only ships the iasl tool mentioned above from the same upstream source,
>and (b) is a little bit dated.  This would be the more complete version of
>the ACPICA tools.

- obsolete tag is needed: http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.2FReplacing_Existing_Packages

- I don't see any License file in the source archive.
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#License_Text

- Where does tar.gz source come from ?
Because in https://www.acpica.org/downloads/ I see the 20130117 whereas the 20130123 seems available in https://github.com/otcshare/acpica/.

In this last case, you should look around  https://fedoraproject.org/wiki/Packaging:NamingGuidelines?rd=Packaging/NamingGuidelines#Snapshot_packages

Comment 3 Michael Schwendt 2013-01-28 09:35:06 UTC
> http://fedorapeople.org/~ahs3/acpica-tools-20130123-1.f18.src.rpm

404 not found. f18 => fc18! ;)


> obsolete tag is needed

It's there (below the BuildRequires in the spec file), but it's too low for the latest Fedora package:

> Obsoletes:      iasl <= 20120913-6

Compare with: yum list iasl

Also, since it includes %{_bindir}/iasl and shall replace package "iasl", a versioned "Provides" for iasl ought to be added.


> further, the pmtools package -- which provides acpidump -- also provides
> a /usr/sbin/acpixtract that we don't really want to collide with

You do collide currently, however, because both builds of acpixtract are in $PATH. For normal users:

  $ rpm -qf $(which acpixtract)
  file /usr/bin/acpixtract is not owned by any package
  $ file $(which acpixtract)
  /usr/bin/acpixtract: symbolic link to `/etc/alternatives/acpixtract'

For root:

  # rpm -qf $(which acpixtract)
  pmtools-20100513-3.fc18.x86_64

/sbin is before /usr/bin in $PATH for root, and vice versa for ordinary users.


> NB: this package does not use the %{optflags} macro
> Rationale: upstream claims that using -O will lead to miscompilation
>            and the resulting tools will be incorrect.  Since ACPI is
>            a reasonably critical part of the environment, we are erring
>            on the side of caution.  Furthermore, it is not important that
>            these tools operate more quickly than they do.  Their present
>            performance level is sufficient.

A few thoughts here:

1) Since I haven't checked whether the source code uses plain C only or also machine/assembler language, does the claim of miscompilation refer to the C source files? Does the test-suite discover the miscompilation?

Given the fact that many thousands of packages are built with Fedora's optflags, miscompilation for this particular software could be due to a questionable programming style (such as dubious/unsafe assumptions about memory layout and e.g. casts). It could be enlightening to track down where it breaks, especially if this code is supposed to be a reference implementation.

2) Has this ever been reported to the GNU compiler developers? Or even Red Hat's compiler maintainers?

3) %optflags are not just for performance. It's also security related and helps locating bugs, too:

  $ rpm --eval %optflags
  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic

Could they be used with -O2 filtered out?
https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags


> %{_mandir}/man1/iasl.1.gz

%{_mandir}/man1/iasl.1*  is the more flexible wildcard for including manual pages, as it allows for the compression method to be disabled/reconfigured.


> - NB: ACPICA documentation is not clearly redistributable so not included

Apparently, this %changelog comment doesn't refer to files included within the src.rpm, does it? I find the comment confusing and ask about it, because if the src.rpm included the docs, they would need to be deleted from it, too.

Comment 4 Antonio T. (sagitter) 2013-01-28 15:49:09 UTC
>> obsolete tag is needed
>
>It's there (below the BuildRequires in the spec file), but it's too low for the >latest Fedora package:

Ops ! I'm sorry, guys. I had not see it. :(

Comment 5 Al Stone (Old account - use ahs3@redhat.com) 2013-01-28 19:00:44 UTC
(In reply to comment #1)
> Hi Al.
> 
> >NB: this package does work on x86_64; it may or may not work properly on
> >i386 or ARM.  Part of getting this package into Fedora is to provide for such
> >additional testing and/or porting as needed.
> 
> About this,
> http://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support

I misspoke :(.  And I had a silly bug in the testing script that I had not
seen.

x86 and x86_64 work: https://koji.fedoraproject.org/koji/taskinfo?taskID=4906710

ARM works: https://arm.koji.fedoraproject.org/koji/taskinfo?taskID=1401630

PPC, sparc and s390 should also build properly (I am currently correcting the
build errors).

At the same time, I had read that part of the guidelines, but it was unclear
to me how that applied to brand new packages.  It seemed sort of odd to file
bugs for something that does not build when the package wasn't in the archive
yet.  I'll post a -2 version once I verify the PPC, sparc and s390 fixes.

Comment 6 Al Stone (Old account - use ahs3@redhat.com) 2013-01-28 21:04:06 UTC
(In reply to comment #5)
> (In reply to comment #1)
> > Hi Al.
> > 
> > >NB: this package does work on x86_64; it may or may not work properly on
> > >i386 or ARM.  Part of getting this package into Fedora is to provide for such
> > >additional testing and/or porting as needed.
> > 
> > About this,
> > http://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support
> 
> I misspoke :(.  And I had a silly bug in the testing script that I had not
> seen.
> 
> x86 and x86_64 work:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=4906710
> 
> ARM works: https://arm.koji.fedoraproject.org/koji/taskinfo?taskID=1401630
> 
> PPC, sparc and s390 should also build properly (I am currently correcting the
> build errors).
> 
> At the same time, I had read that part of the guidelines, but it was unclear
> to me how that applied to brand new packages.  It seemed sort of odd to file
> bugs for something that does not build when the package wasn't in the archive
> yet.  I'll post a -2 version once I verify the PPC, sparc and s390 fixes.

Further investigation on my part has me convinced that ExcludeArch is still
the proper way to go.  Neither PPC, Sparc or s390 (of any flavor) provide
or support ACPI in any meaningful way.  It's just simply not the way these
systems work on boot.  Therefore, providing support for them in this package
is essentially meaningless; you wouldn't be able to use any of the results
produced.

The package has been modified accordingly (with an explanation added) and will
be in the -2 version to be posted later today.

Comment 7 Al Stone (Old account - use ahs3@redhat.com) 2013-01-28 21:28:05 UTC
(In reply to comment #2)
> Just few other comments.
> 
> >NB: the intent would be for this package to supercede the iasl package which
> >(a) only ships the iasl tool mentioned above from the same upstream source,
> >and (b) is a little bit dated.  This would be the more complete version of
> >the ACPICA tools.
> 
> - obsolete tag is needed:
> http://fedoraproject.org/wiki/Packaging:Guidelines#Renaming.
> 2FReplacing_Existing_Packages

As noted elsewhere, there is an Obsoletes tag.  Added a Provides tag to
help clarify, too.

> - I don't see any License file in the source archive.
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/
> LicensingGuidelines#License_Text
 
Added.  Thanks for catching that!

> - Where does tar.gz source come from ?
> Because in https://www.acpica.org/downloads/ I see the 20130117 whereas the
> 20130123 seems available in https://github.com/otcshare/acpica/.
> 
> In this last case, you should look around 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines?rd=Packaging/
> NamingGuidelines#Snapshot_packages

This is indeed a snapshot, so the version is now 20130123git, as I believe
the policy is indicating it should be.  Note that this is the only versioning
that upstream uses on the code, unfortunately, so a version such as
"1.0.20130123git" would not make a great deal of sense :(.

Comment 8 Al Stone (Old account - use ahs3@redhat.com) 2013-01-28 22:00:05 UTC
(In reply to comment #3)
> > http://fedorapeople.org/~ahs3/acpica-tools-20130123-1.f18.src.rpm
> 
> 404 not found. f18 => fc18! ;)
 
D'oh.  Sorry about that.  I type fast enough I sometimes forget that a
quick cut'n'paste might be more accurate :).

> > obsolete tag is needed
> 
> It's there (below the BuildRequires in the spec file), but it's too low for
> the latest Fedora package:
> 
> > Obsoletes:      iasl <= 20120913-6
> 
> Compare with: yum list iasl
> 
> Also, since it includes %{_bindir}/iasl and shall replace package "iasl", a
> versioned "Provides" for iasl ought to be added.

I've put in a versioned provide for iasl now.

Could you please explain the "too low" part?  I'm puzzled (as usual :).

On f18, if I do "yum list iasl", I get 20100528-6.  If I look at the
latest rawhide source, the spec file indicates 20120913-6.  If the idea
is to replace everything rawhide and earlier, what should the version be?

> > further, the pmtools package -- which provides acpidump -- also provides
> > a /usr/sbin/acpixtract that we don't really want to collide with
> 
> You do collide currently, however, because both builds of acpixtract are in
> $PATH. For normal users:
> 
>   $ rpm -qf $(which acpixtract)
>   file /usr/bin/acpixtract is not owned by any package
>   $ file $(which acpixtract)
>   /usr/bin/acpixtract: symbolic link to `/etc/alternatives/acpixtract'
> 
> For root:
> 
>   # rpm -qf $(which acpixtract)
>   pmtools-20100513-3.fc18.x86_64
> 
> /sbin is before /usr/bin in $PATH for root, and vice versa for ordinary
> users.

Right.  This is why I've added the use of alternatives to the package.

At the same time, what I will need to do is provide a patch to the pmtools
package so that they also use alternatives (and I would recommend moving
the binaries to /usr/bin -- I don't think /usr/sbin is really necessary
for acpixtract, at least).  My intent was to provide that patch after this
package had passed review but before it got checked into git.

> 
> > NB: this package does not use the %{optflags} macro
> > Rationale: upstream claims that using -O will lead to miscompilation
> >            and the resulting tools will be incorrect.  Since ACPI is
> >            a reasonably critical part of the environment, we are erring
> >            on the side of caution.  Furthermore, it is not important that
> >            these tools operate more quickly than they do.  Their present
> >            performance level is sufficient.
> 
> A few thoughts here:
> 
> 1) Since I haven't checked whether the source code uses plain C only or also
> machine/assembler language, does the claim of miscompilation refer to the C
> source files? Does the test-suite discover the miscompilation?

There is only C source involved here.  I do not know if the test suite
will discover the miscompilation; I'll have to have further discussion
with upstream to determine that.

> Given the fact that many thousands of packages are built with Fedora's
> optflags, miscompilation for this particular software could be due to a
> questionable programming style (such as dubious/unsafe assumptions about
> memory layout and e.g. casts). It could be enlightening to track down where
> it breaks, especially if this code is supposed to be a reference
> implementation.

Agreed.  I think what I would like to do is file a bug against this package
eventually and hunt this down.  My own suspicion is that the issue is more
likely due to the source having to support MSVC and GCC (plus a few others)
that may cause the underlying problem.

> 2) Has this ever been reported to the GNU compiler developers? Or even Red
> Hat's compiler maintainers?
 
Not that I know of; it's a long-standing claim that pre-dates my use of
or involvement in ACPI, I'm afraid.  I will try to determine if it has,
however, and report on the specific problem once I uncover it.

> 3) %optflags are not just for performance. It's also security related and
> helps locating bugs, too:
> 
>   $ rpm --eval %optflags
>   -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector
> --param=ssp-buffer-size=4  -m64 -mtune=generic
> 
> Could they be used with -O2 filtered out?
> https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
 
Understood.  I would prefer to use %{optflags}.  There is a patch
called acpica-tools-config.patch that sets the values that these tools
will tolerate with GCC; unfortunately, this even includes turning off
FORTIFY_SOURCE.  All I can offer at this point is to work with upstream
over time to correct this.  I don't really know if they have been slow
to change, or previous users of this source package have not pushed the
issue with them.

> > %{_mandir}/man1/iasl.1.gz
> 
> %{_mandir}/man1/iasl.1*  is the more flexible wildcard for including manual
> pages, as it allows for the compression method to be disabled/reconfigured.

Good to know.  Thanks.  Fixed in the updated spec file.

> > - NB: ACPICA documentation is not clearly redistributable so not included
> 
> Apparently, this %changelog comment doesn't refer to files included within
> the src.rpm, does it? I find the comment confusing and ask about it, because
> if the src.rpm included the docs, they would need to be deleted from it, too.

Hrm.  I'll try to reword that.  No, the documentation being referred to is
*not* in the srpm.  My intent with the note was to indicate that if you use
git from upstream, you will get the documents, but that you will not find them
in this copy of the source.

Comment 9 Al Stone (Old account - use ahs3@redhat.com) 2013-01-28 22:22:50 UTC
Updated versions now available.

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130123git-2.fc18.src.rpm

Thanks for everyone's help so far!

Comment 10 Michael Schwendt 2013-01-29 10:57:31 UTC
> On f18, if I do "yum list iasl", I get 20100528-6. 

> Could you please explain the "too low" part?

Sure. You somehow miss the %{?dist} macro at the end of the Release tag. The value the macro expands to is not ignored during RPM version comparison:

  # rpm --eval %dist
  .fc19

  # yum list iasl|grep ^iasl
  iasl.x86_64                       20120913-6.fc19                        rawhide

  # rpmdev-vercmp 20120913-6.fc19 20120913-6
  20120913-6.fc19 > 20120913-6

> what should the version be?

Either "<=" the latest EVR from Rawhide or "<" the next higher Release value, i.e. either

  Obsoletes:      iasl <= 20120913-6.fc19
or
  Obsoletes:      iasl < 20120913-7

[...]

> Provides:       iasl > 20120913-6

Very unusual. Rather:
Provides: iasl = %{version}-%{release}

[...]

> I will need to do is provide a patch to the pmtools
> package so that they also use alternatives

Ah, that makes sense, of course.


> This is indeed a snapshot, so the version is now 20130123git,

For a snapshot, you would need to adhere to the following guideline

  https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

| There are several cases where upstream is not providing the source
| to you in an upstream tarball. In these cases you must document how
| to generate the tarball used in the rpm either through a spec file
| comment or a script included as a separate SourceX:. 

because the URL you've constructed results in "404 not found", as well as the Packaging Naming Guidelines for snapshot packages (which Antonio has pointed at):

  Version: 20130117

Which is the last official release of the source tarball, because typically one doesn't "make up" own Version numbers, even if it may be possible to predict the next version. And

  Release: 1.20130123git%{?dist}

to apply Fedora's naming guidelines for post-release snapshot packages:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Post-Release_packages

Alternatively, if you would insist on predicting the next officially released version, you could apply the pre-release snapshot naming guidelines (which isn't prettier however because for newer snapshots you would change also the Version tag, not just the Release):

  Version: 20130123
  Release: 0.1.20130123git%{?dist}

There, for updates of the package, you would bump the  "0." and/or the snapshot date. For a future officially released tarball, you would update Version and reset Release back to "1%{?dist}".

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages
|
| If the snapshot package is considered a "pre-release package",
| you should follow the guidelines listed in Pre-Release Packages
| for snapshot packages,

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

Comment 11 Michael Schwendt 2013-01-29 11:00:25 UTC
Cut'n'paste error in my previous comment:

> There, for updates of the package, you would bump the  "0." and/or ...

... bump the value at the right of "0." and/or ...

Comment 12 Al Stone (Old account - use ahs3@redhat.com) 2013-01-30 21:10:18 UTC
(In reply to comment #10)
> > On f18, if I do "yum list iasl", I get 20100528-6. 
> 
> > Could you please explain the "too low" part?
> 
> Sure. You somehow miss the %{?dist} macro at the end of the Release tag. The
> value the macro expands to is not ignored during RPM version comparison:
> 
>   # rpm --eval %dist
>   .fc19
> 
>   # yum list iasl|grep ^iasl
>   iasl.x86_64                       20120913-6.fc19                       
> rawhide
> 
>   # rpmdev-vercmp 20120913-6.fc19 20120913-6
>   20120913-6.fc19 > 20120913-6
 
Whups.  Thanks.  I was missing the obvious part (the use of %dist).

> > what should the version be?
> 
> Either "<=" the latest EVR from Rawhide or "<" the next higher Release
> value, i.e. either
> 
>   Obsoletes:      iasl <= 20120913-6.fc19
> or
>   Obsoletes:      iasl < 20120913-7
> 
> [...]
> 
> > Provides:       iasl > 20120913-6
> 
> Very unusual. Rather:
> Provides: iasl = %{version}-%{release}

Aha.  Right.  I didn't think that through properly.  Many thanks for the explanation.

> [...]
> 
> > I will need to do is provide a patch to the pmtools
> > package so that they also use alternatives
> 
> Ah, that makes sense, of course.

Cool.  Thanks.
 
> 
> > This is indeed a snapshot, so the version is now 20130123git,
> 
> For a snapshot, you would need to adhere to the following guideline
> 
>   https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control
> 
> | There are several cases where upstream is not providing the source
> | to you in an upstream tarball. In these cases you must document how
> | to generate the tarball used in the rpm either through a spec file
> | comment or a script included as a separate SourceX:. 
> 
> because the URL you've constructed results in "404 not found", as well as
> the Packaging Naming Guidelines for snapshot packages (which Antonio has
> pointed at):
> 
>   Version: 20130117
 
Aha.  I'm used to Debian packaging which is a little more
relaxed in this regard.

> Which is the last official release of the source tarball, because typically
> one doesn't "make up" own Version numbers, even if it may be possible to
> predict the next version. And
> 
>   Release: 1.20130123git%{?dist}
> 
> to apply Fedora's naming guidelines for post-release snapshot packages:
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Post-
> Release_packages
> 
> Alternatively, if you would insist on predicting the next officially
> released version, you could apply the pre-release snapshot naming guidelines
> (which isn't prettier however because for newer snapshots you would change
> also the Version tag, not just the Release):
> 
>   Version: 20130123
>   Release: 0.1.20130123git%{?dist}
> 
> There, for updates of the package, you would bump the  "0." and/or the
> snapshot date. For a future officially released tarball, you would update
> Version and reset Release back to "1%{?dist}".
> 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages
> |
> | If the snapshot package is considered a "pre-release package",
> | you should follow the guidelines listed in Pre-Release Packages
> | for snapshot packages,
> 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-
> Release_packages

Hrm.  Stepping back a second and rethinking the versioning, it is not
critical to have the latest version from git (nice, but not critical).
I think it would better to drop this and go to something that might
convey more useful info to the user; the upstream git doesn't really
change all that often, either (usually just at release time anyway).

What upstream does is release a version supporting a specific release of
the ACPI specification (5.0, in this case).  Then, they periodically
update the implementation (changing the date in the tarball name).  So,
my first thought was to do something like this:

   Version: 5.0
   Release: 1.20130117%{?dist}

and increment the 1. at the beginning of Release for each packaging change
and/or bug fix required, similar to what was suggested above.  If I need to
pull something from git, do it as a patch and carry the patch until the next
official release tarball.  At the next tarball, reset the release back to
1.<date>.

Unfortunately...

   $ rpmdev-vercmp 5.0-7.20130117.fc18 5.0-1.20130217.fc18
   5.0-7.20130117.fc18 > 5.0-1.20130217.fc18

However, this approach compares properly:

   Version: 5.0
   Release: 20130117.1%{?dist}

   $ rpmdev-vercmp 5.0-20130117.7.fc18 5.0-20130217.1.fc18
   5.0-20130117.7.fc18 < 5.0-20130217.1.fc18

When the ACPI 6.0 specification comes out, 5.0 -> 6.0.  When a new official
tarball comes out, 20130117 -> the new date.  If a packaging fix or a new
patch is needed, .1 -> .2, and so on.

I'll prepare rpm's with this scheme and post them.  Let me know what you think.
My sense is that this approach is just a lot more straightforward and I was
making things more complicated than need be without gaining anything by it.

Comment 13 Al Stone (Old account - use ahs3@redhat.com) 2013-01-31 00:49:21 UTC
Updated versions now available.

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-5.0-20130117.1.fc18.src.rpm

Thanks again for all the suggestions.

Comment 14 Michael Schwendt 2013-01-31 12:21:38 UTC
Oh, discussing Debian packaging practices is beyond the scope of a Fedora package review request. ;)

I would also favour "more relaxed" Fedora packaging guidelines in a few areas, but with more freedom comes more responsibility, and simply adhering to package versioning guidelines makes it easier to avoid common pitfalls (such as violated dist-upgrade paths, desire to downgrade something, Epoch madness) and reinventing the wheel.

[...]

> Version: 5.0

You may consider the ACPI specification version relevant, but it is not being used for the versioning scheme of the acpica-unix archive. Where's the benefit?

acixf.h
#define ACPI_CA_VERSION                 0x20130117

changes.txt
17 January 2013. Summary of changes for version 20130117:

https://www.acpica.org/downloads/linux.php
The current release of ACPICA is version 20130117.

The tarball: acpica-unix-20130117.tar.gz

http://rpmfind.net/linux/rpm2html/search.php?query=acpica
https://bugs.launchpad.net/ubuntu/+source/acpica-unix/+bug/1060791

The Obsoletes/Provides pair now would be self-obsoleting

  Provides:       iasl = %{version}-%{release}
  Obsoletes:      iasl < 20120913-7

and would advertise a changed versioning scheme for iasl, too.


> Release: 20130117.1%{?dist}

So, I see you've returned to the official tarball release.

You invent your own versioning scheme, with the real upstream version being part of the Release tag.

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Version_Tag
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag

It's also not as flexible as you might think. Imagine the need to release an update for an older dist. For ordinary package changes, you could only bump the Release tag at the very right side,

   Release: 20130117.1%{?dist}
 ->
   Release: 20130117.1%{?dist}.1

or else a package for Fedora 17 could become newer than a package for Fedora 18.
That's described here: https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Minor_release_bumps_for_old_branches

Even more so, if you ever wanted to use a new snapshot for an older dist only. With the date value at the most-significant position of the Release tag, you could not increase it without doing that also for all newer dists.

rpmdev-bumpspec would need to be patched/enhanced, too, to understand this versioning scheme. For mass-rebuilds of Fedora packages, it would apply only the "minor release bump" to this package. Not pretty.

Comment 15 Al Stone (Old account - use ahs3@redhat.com) 2013-01-31 16:27:33 UTC
(In reply to comment #14)
> Oh, discussing Debian packaging practices is beyond the scope of a Fedora
> package review request. ;)
 
Agreed :-).

> [...]
> 
> > Version: 5.0
> 
> You may consider the ACPI specification version relevant, but it is not
> being used for the versioning scheme of the acpica-unix archive. Where's the
> benefit?

My thinking was it would aid the user.  However, given that the ACPI
spec has never been in the versioning _before_, I can concede on that
point.  It doesn't really add that much.

> acixf.h
> #define ACPI_CA_VERSION                 0x20130117
> 
> changes.txt
> 17 January 2013. Summary of changes for version 20130117:
> 
> https://www.acpica.org/downloads/linux.php
> The current release of ACPICA is version 20130117.
> 
> The tarball: acpica-unix-20130117.tar.gz
> 
> http://rpmfind.net/linux/rpm2html/search.php?query=acpica
> https://bugs.launchpad.net/ubuntu/+source/acpica-unix/+bug/1060791
 
Hrm.  Fair enough.  It would make it more difficult to find things
across distros; I had not considered that part of it.

> The Obsoletes/Provides pair now would be self-obsoleting
> 
>   Provides:       iasl = %{version}-%{release}
>   Obsoletes:      iasl < 20120913-7
> 
> and would advertise a changed versioning scheme for iasl, too.
> 
> 
> > Release: 20130117.1%{?dist}
> 
> So, I see you've returned to the official tarball release.
 
Yup.  Examining the changes in git vs the official tarball in more detail,
they were not sufficient to justify going to a snapshot type of package.
They were useful patches, but "nice to have" not "essential to working
properly."  The benefit was far outweighed by the effort for the snapshot
scheme.

> You invent your own versioning scheme, with the real upstream version being
> part of the Release tag.
> 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Version_Tag
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag
> [...]

Yes, I did, with the thought that it might be more useful (I seek to
continuously improve things whenever I can...).  In this case, though, the
counter-arguments so far have had more weight -- and more practicality that
I had not seen before.  I've learned a great deal :).

So let's just use the original straightforward scheme.  I can work with
that.  That would be:

   Version: 20130117
   Release: 1%{?dist}

which should handle the Obsoletes/Provides, as well.

Comment 16 Al Stone (Old account - use ahs3@redhat.com) 2013-01-31 23:31:57 UTC
Updated versions now available.

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130117-5.fc18.src.rpm

Thanks again for all the help.

Comment 17 Al Stone (Old account - use ahs3@redhat.com) 2013-02-19 02:30:23 UTC
Upstream has now provided a newer version.  Packages have been rebuilt and are ready to push into SCM most any time.

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130214-1.fc18.src.rpm

Koji builds:

-- x86/x86_64:

   http://koji.fedoraproject.org/koji/taskinfo?taskID=5028666

-- ARM:

   http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=1449293

You may retrieve the packages from a yum repository, if you wish; use the following config:

[experimental]
name=experimental
baseurl=http://repos.fedorapeople.org/repos/ahs3/experimental/fedora-18/x86_64/
        http://repos.fedorapeople.org/repos/ahs3/experimental/fedora-18/noarch/
        http://repos.fedorapeople.org/repos/ahs3/experimental/fedora-18/SRPMS/
enabled=1
gpgcheck=1

Do I have approval to go to the next step in getting this package into Fedora?

Thanks for the time and effort.

Comment 18 Chris Tyler 2013-03-05 08:41:09 UTC
Non-sponsor review:

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

Key:
[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.
[!]: %build honors applicable compiler flags or justifies otherwise.

     Note: Package comments explain why optimizations are disabled, 
     but the other compiler option flags should be honoured 
     (via %{optflags}) such as -g

[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.

     Note: rm -rf %{buildroot} present but not required

[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4

     Note: %defattr present but not needed

[-]: Package contains desktop file if it is a GUI application.
[!]: Development files must be in a -devel package

     Note: should be automatically produced if -g option is used
     during make

[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[!]: Package is not known to require ExcludeArch.

     Note: ExcludeArch should not be included (these tools can
     be used to cross-prepare ACPI tables even on systems that
     cannot use them)

[x]: Package complies to the 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 %doc.
[x]: License field in the package spec file matches the actual license.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[!]: Package does not generate any conflict.

     Note: Package should obsolete pmtools - alternatives mechanism will
     only work if the other package is alternatives-aware, and it appears
     to contain an older version of the same tools. Coordinate the 
     retirement of the obsoleted packages with the other package 
     maintainers.

[x]: Package obeys FHS, except libexecdir and /usr/target.
[!]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.

     See note above.

[x]: Package must own all directories that it creates.
[!]: Package does not own files or directories owned by other packages.

     See note on pmtools conflict above.

[x]: Requires correct, justified where necessary.
[!]: Only use %_sourcedir in very specific situations.

     Note: %_sourcedir is used. Use %{SOURCE1} to refer to the Source1
     file instead.

[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.

     See notes on optflags above.

[-]: Large documentation must go in a -doc subpackage.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[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]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[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:
[!]: Uses parallel make.

     Note: Add %{?_smp_mflags} to the make to enable this. 

[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)

     Note: %clean present but not required

[!]: 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.
[!]: Patches link to upstream bugs/comments/lists or are otherwise justified.

     Note: Making "alike as possible" to Debian doesn't really explain
     why the patches are justified.

[-]: 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.
[x]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[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]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[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: acpica-tools-20130214-1.fc17.x86_64.rpm
acpica-tools.x86_64: W: spelling-error %description -l en_US acpidump -> dumpiness
acpica-tools.x86_64: W: spelling-error %description -l en_US pmtools -> pm tools, pm-tools, tools
1 packages and 0 specfiles checked; 0 errors, 2 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint acpica-tools
acpica-tools.x86_64: W: spelling-error %description -l en_US acpidump -> dumpiness
acpica-tools.x86_64: W: spelling-error %description -l en_US pmtools -> pm tools, pm-tools, tools
1 packages and 0 specfiles checked; 0 errors, 2 warnings.


===== Additional comments:

- Recommend the use of globs in %files section to (a) reduce spec file size and (b) reduce the number of edits required as upstream changes (e.g., adds or deletes binaries):

%{_bindir}/*
%{_mandir}/*/*

The install section could also use globs to reduce editing for future releases: 

install -pD generate/unix/bin/*/* %{buildroot}%{_bindir}/
install -pDm 0644 *.1 %{buildroot}%{_mandir}/man1/

- Avoid macro references in changelog (%check)

- README.Fedora notes on git clone steps are apparently obsolete

Comment 19 Michael Schwendt 2013-03-07 12:44:53 UTC
> the other compiler option flags should be honoured 
>      (via %{optflags}) such as -g

Agreed. I had asked about filtering out -O2, since not all of %optflags
is about optimizing. Some help with locating bugs.


> [!]: Development files must be in a -devel package
> 
>      Note: should be automatically produced if -g option is used
>      during make

Could you elaborate? Perhaps you refer to the -debuginfo package?


> ExcludeArch should not be included ...

Either that or (as linked before) there must be tickets that
explain why ExcludeArch is used:
http://fedoraproject.org/wiki/Packaging:Guidelines#Architecture_Support


> Package should obsolete pmtools - alternatives mechanism will
> only work if the other package is alternatives-aware

Since the plan was to add alternatives to pmtools, a ticket could be
opened for that, blocking this review.

Comment 20 Chris Tyler 2013-03-19 22:40:25 UTC
>> [!]: Development files must be in a -devel package
>> 
>>      Note: should be automatically produced if -g option is used
>>      during make
>
>Could you elaborate? Perhaps you refer to the -debuginfo package?

Yes, I meant -debuginfo ... reading too fast  :-(



>> Package should obsolete pmtools - alternatives mechanism will
>> only work if the other package is alternatives-aware
>
>Since the plan was to add alternatives to pmtools, a ticket could be
>opened for that, blocking this review.

But pmtools isn't really an alternative -- it's just an old version of the same code (also from Intel).

The page at the URL from the pmtools package (http://www.lesswatts.org/projects/acpi/utilities.php) says "The latest ACPICA package, including iASL compiler is here: http://www.acpica.org/downloads/", which is where this package comes from.

The lesswatts initiative dates back to ~2008 and hasn't seen a lot of love since (the site shows 2.6.26 (July 2008) as the last kernel in the graphs).

(Come to think of it, I have a flourescent-green lesswatts.org shirt from OLS 2008...)

Comment 21 Al Stone (Old account - use ahs3@redhat.com) 2013-03-22 00:09:37 UTC
(In reply to comment #18)
> Non-sponsor review:
> 
> Package Review
> ==============
> 
> Key:
> [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.
> [!]: %build honors applicable compiler flags or justifies otherwise.
> 
>      Note: Package comments explain why optimizations are disabled, 
>      but the other compiler option flags should be honoured 
>      (via %{optflags}) such as -g

Use of %{optflags} now incorporated.  It appears to have no affect on the test
results and upstream does not appear to be as adamant as they used to be about
not using -O with this latest version.
 
> [x]: Package contains no bundled libraries without FPC exception.
> [x]: Changelog in prescribed format.
> [!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> 
>      Note: rm -rf %{buildroot} present but not required

Removed.
 
> [x]: Sources contain only permissible code or content.
> [x]: Each %files section contains %defattr if rpm < 4.4
> 
>      Note: %defattr present but not needed

Removed.  No sense in carrying around something not needed.

> [-]: Package contains desktop file if it is a GUI application.
> [!]: Development files must be in a -devel package
> 
>      Note: should be automatically produced if -g option is used
>      during make

Is this applicable? -debuginfo packages are automatically
produced, but this package contains no libraries that would
need header files and the like.

> [x]: Package requires other packages for directories it uses.
> [x]: Package uses nothing in %doc for runtime.
> [!]: Package is not known to require ExcludeArch.
> 
>      Note: ExcludeArch should not be included (these tools can
>      be used to cross-prepare ACPI tables even on systems that
>      cannot use them)

Removed.  PPC and s390 do not build from source; as secondary architectures, this is not critical, but I would like to resolve it soon.

> [x]: Package complies to the 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 %doc.
> [x]: License field in the package spec file matches the actual license.
> [x]: Package consistently uses macros (instead of hard-coded directory
>      names).
> [x]: Package is named according to the Package Naming Guidelines.
> [!]: Package does not generate any conflict.
> 
>      Note: Package should obsolete pmtools - alternatives mechanism will
>      only work if the other package is alternatives-aware, and it appears
>      to contain an older version of the same tools. Coordinate the 
>      retirement of the obsoleted packages with the other package 
>      maintainers.

Obsoleting pmtools would remove the acpidump tool from Fedora; I actually
use it, and perhaps others do, too.  What I've done is provide a patch to
the pmtools package and filed a bug asking for pmtools to use the alternatives
mechanism for acpixtract (the only command in common between the two packages);
please see BZ#924442 for details.  I've also made that a blocker for this bug.
 
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [!]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> 
>      See note above.

See note above :).

> [x]: Package must own all directories that it creates.
> [!]: Package does not own files or directories owned by other packages.
> 
>      See note on pmtools conflict above.

See note above :).

> [x]: Requires correct, justified where necessary.
> [!]: Only use %_sourcedir in very specific situations.
> 
>      Note: %_sourcedir is used. Use %{SOURCE1} to refer to the Source1
>      file instead.

Fixed.
 
> [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.
> 
>      See notes on optflags above.

With use of %optflags, this should now be corrected.

> [-]: Large documentation must go in a -doc subpackage.
> [x]: All build dependencies are listed in BuildRequires, except for any that
>      are listed in the exceptions section of Packaging Guidelines.
> [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]: Spec file lacks Packager, Vendor, PreReq tags.
> [x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
>      work.
> [x]: Package is named using only allowed ASCII characters.
> [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:
> [!]: Uses parallel make.
> 
>      Note: Add %{?_smp_mflags} to the make to enable this.

Added.

> 
> [!]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> 
>      Note: %clean present but not required

Removed %clean section completely.

> [!]: If the source package does not include license text(s) as a separate
> file
>      from upstream, the packager SHOULD query upstream to include it.

The source is being licensed under the GPLv2 (and upstream is pretty
religious about including the right text in all of the source files).

Is it necessary to include a copy of GPLv2?  I can, but it seems redundant.

> [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.
> [!]: Patches link to upstream bugs/comments/lists or are otherwise justified.
> 
>      Note: Making "alike as possible" to Debian doesn't really explain
>      why the patches are justified.

Ah.  True.  The patches are from the original iasl package and still solve
some existing problems for big-endian builds and for systems that do not
like unaligned accesses.  I mistakenly removed the commentary from the
front of the patches; that has been restored plus some of the additional
commentary from the iasl changelog that explained which BZ#s were being
fixed.

NB: in the process of checking out other arch builds for this reply, I had
to create another patch (name-miscompare.patch) to correct an upstream bug
I uncovered by running the misc tests (incorrect byte order on big-endian
machines).

> [-]: 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.
> [x]: %check is present and all tests pass.
> [x]: Packages should try to preserve timestamps of original installed files.
> [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]: Dist tag is present.
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [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: acpica-tools-20130214-1.fc17.x86_64.rpm
> acpica-tools.x86_64: W: spelling-error %description -l en_US acpidump ->
> dumpiness
> acpica-tools.x86_64: W: spelling-error %description -l en_US pmtools -> pm
> tools, pm-tools, tools
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> 
> 
> 
> Rpmlint (installed packages)
> ----------------------------
> # rpmlint acpica-tools
> acpica-tools.x86_64: W: spelling-error %description -l en_US acpidump ->
> dumpiness
> acpica-tools.x86_64: W: spelling-error %description -l en_US pmtools -> pm
> tools, pm-tools, tools
> 1 packages and 0 specfiles checked; 0 errors, 2 warnings.
> 
> 
> ===== Additional comments:
> 
> - Recommend the use of globs in %files section to (a) reduce spec file size
> and (b) reduce the number of edits required as upstream changes (e.g., adds
> or deletes binaries):
> 
> %{_bindir}/*
> %{_mandir}/*/*

Done.

> The install section could also use globs to reduce editing for future
> releases: 
> 
> install -pD generate/unix/bin/*/* %{buildroot}%{_bindir}/
> install -pDm 0644 *.1 %{buildroot}%{_mandir}/man1/

Done.

> - Avoid macro references in changelog (%check)

Whups.  Fixed.
 
> - README.Fedora notes on git clone steps are apparently obsolete

Removed.

Comment 22 Al Stone (Old account - use ahs3@redhat.com) 2013-03-22 00:29:29 UTC
Updated versions now available.

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130214-2.fc18.src.rpm

Thanks again for all the help.

Comment 23 Al Stone (Old account - use ahs3@redhat.com) 2013-03-22 00:36:23 UTC
Successful koji scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5155146

Comment 24 Peter Robinson 2013-04-11 20:47:40 UTC
I'll sponsor Al so removing blocker

Comment 25 Peter Robinson 2013-04-12 09:07:25 UTC
> Further investigation on my part has me convinced that ExcludeArch is still
> the proper way to go.  Neither PPC, Sparc or s390 (of any flavor) provide
> or support ACPI in any meaningful way.  It's just simply not the way these
> systems work on boot.  Therefore, providing support for them in this package
> is essentially meaningless; you wouldn't be able to use any of the results
> produced.

The following is probably the best way to handle it  because if you do ExcludeArch it won't take into account other bringups (Tile/MIPS/jcm) that people way wish to do in the future.

ExclusiveArch: %{ix86} x86_64 %{arm}

Comment 26 Al Stone (Old account - use ahs3@redhat.com) 2013-04-15 22:08:19 UTC
Re comment #25 on Exclude/Exclusive Arch: the package definitely works on x86* platforms and on ARM; the others are secondaries and will likely need help from the maintainers there should they want this package at all.  Hence, I'm leaving out any exclusions.

A new upstream version became available.  Successful koji scratch build may be found here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5256169

Updated versions can also be found here:

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130328-1.fc18.src.rpm

Comment 27 Al Stone (Old account - use ahs3@redhat.com) 2013-04-16 21:18:21 UTC
Successful f19 build:

https://koji.fedoraproject.org/koji/taskinfo?taskID=5262099

Comment 28 Al Stone (Old account - use ahs3@redhat.com) 2013-05-29 03:04:30 UTC
New upstream version; updated package is here:

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130517-1.fc18.src.rpm

Successful koji scratch build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5436035

Comment 29 Mario Blättermann 2013-06-08 14:03:17 UTC
Please don't set the fedora-review? flag for your own package. This has to be set by a reviewer.

Comment 30 Al Stone (Old account - use ahs3@redhat.com) 2013-06-08 16:57:42 UTC
(In reply to Mario Blättermann from comment #29)
> Please don't set the fedora-review? flag for your own package. This has to
> be set by a reviewer.

Ah, my apologies.  Thank you for removing it; I had inadvertently done so.

Any idea when someone might pick this up so that I can get it into Fedora?  I have now received sponsorship as a packager, it has been in the queue for four months and had extensive review, and builds for both f18 and f19.  Not sure what's left to do, at this point, other than just wait....

Comment 31 Peter Robinson 2013-06-08 17:20:00 UTC
I'll take it and try and get it done this week

Comment 32 Al Stone (Old account - use ahs3@redhat.com) 2013-06-10 13:48:22 UTC
Many thanks, Peter.  Let me know if you find anything and I'll jump on it ASAP.

Comment 33 Account closed by the user 2013-07-17 13:21:41 UTC
(In reply to Al Stone from comment #21)
> (In reply to comment #18)

> >      Note: Package should obsolete pmtools - alternatives mechanism will
> >      only work if the other package is alternatives-aware, and it appears
> >      to contain an older version of the same tools. Coordinate the 
> >      retirement of the obsoleted packages with the other package 
> >      maintainers.
> 
> Obsoleting pmtools would remove the acpidump tool from Fedora; I actually
> use it, and perhaps others do, too.  What I've done is provide a patch to
> the pmtools package and filed a bug asking for pmtools to use the
> alternatives
> mechanism for acpixtract (the only command in common between the two
> packages);
> please see BZ#924442 for details.  I've also made that a blocker for this
> bug.



pmtools(acpidump and acpixtract) package was abandoned.
Former maintainer(Len Brown <len.brown>) says:

> -----Original Message-----
> From: Xose Vazquez Perez [xose.vazquez]
> Sent: Saturday, January 26, 2013 10:19 PM
> To: lenb; Brown, Len
> Subject: pmtools: where ??
>
> hi Len,
>
> http://lesswatts.org/projects/acpi/utilities.php is outdated.
> And http://ftp.kernel.org/pub/linux/kernel/people/lenb/acpi/utils/ is empty.
>
> where can I download latest pmtools ?

-------- Original Message --------
Subject: RE: pmtools: where ??
Date: Mon, 28 Jan 2013 21:28:39 +0000
From: Brown, Len <len.brown>
To: Xose Vazquez Perez <xose.vazquez>

pmtools no longer exists.

acpidump is in the linux kernel tree
under tools/power/acpi/

acpixract comes along with iasl in the acpica release
on acpica.org.

cheers,
-Len
-------- End --------


acpidump.* : http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/tools/power/acpi/

Comment 34 Paolo Bonzini 2013-07-21 22:24:02 UTC
Omitting PPC, SPARC and s390 support for iasl would be a regression.  On PPC, SPARC and s390 one can emulate an x86 system using qemu-system-x86.  Cross-compiling firmware is supported, and it uses the iasl binary.

The iasl package contains the necessary patches for big-endian support (unfortunately not supported upstream).

I don't know about the remaining tools; if it is not too hard it would be nice to have them too.

Comment 35 Al Stone (Old account - use ahs3@redhat.com) 2013-07-25 00:01:24 UTC
(In reply to Paolo Bonzini from comment #34)
> Omitting PPC, SPARC and s390 support for iasl would be a regression.  On
> PPC, SPARC and s390 one can emulate an x86 system using qemu-system-x86. 
> Cross-compiling firmware is supported, and it uses the iasl binary.
> 
> The iasl package contains the necessary patches for big-endian support
> (unfortunately not supported upstream).
> 
> I don't know about the remaining tools; if it is not too hard it would be
> nice to have them too.

So far, I have not so much omitted these architectures as left them to the porters for those architectures.  The original patches that provided this support are still carried along for iasl; the spec file will allow the package to be built on these architectures.  In theory, all of the other tools should just work (in practice, that has not been tested).

I'll make some time to determine if the latest versions still work on these architectures and if not, fix them.

Comment 36 Al Stone (Old account - use ahs3@redhat.com) 2013-07-25 00:06:29 UTC
(In reply to Xose Vazquez Perez from comment #33)
> (In reply to Al Stone from comment #21)
> > (In reply to comment #18)
> 
> > >      Note: Package should obsolete pmtools - alternatives mechanism will
> > >      only work if the other package is alternatives-aware, and it appears
> > >      to contain an older version of the same tools. Coordinate the 
> > >      retirement of the obsoleted packages with the other package 
> > >      maintainers.
> > 
> > Obsoleting pmtools would remove the acpidump tool from Fedora; I actually
> > use it, and perhaps others do, too.  What I've done is provide a patch to
> > the pmtools package and filed a bug asking for pmtools to use the
> > alternatives
> > mechanism for acpixtract (the only command in common between the two
> > packages);
> > please see BZ#924442 for details.  I've also made that a blocker for this
> > bug.
> 
> 
> 
> pmtools(acpidump and acpixtract) package was abandoned.
> Former maintainer(Len Brown <len.brown>) says:
> 
> > -----Original Message-----
> > From: Xose Vazquez Perez [xose.vazquez]
> > Sent: Saturday, January 26, 2013 10:19 PM
> > To: lenb; Brown, Len
> > Subject: pmtools: where ??
> >
> > hi Len,
> >
> > http://lesswatts.org/projects/acpi/utilities.php is outdated.
> > And http://ftp.kernel.org/pub/linux/kernel/people/lenb/acpi/utils/ is empty.
> >
> > where can I download latest pmtools ?
> 
> -------- Original Message --------
> Subject: RE: pmtools: where ??
> Date: Mon, 28 Jan 2013 21:28:39 +0000
> From: Brown, Len <len.brown>
> To: Xose Vazquez Perez <xose.vazquez>
> 
> pmtools no longer exists.
> 
> acpidump is in the linux kernel tree
> under tools/power/acpi/
> 
> acpixract comes along with iasl in the acpica release
> on acpica.org.
> 
> cheers,
> -Len
> -------- End --------
> 
> 
> acpidump.* :
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/tools/
> power/acpi/

Note, too, that ACPICA now provides their own version of acpidump, and as of the 20130626-1 I will add it to the acpica-tools package as an alternative to the one from pmtools; it was added just after 20130517 was first packaged.  I'll add a pointer to the 20130626-1 packages as soon as I have them done.

NB: lesswatts.org where the original pmtools source lived no longer exists.

Comment 37 Al Stone (Old account - use ahs3@redhat.com) 2013-07-26 00:38:19 UTC
New upstream version; updated package is here:

Spec URL: http://fedorapeople.org/~ahs3/acpica-tools.spec
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130626-1.fc18.src.rpm
SRPM URL: http://fedorapeople.org/~ahs3/acpica-tools-20130626-1.fc19.src.rpm

Successful koji scratch build for f18 (primary archs):

http://koji.fedoraproject.org/koji/taskinfo?taskID=5658489

Successful koji scratch build for f19 (primary archs):

http://koji.fedoraproject.org/koji/taskinfo?taskID=5658465

Other archs still pending.

Comment 38 Paolo Bonzini 2013-07-26 11:39:57 UTC
> The original patches that provided this
> support are still carried along for iasl

Thanks for confirming, that's enough to avoid regressions!

Comment 39 Al Stone (Old account - use ahs3@redhat.com) 2013-07-26 21:10:09 UTC
Successful koji scratch build for f19 on ppc/ppc64:

http://ppc.koji.fedoraproject.org/koji/taskinfo?taskID=1265768

Comment 40 Al Stone (Old account - use ahs3@redhat.com) 2013-07-26 22:25:41 UTC
Successful koji scratch build for f19 on ARM:

http://arm.koji.fedoraproject.org/koji/taskinfo?taskID=2023272

Comment 41 Al Stone (Old account - use ahs3@redhat.com) 2013-08-18 21:29:26 UTC
New upstream version; updated package is here:

Spec URL: http://ahs3.fedorapeople.org/acpica-tools.spec
SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc18.src.rpm
SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc19.src.rpm
SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc20.src.rpm

Successful koji scratch build for f18 (primary archs):

http://koji.fedoraproject.org/koji/taskinfo?taskID=5827146

Successful koji scratch build for f19 (primary archs):

http://koji.fedoraproject.org/koji/taskinfo?taskID=5827201

Successful koji scratch build for f20 (primary archs):

http://koji.fedoraproject.org/koji/taskinfo?taskID=5827154

Comment 43 Peter Robinson 2013-08-19 10:13:16 UTC
(In reply to Peter Robinson from comment #42)
> > Spec URL: http://ahs3.fedorapeople.org/acpica-tools.spec
> > SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc18.src.rpm
> > SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc19.src.rpm
> > SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc20.src.rpm
> 
> Is there any difference between the 3 src.rpms?

In fact none of them are there. Going with acpica-tools-20130725-1.fc20.src.rpm

Comment 44 Peter Robinson 2013-08-19 10:50:29 UTC
- rpmlint output

A few things to fix up here. Ignore the spelling-error. The main ones that need attention are invalid-url, mixed-use-of-spaces-and-tabs and likely setup-not-quiet.

acpica-tools.src: W: spelling-error %description -l en_US iasl -> ails, sail, isl
acpica-tools.src: W: spelling-error %description -l en_US acpibin -> backspin
acpica-tools.src: W: spelling-error %description -l en_US acpidump -> dumpiness
acpica-tools.src: W: spelling-error %description -l en_US acpiexec -> apiece
acpica-tools.src: W: spelling-error %description -l en_US acpihelp -> perihelia
acpica-tools.src: W: spelling-error %description -l en_US acpinames -> pinnacles
acpica-tools.src: W: spelling-error %description -l en_US acpisrc -> acropolis
acpica-tools.src: W: spelling-error %description -l en_US acpixtract -> extraction
acpica-tools.src: W: spelling-error %description -l en_US pmtools -> pm tools, pm-tools, tools
acpica-tools.src: W: strange-permission run-misc-tests.sh 0755L
acpica-tools.src:36: W: unversioned-explicit-provides acpixtract
acpica-tools.src:44: W: unversioned-explicit-provides acpidump
acpica-tools.src:76: W: setup-not-quiet
acpica-tools.src:103: W: macro-in-comment %{optflags}
acpica-tools.src:103: W: macro-in-comment %{_smp_mflags}
acpica-tools.src:44: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 44)
acpica-tools.src: W: invalid-url Source1: https://www.acpica.org/download/acpitests-unix-20130725.tar.gz HTTP Error 404: Not Found
acpica-tools.src: W: invalid-url Source0: https://www.acpica.org/download/acpica-unix2-20130725.tar.gz HTTP Error 404: Not Found
acpica-tools.spec:36: W: unversioned-explicit-provides acpixtract
acpica-tools.spec:44: W: unversioned-explicit-provides acpidump
acpica-tools.spec:76: W: setup-not-quiet
acpica-tools.spec:103: W: macro-in-comment %{optflags}
acpica-tools.spec:103: W: macro-in-comment %{_smp_mflags}
acpica-tools.spec:44: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 44)
acpica-tools.spec: W: invalid-url Source1: https://www.acpica.org/download/acpitests-unix-20130725.tar.gz HTTP Error 404: Not Found
acpica-tools.spec: W: invalid-url Source0: https://www.acpica.org/download/acpica-unix2-20130725.tar.gz HTTP Error 404: Not Found
1 packages and 1 specfiles checked; 0 errors, 26 warnings.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
- license matches the actual package license

There is no included licence file at all and running a "licensecheck -r acpica-unix2-20130725" produces a bunch of unknown and the following:
generate/unix/iasl/obj/ GPL (v3 or later)

+ latest version packaged

- %doc includes license file
+ spec file written in American English
+ spec file is legible
- upstream sources match sources in the srpm
  can't verify, source URL gives 404
+ package successfully builds on at least one architecture
  tested using koji scratch build
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
n/a binary RPM with shared library files must call ldconfig in %post and %postun+ does not use Prefix: /usr
n/a package owns all directories it creates
n/a no duplicate files in %files
+ Package perserves timestamps on install
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package runtime 
n/a header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a devel must require the fully versioned base
+ packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
+ packages must not own files or directories owned by other packages
+ filenames must be valid UTF-8

Optional:

+ if there is no license file, packager should query upstream to include it
n/a translations of description and summary for non-English languages, if
available
+ reviewer should build the package in mock/koji
n/a the package should build into binary RPMs on all supported architectures
n/a review should test the package functions as described
+ scriptlets should be sane
n/a non -devel packages should require fully versioned base
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or
/usr/sbin
+ Package should have man files

Comment 45 Peter Robinson 2013-08-19 11:12:28 UTC
> The source is being licensed under the GPLv2 (and upstream is pretty
> religious about including the right text in all of the source files).
> 
> Is it necessary to include a copy of GPLv2?  I can, but it seems redundant.

Amusingly "licensecheck --recursive" doesn't properly pick up the GPLv2 license header in any of the source files. 

So while all appear to be GPLv2 there's some in generate/unix/iasl/obj/ that are GPLv3+ so it's possible that at least some binaries are licensed differently.

http://fedoraproject.org/wiki/Licensing:Main#GPL_Compatibility_Matrix

I can't find explicitly where it says we need to include a licence/copying file but in the subpackaging it states the details of it.

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing

Comment 46 Al Stone (Old account - use ahs3@redhat.com) 2013-08-20 02:25:30 UTC
(In reply to Peter Robinson from comment #43)
> (In reply to Peter Robinson from comment #42)
> > > Spec URL: http://ahs3.fedorapeople.org/acpica-tools.spec
> > > SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc18.src.rpm
> > > SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc19.src.rpm
> > > SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-1.fc20.src.rpm
> > 
> > Is there any difference between the 3 src.rpms?
> 
> In fact none of them are there. Going with
> acpica-tools-20130725-1.fc20.src.rpm

Argh.  Sorry about that.  Faulty SCP on my part.  Yes, they are all the same, other than the version number.

Comment 47 Al Stone (Old account - use ahs3@redhat.com) 2013-08-21 00:40:20 UTC
Updated package files:

Spec URL: http://ahs3.fedorapeople.org/acpica-tools.spec
SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130626-2.fc19.src.rpm

Koji results from same:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5835349


(In reply to Peter Robinson from comment #44)
I've fixed up all the warnings reported by rpmlint except for the spelling errors.  At least, the spelling errors are all it reports to me anymore :).

The licensing file I'll touch on below.  I've submitted a possible patch
to upstream for a LICENSE file clarifying things.  We'll see how that goes.


(In reply to Peter Robinson from comment #45)
> > The source is being licensed under the GPLv2 (and upstream is pretty
> > religious about including the right text in all of the source files).
> > 
> > Is it necessary to include a copy of GPLv2?  I can, but it seems redundant.
> 
> Amusingly "licensecheck --recursive" doesn't properly pick up the GPLv2
> license header in any of the source files. 
> 
> So while all appear to be GPLv2 there's some in generate/unix/iasl/obj/ that
> are GPLv3+ so it's possible that at least some binaries are licensed
> differently.

All of the files in generate/unix/iasl/obj are generated files.  The ones noted as GPLv3 are all created during the build by bison.  On reading those files, though, there is a special proviso for using them in non-GPLv3 programs that allows them to be treated as GPLv2 source files.

The UNKNOWN entries are all part of AAPITS tests provided by upstream; per the ACPICA site (www.acpica.org), the FAQ says they can redistributed under the GPL and they are part of the test suite source code ACPICA (upstream) makes available.  I am getting clarification, just to be sure.  Worst case, we would have to take the test cases out of the %check portion of the package.
 
> http://fedoraproject.org/wiki/Licensing:Main#GPL_Compatibility_Matrix
> 
> I can't find explicitly where it says we need to include a licence/copying
> file but in the subpackaging it states the details of it.
> 
> https://fedoraproject.org/wiki/Packaging:
> LicensingGuidelines#Subpackage_Licensing

Right.  I've submitted a patch upstream to clarify; if they accept it, there will be a separate LICENSE file available.

Comment 48 Al Stone (Old account - use ahs3@redhat.com) 2013-08-21 20:57:28 UTC
A little update: the patch I submitted to clarify licensing was not accepted by upstream; they are very reluctant to include a separate file because of their dual licensing (they don't want anyone confused, hence they make it explicit in each source file).

However, I was assured that in the next release (due within a week), all of the UNKNOWNs found by licensecheck before should be fixed and have the same header as all of the other source code files.  Makefiles and scripts may or may not have them by them; I'm investigating another patch to correct those.

So, where does that leave us?  Is the review completed and the package acceptable?  Or do we need to wait until we get the next version from upstream?

Thanks again for all the help!

Comment 49 Al Stone (Old account - use ahs3@redhat.com) 2013-08-26 20:48:23 UTC
Updated package files (new upstream):

Spec URL: http://ahs3.fedorapeople.org/acpica-tools.spec
SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130823-1.fc19.src.rpm

Koji results from same:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5856931

rpmlint results are clean, and licensing has been added to all of the AAPITS files that were previously a problem.

Comment 50 Peter Robinson 2013-08-27 19:31:36 UTC
(In reply to Al Stone from comment #48)
> A little update: the patch I submitted to clarify licensing was not accepted
> by upstream; they are very reluctant to include a separate file because of
> their dual licensing (they don't want anyone confused, hence they make it
> explicit in each source file).

We need to include a copy of the licence because the source isn't necessarily distributed by default (in say a livecd) so it would mean that the package is in violation of the GPL by not distributing the license text with the binaries.

Comment 51 Al Stone (Old account - use ahs3@redhat.com) 2013-08-28 02:06:37 UTC
Found (at fsf.org) and included a copy of the GPLv2 license in the %doc files as 'COPYING' in order to properly comply with the GPL.

Updated package files:

Spec URL: http://ahs3.fedorapeople.org/acpica-tools.spec
SRPM URL: http://ahs3.fedorapeople.org/acpica-tools-20130823-2.fc19.src.rpm

Koji results from same:

http://koji.fedoraproject.org/koji/taskinfo?taskID=5862936

Comment 52 Peter Robinson 2013-08-28 13:56:50 UTC
Looks good to me. APPROVED!

Comment 53 Al Stone (Old account - use ahs3@redhat.com) 2013-08-28 15:56:35 UTC
New Package SCM Request
=======================
Package Name: acpica-tools
Short Description: ACPICA tools for the development and debug of ACPI tables
Owners: ahs3
Branches: f18 f19 f20
InitialCC:

Comment 54 Gwyn Ciesla 2013-08-28 16:16:07 UTC
Git done (by process-git-requests).

Comment 55 Fedora Update System 2013-08-28 21:46:10 UTC
acpica-tools-20130823-2.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/acpica-tools-20130823-2.fc19

Comment 56 Fedora Update System 2013-08-28 21:46:21 UTC
acpica-tools-20130823-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/acpica-tools-20130823-2.fc18

Comment 57 Fedora Update System 2013-08-29 22:23:02 UTC
acpica-tools-20130823-2.fc19 has been pushed to the Fedora 19 testing repository.

Comment 58 Fedora Update System 2013-09-07 01:27:23 UTC
acpica-tools-20130823-2.fc18 has been pushed to the Fedora 18 stable repository.

Comment 59 Fedora Update System 2013-09-07 01:28:25 UTC
acpica-tools-20130823-2.fc19 has been pushed to the Fedora 19 stable repository.


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