[Bug 904843] Review Request: acpica-tools - ACPICA tools for the development and debug of ACPI tables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=904843

--- Comment #8 from Al Stone <ahs3@xxxxxxxxxx> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=D4pkazRxLP&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]