Bug 225244

Summary: Merge Review: amtu
Product: [Fedora] Fedora Reporter: Nobody's working on this, feel free to take it <nobody>
Component: Package ReviewAssignee: Kevin Fenzi <kevin>
Status: CLOSED RAWHIDE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: sgrubb, toshio
Target Milestone: ---Flags: kevin: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-05-30 02:53:23 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Nobody's working on this, feel free to take it 2007-01-29 21:00:23 UTC
Fedora Merge Review: amtu

http://cvs.fedora.redhat.com/viewcvs/devel/amtu/

Comment 1 Kevin Fenzi 2007-02-03 18:52:13 UTC
I would be happy to review this package. 
Look for a full review in a bit. 


Comment 2 Kevin Fenzi 2007-02-03 19:31:17 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (CPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should have dist tag
See below - Should package latest version
0 outstanding bugs - check for outstanding bugs on package.

Issues:

1. Where is the upstream for this version?
http://sourceforge.net/project/showfiles.php?group_id=130497
seems to have version 1.0.2, where is 1.0.4 available?

2. our pal rpmlint says:

rpmlint on ./amtu-debuginfo-1.0.4-4.fc7.i386.rpm
W: amtu-debuginfo invalid-license Common Public License

This seems to be ignoreable... the CPL is a ok license.

W: amtu-debuginfo no-url-tag

Should add URL tag, perhaps:
http://sourceforge.net/project/showfiles.php?group_id=130497

rpmlint on ./amtu-1.0.4-4.fc7.src.rpm
W: amtu invalid-license Common Public License
W: amtu no-url-tag
W: amtu setup-not-quiet

Setup should have -q on it?
E: amtu configure-without-libdir-spec

Why aren't you using %configure?

W: amtu macro-in-%changelog clean
W: amtu macro-in-%changelog files

Those should be %%clean and %%files

E: amtu non-readable /usr/bin/amtu 0750
E: amtu non-standard-executable-perm /usr/bin/amtu 0750

Why is this 750?

W: amtu wrong-file-end-of-line-encoding /usr/share/doc/amtu-1.0.4/AMTUHowTo.txt

This should be fixed to not have doc line endings...

3. Why the compiler setting stuff in build?
Also, can you use %{smp_mflags} ?

4. Is the "Requires: audit >= 1.1.2" required? Looks like rpm picks up the
libaudit requirement...

Comment 3 Steve Grubb 2007-02-04 14:03:10 UTC
1. Where is the upstream for this version?

IBM has it.

2. our pal rpmlint says:

>Should add URL tag, perhaps:
>http://sourceforge.net/project/showfiles.php?group_id=130497

I don't know where IBM distributes it from. I work directly with them on CC
evals and the tarball is emailed to me.

>Setup should have -q on it?

I suppose it could. Doesn't hurt anything either way.

>Why aren't you using %configure?

In my experience, some packages will not build with %configure. We might be able
to change it.

>W: amtu macro-in-%changelog clean

will change

>W: amtu macro-in-%changelog files

will change

>E: amtu non-readable /usr/bin/amtu 0750
>E: amtu non-standard-executable-perm /usr/bin/amtu 0750
>
>Why is this 750?

Because it needs to be. It requires privileges to run. If it were available to
accounts that could run it and fail, the audit requirements are higher.

>W: amtu wrong-file-end-of-line-encoding /usr/share/doc/amtu-1.0.4/AMTUHowTo.txt
>
>This should be fixed to not have doc line endings...

Attach a patch please.

>3. Why the compiler setting stuff in build?

Because it requires special handling between the arches and to follow the
guidelines issued internally to Red Hat. If you have redhat-rpm-config
installed, there are build flags that get picked up this way.

>Also, can you use %{smp_mflags} ?

why?

>4. Is the "Requires: audit >= 1.1.2" required? Looks like rpm picks up the
>libaudit requirement...

Yes, but there are different versions of audit and that one has the api that we
want.


Comment 4 Kevin Fenzi 2007-02-04 15:20:37 UTC
In reply to comment #3: 

>I don't know where IBM distributes it from. I work directly with them on CC
>evals and the tarball is emailed to me.

Could you perhaps ping them to update the sourceforge site with the latest?
Then it would be easy to check against upstream. 

>>Setup should have -q on it?
>
>I suppose it could. Doesn't hurt anything either way.

True, just more in line with all the other packages... 

>>Why aren't you using %configure?
>
>In my experience, some packages will not build with %configure. We might be
>able to change it.

Can you try and do so?

>>Why is this 750?
>
>Because it needs to be. It requires privileges to run. If it were available to
>accounts that could run it and fail, the audit requirements are higher.

Sounds reasonable. 

>>This should be fixed to not have doc line endings...
>
>Attach a patch please.

You can do it in the spec... just add: 

%{__sed} -i 's/\r//' AMTUHowTo.txt

>>3. Why the compiler setting stuff in build?
>
>Because it requires special handling between the arches and to follow the
>guidelines issued internally to Red Hat. If you have redhat-rpm-config
>installed, there are build flags that get picked up this way.

It seems like if that was the case you would want to just override the
RPM_OPT_FLAGS in those places (ie, in rpm or macro files). 

>>Also, can you use %{smp_mflags} ?
>
>why?

Indeed this is a small package and it might not be worth it, but if it works, 
why not? Would save a bit of build time. 

Agreed on everything else... 

When you have updated rawhide/cvs, let me know and reassign me on this bug, 
and I will be happy to check over changes. 


Comment 5 Steve Grubb 2007-02-08 15:31:02 UTC
amtu-1.0.4-5.fc7 was built to satisfy most of these requests. I looked at
AMTUHowTo.txt and don't see anything wrong with it. Neither vi, cat, or gedit
have a problem displaying the file...so I am reluctant to make a change for
something that's not causing problems.

Comment 6 Kevin Fenzi 2007-02-09 04:03:55 UTC
Thanks Steve. 

Reassigning this to me to look at. Hopefully later tonight, but if not, tomorrow. 
Thanks for the prompt fixes... 

Comment 7 Kevin Fenzi 2007-02-11 20:34:42 UTC
Sorry for the delay in getting back to this review. 

Everything looks fixed except for 3 items... 

1. On the AMTUHowTo.txt having DOS/win line endings, I guess that fixing that
isn't a blocker. The rpmlint info for that says: "could prevent it from being
displayed correctly in some circumstances." I am unsure what those circumstances
would be. 

2. Forgot to mention that your buildroot is not correct. You should use:
      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

3. Not sure what we can do about the upstream source not being somewhere that
can be checked. The package review guidelines have: 
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.

There does appear to be an upstream source on the sourceforge project, but it's
not the one you are shipping. Have you been able to contact the IBM source to
update that project with the source you are shipping?

Once you have addressed these items (either by making the suggested changes, or
by explaining why they don't make sense), please reassign this review back 
to me, and change the 'fedora-review' flag back to ? for me to take action. 

Comment 8 Steve Grubb 2007-02-17 01:01:50 UTC
I built a new version of amtu to update the buildroot.

Comment 9 Kevin Fenzi 2007-02-24 03:59:37 UTC
Thanks Steve. 

Any word from IBM about them distributing the source via sourceforge?
Not sure what else to do here, since we really should be able to verify 
the upstream source. :( 

I'm going to CC toshio on this bug to see if he has any ideas on how we 
can meet the guidelines here. 

I'm also going to move the assigned and such around per the new offical 
review guidelines. More info at: 
https://www.redhat.com/archives/fedora-maintainers/2007-February/msg00682.html

Comment 10 Toshio Kuratomi 2007-02-26 05:40:10 UTC
Hmm... So how did we get the sources from IBM?  If they have a public source
control repository that would be one way to point to where we got the sources. 
If it came to use via private mail or on a disk from IBM or something then it
seems like we have a special relationship with IBM for this.  Is any other
distro shipping this version?  Or are we the only ones?

Comment 11 Steve Grubb 2007-02-26 13:25:11 UTC
Yes, we do have a special relationship with IBM. I have no idea if anyone else
ships this version. As mentioned in comment #3, the source code was emailed to me.

Comment 12 Toshio Kuratomi 2007-02-26 19:21:16 UTC
Sorry Steve, I missed that part of #3 when I read this the first time.

I would say, unless IBM makes new releases on sourceforge, we should treat this
as a case of We Are Upstream:
  http://www.fedoraproject.org/wiki/PackagingDrafts/SourceUrl#WeAreUpstream
(That was voted on last week and will be made official tomorrow as no one has
objected.)

the comment should contain something like: 
# We work closely with IBM on this from sources that they share with us via email.
# Unless IBM begins to update their work on amtu.sf.net, this SRPM will be
# considered the upstream source.

Comment 13 Kevin Fenzi 2007-02-27 23:46:12 UTC
I'm ok with adding the comment per comment #12 to the spec. 

I would add that the next time IBM mails you a new source, you might reply and
ask them to consider releasing to the sourceforge site as well. ;) 

That was the last blocker I see here... I will go ahead and APPROVE this package
now, and you can close it rawhide once you have added that comment and the new
version has been pushed out. 

Thanks for your attention Steve, and thanks for assisting with this review Toshio. 

Comment 14 Steve Grubb 2007-03-08 15:24:32 UTC
New upstream version was built with sources that are public! So, you can check
the md5sum and we do not need to put we are upstream in the srpm at this point.

Comment 15 Kevin Fenzi 2007-03-08 17:29:46 UTC
Excellent! 
I just checked and the sources now match up with the upstream project just fine. 

Thanks again. 

Comment 16 Kevin Fenzi 2007-05-30 02:53:23 UTC
Since this is approved and released, closing this bug.