Bug 225244
Summary: | Merge Review: amtu | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Nobody's working on this, feel free to take it <nobody> |
Component: | Package Review | Assignee: | Kevin Fenzi <kevin> |
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | 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
I would be happy to review this package. Look for a full review in a bit. 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... 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. 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. 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. Thanks Steve. Reassigning this to me to look at. Hopefully later tonight, but if not, tomorrow. Thanks for the prompt fixes... 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. I built a new version of amtu to update the buildroot. 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 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? 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. 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. 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. 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. Excellent! I just checked and the sources now match up with the upstream project just fine. Thanks again. Since this is approved and released, closing this bug. |