Bug 445224

is this for ppc64 arch only package and not on i386?

yes, although with some work i386 support could be added.

A couple of brief questions:

Is the .h file really needed at runtime?  If not, it should likely be in a
-devel subpackage.

Does the package not build at all on x86, or is it a matter of missing tapsets?

When you run the Systemtap translator, stap, it builds a kernel module, compiles
it and runs it.  The usr_itrace.h is needed to compile the generated kernel module.

The package has not been developed for x86, although part usr_itrace.stp tapset
was developed and tested somewhat on x86.  I'm just not sure if there pieces
missing for x86 due to lack of development/testing on x86.

(In reply to comment #4)
> When you run the Systemtap translator, stap, it builds a kernel module, compiles
> it and runs it.  The usr_itrace.h is needed to compile the generated kernel
module.

Ah, yes.  Ok, makes sense.

> The package has not been developed for x86, although part usr_itrace.stp tapset
> was developed and tested somewhat on x86.  I'm just not sure if there pieces
> missing for x86 due to lack of development/testing on x86.

Hm.  Ok.  We'll come back to that in a bit.

More questions:

This looks like a subset of the dpiperf.dynamic package.  It seems to contain a
newer (?) version of the dpiperf.dynamic tarball that has simply been rebranded
to stapitrace-<version>.tar.gz in the SPEC file.  Is this a CVS snapshot of the
dpiperf tarball?

I ask because if it is, you should probably follow the snapshot guidelines[1]
instead of creating a tarball that can't actually be downloaded from the project
site.  One of the review criteria is that the package has verifiable source:

"- MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL. Reviewers should use md5sum for this task. If no
upstream URL can be specified for this package, please see the  Source URL
Guidelines for how to deal with this."


[1] https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

(In reply to comment #5)
 
> 
> More questions:
> 
> This looks like a subset of the dpiperf.dynamic package.  It seems to contain a
> newer (?) version of the dpiperf.dynamic tarball that has simply been rebranded
> to stapitrace-<version>.tar.gz in the SPEC file.  Is this a CVS snapshot of the
> dpiperf tarball?
The tarball is one that was created for me by the Performance Inspector(PI)
maintainer which includes the modifications that I need to their source.  My
changes have been comitted into CVS, just not available yet on the PI web site.
 The maintainer assures me that a newer tarball containing my source mods will
be available on the PI web site in July.

Is it possible to specify the URL for the tarball on the PI web site in the spec
file?  I think I tried setting the URL and Source0 variables in the spec file to
point to a tarball on the PI website, but my recollection is that it looked for
Source0 in the SOURCES directory anyways.  I thought the URL variable in the
spec file was just supposed to name the project web site where documentation can
be found.  When the updated tarball is available, should I set the URL variable
to be the path that would be used by wget to get the tarball?  In that case
should I change the Source0 variable to use that same name as indicated in the
URL variable?
> 
> I ask because if it is, you should probably follow the snapshot guidelines[1]
I was planning on somehow pointing to the publicly available tarball with my
source changes in it, but I wonder if I wouldn't be better off referencing cvs
since it is probably inevitable that the review process will turn up something
that I need to change.  Would I just manually grab a CVS snapshot and build my
own tarball with YYYYMMDDcvs in the name and reference it from the Source0
variable?  e.g. Dpiperf-YYYYMMDDcvs.tar.gz   (Dpiperf is a PI naming convention)

Do you think I should make this a pre-release package with an "alpha" in the
Release name?

> instead of creating a tarball that can't actually be downloaded from the project
> site.  One of the review criteria is that the package has verifiable source:
> 
> "- MUST: The sources used to build the package must match the upstream source,
> as provided in the spec URL. Reviewers should use md5sum for this task. If no
> upstream URL can be specified for this package, please see the  Source URL
> Guidelines for how to deal with this."
> 
> 
> [1] https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

source files match upstream:
 ab74f8ed90b8ba9a1d3904892622fc80 (tarball matches tarball found in upstream SRPM)
specfile is properly named, is cleanly written and uses macros consistently.
dist tag is present.
build root is correct
license field matches the actual license. (though itrace.c has GPL v2.1 listed,
which doesn't exist.  That should probably be fixed upstream)
license is open source-compatible.
license text not included upstream.
latest version is being packaged.
BuildRequires are proper.
compiler flags are appropriate.
%clean is present.
rpmlint is silent. 
owns the directories it creates.
doesn't own any directories it shouldn't.
no duplicates in %files.
file permissions are appropriate.
no scriptlets present.
code, not content.
documentation is small, so no -docs subpackage is necessary.
%docs are not necessary for the proper functioning of the package.
no headers.
no pkgconfig files.
no libtool .la droppings.

Needs fixing:

package meets naming and versioning guidelines.
package builds in mock:
http://koji.fedoraproject.org/koji/getfile?taskID=664952&name=build.log
package installs properly. (couldn't check)
debuginfo package looks complete. (couldn't check)
final provides and requires are sane (couldn't check)
if shared libraries are present, make sure ldconfig is run

(In reply to comment #7)
> source files match upstream:
>  ab74f8ed90b8ba9a1d3904892622fc80 (tarball matches tarball found in upstream SRPM)
> specfile is properly named, is cleanly written and uses macros consistently.
> dist tag is present.
> build root is correct
> license field matches the actual license. (though itrace.c has GPL v2.1 listed,
> which doesn't exist.  That should probably be fixed upstream)
> license is open source-compatible.
> license text not included upstream.
> latest version is being packaged.
> BuildRequires are proper.
> compiler flags are appropriate.
> %clean is present.
> rpmlint is silent. 
> owns the directories it creates.
> doesn't own any directories it shouldn't.
> no duplicates in %files.
> file permissions are appropriate.
> no scriptlets present.
> code, not content.
> documentation is small, so no -docs subpackage is necessary.
> %docs are not necessary for the proper functioning of the package.
> no headers.
> no pkgconfig files.
> no libtool .la droppings.
> 
> Needs fixing:
> 
> package meets naming and versioning guidelines.
> package builds in mock:
> http://koji.fedoraproject.org/koji/getfile?taskID=664952&name=build.log
> package installs properly. (couldn't check)
> debuginfo package looks complete. (couldn't check)
> final provides and requires are sane (couldn't check)
> if shared libraries are present, make sure ldconfig is run

So, the only issue under the "Needs fixing" category is the build error you got
in mock.  I'll look into that one.

Also, did you have any response to my questions (in previous comment) about how
I should be referencing the source:  tarball vs. CVS

(In reply to comment #8) 
> > Needs fixing:
> > 
> > package meets naming and versioning guidelines.
> > package builds in mock:
> > http://koji.fedoraproject.org/koji/getfile?taskID=664952&name=build.log
> > package installs properly. (couldn't check)
> > debuginfo package looks complete. (couldn't check)
> > final provides and requires are sane (couldn't check)
> > if shared libraries are present, make sure ldconfig is run
> 
> So, the only issue under the "Needs fixing" category is the build error you got
> in mock.  I'll look into that one.

That, and the naming.  See below.

> Also, did you have any response to my questions (in previous comment) about how
> I should be referencing the source:  tarball vs. CVS

If you are still working on the feature upstream and it's not released as of
yet, I would recommend a CVS snapshot and a pre-release package, yes.

Once the utility is included in a real upstream tarball, you can fixup the RPM
specfile to use that.  While it's a bit more work upfront for the packager, it
more accurately reflects the state of the package upstream and gives you a bit
more flexibility if you want to respin the snapshot to bring in more fixes, etc.

(In reply to comment #9)
> > So, the only issue under the "Needs fixing" category is the build error you got
> > in mock.  I'll look into that one.
> 
> That, and the naming.  See below.
I didn't see anything below about the naming issue.  What is the naming problem?

> > > package meets naming and versioning guidelines.
> > > package builds in mock:
> > > http://koji.fedoraproject.org/koji/getfile?taskID=664952&name=build.log
I think the problem in the build was because configure didn't really work.
What kind OS was on your test machine?  I think the crux of the problem is in
the output from configure:
--------------------------------------------------------------------------
CPU = powerpc64, OS = linux-gnu, Vendor = redhat, HostType = powerpc64
--------------------------------------------------------------------------
ERROR: Platform powerpc64 not yet supported

On the fedora9 ppc machine I tried I got:
--------------------------------------------------------------------------
CPU = powerpc64, OS = linux-gnu, Vendor = redhat, HostType = powerpc
--------------------------------------------------------------------------

So, it is HostType=powerpc vs HostType=powerpc64.  On my machine I don't get
this configuration error.  

What does config.guess return on your machine?

On my Fedora9 ppc machine I get:
powerpc64-redhat-linux-gnu

which is the same thing I see in a RHEL5.2 ppc machine.

If you think that the configure script should be able to handle the
HostType=powerpc64, I can go ahead and patch the configure.in.

> > > package installs properly. (couldn't check)
> > > debuginfo package looks complete. (couldn't check)
> > > final provides and requires are sane (couldn't check)
> > > if shared libraries are present, make sure ldconfig is run
What does this mean by "couldn't check"? I was unable to do hardly any testing
on the fedora9 machine because I couldn't find compatible pieces like
binutils-devel since my machine is in ABAT.  I spent about a week some time ago
trying to get Fedora9-alpha installed from CDs on a ppc machine but never could
get it to work.

> 
> That, and the naming.  See below.
> 
What is the naming issue?

(In reply to comment #11)
> > > > package meets naming and versioning guidelines.
> > > > package builds in mock:
> > > > http://koji.fedoraproject.org/koji/getfile?taskID=664952&name=build.log
> I think the problem in the build was because configure didn't really work.
> What kind OS was on your test machine?  

This was an official Fedora rawhide builder.

> I think the crux of the problem is in
> the output from configure:
> --------------------------------------------------------------------------
> CPU = powerpc64, OS = linux-gnu, Vendor = redhat, HostType = powerpc64
> --------------------------------------------------------------------------
> ERROR: Platform powerpc64 not yet supported
> 
> On the fedora9 ppc machine I tried I got:
> --------------------------------------------------------------------------
> CPU = powerpc64, OS = linux-gnu, Vendor = redhat, HostType = powerpc
> --------------------------------------------------------------------------
> 
> So, it is HostType=powerpc vs HostType=powerpc64.  On my machine I don't get
> this configuration error.  
> 
> What does config.guess return on your machine?

There were some changes in rawhide recently on this.  I'll see if I can dig up
the email thread.

> On my Fedora9 ppc machine I get:
> powerpc64-redhat-linux-gnu
> 
> which is the same thing I see in a RHEL5.2 ppc machine.

RHEL 5.x has no bearing here.  It's quite old compared to Fedora.

> If you think that the configure script should be able to handle the
> HostType=powerpc64, I can go ahead and patch the configure.in.

That might have been the solution, but I'll check.

> > > > package installs properly. (couldn't check)
> > > > debuginfo package looks complete. (couldn't check)
> > > > final provides and requires are sane (couldn't check)
> > > > if shared libraries are present, make sure ldconfig is run
> What does this mean by "couldn't check"? 

The package didn't build, so I couldn't check those items.  Looking at the spec
file, I don't expect any issues.

> I was unable to do hardly any testing
> on the fedora9 machine because I couldn't find compatible pieces like
> binutils-devel since my machine is in ABAT.  I spent about a week some time ago
> trying to get Fedora9-alpha installed from CDs on a ppc machine but never could
> get it to work.

Fedora 9 has been released for a month now.  Try using the Gold release, as
Alpha is very old and is known to have bugs.

> > That, and the naming.  See below.
> > 
> What is the naming issue?

Sorry, I should have been more clear.  I included the Release and Version fields
here.  If you do a CVS snapshot package, you'll need to fixup the Release and
Version fields appropriately.

SPEC: http://jwboyer.fedorapeople.org/stapitrace.spec
SRPM:
http://jwboyer.fedorapeople.org/stapitrace-1.0.0-0.20080622cvs_alpha.fc9.src.rpm

All of the review issues have been worked out with Dave in the above files.  I
consider this APPROVED.

New Package CVS Request
=======================
Package Name: stapitrace
Short Description: user instruction tracing tool
Owners: dcnomura
Branches:
InitialCC: dcnomura
Cvsextras Commits: yes

cvs done.

Created attachment 313294 [details]
updates for Roland McGraths upgrade to utrace

Roland McGrath is upgrading utrace in Fedora10.  The attached tar file contains a src rpm, spec file, and patch file to upgrade Dpiperf/src/stap/usr_itrace.stp

This is based on a preliminary version of Roland's changes and has only had limited testing due to the need for uprobes (which also must be adapted for the new utrace)

(In reply to comment #17)
> Created an attachment (id=313294) [details]
> updates for Roland McGraths upgrade to utrace
> 
> Roland McGrath is upgrading utrace in Fedora10.  The attached tar file contains
> a src rpm, spec file, and patch file to upgrade Dpiperf/src/stap/usr_itrace.stp
> 
> This is based on a preliminary version of Roland's changes and has only had
> limited testing due to the need for uprobes (which also must be adapted for the
> new utrace)

Just handle this in CVS at your leisure, or open a different bug for it.  The review is done and this bug should be closed out.

Package Change Request
======================
Package Name: stapitrace
New Branches: EL-5

Could we get an ack from the current owner of this package?  He hasn't indicated any preference with respect to EPEL branches, so we can't just go ahead and create them.  The EPEL policy is at http://fedoraproject.org/wiki/Getting_a_Fedora_package_in_EPEL

(In reply to comment #20)
> Could we get an ack from the current owner of this package? 

The fact that Maynard isn't listed as the primary package owner is an oversight.  He owns this for all intents and purposes and he is listed as a fully authorized co-maintainer on all branches.

He has my ACK to do whatever he wants with this.

CVS Done