Bug 225881 - Merge Review: hardlink
Merge Review: hardlink
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: manuel wolfshant
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:02 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-14 13:52:04 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
wolfy: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:02:41 EST
Fedora Merge Review: hardlink

http://cvs.fedora.redhat.com/viewcvs/devel/hardlink/
Initial Owner: jnovy@redhat.com
Comment 1 Roozbeh Pournader 2007-02-03 21:14:59 EST
BLOCKER:
No upstream or URL mentioned to check against (MUST item)
Comment 2 Jindrich Novy 2007-02-05 07:35:19 EST
Hardlink desn't have an upstream actually as the only repository, would you
object to have an URL like this:

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

in the URL tag?
Comment 3 manuel wolfshant 2007-02-05 07:56:55 EST
The purpose of the URL is to inform the users (and the reviewers :) ) where to
look for additional info about the content of the package. Your suggestion is OK
assuming that content at that the URL is there to stay and that it will reflect
further changes on the software.

You should also consider 
- switching to %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n) as
buildroot
- using just the rm -fR part in %clean
- I think (I am not sure now) that -Wall is part of $RPM_OPT_FLAG

Why was the epoch field needed ? I see no mention about it in the Changelog,
just a bump in Feb 2006
Comment 4 Roozbeh Pournader 2007-02-05 08:21:42 EST
(In reply to comment #3)
> Why was the epoch field needed ? I see no mention about it in the Changelog,
> just a bump in Feb 2006

Not related to review of course. If it was added somewhen, it's going to stay
with us forever...
Comment 5 Jindrich Novy 2007-02-05 08:34:26 EST
Please check the current CVS hardlink at:

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

If you don't have any other objection I'm going to build this version.
Comment 6 Jindrich Novy 2007-02-05 08:37:00 EST
Or better, checkout it from CVS directly, the webcvs needs some time to get
synced with the latest commit I just made.
Comment 7 Ralf Corsepius 2007-02-05 08:40:40 EST
MUSTFIX:
- Remove this:
[ "$RPM_BUILD_ROOT" != "/" ] && [ -d $RPM_BUILD_ROOT ] && rm -rf $RPM_BUILD_ROOT;

- Remove the -g from this:
gcc -Wall $RPM_OPT_FLAGS -g %{SOURCE0} -o hardlink

This overrides debug flags that might be contained in RPM_OPT_FLAGS
Comment 8 Jindrich Novy 2007-02-05 08:45:36 EST
Ralf Corsepius: cvs up

your are looking into an old revision.

I removed the stuff from the %clean in the last commit.
Comment 9 Ralf Corsepius 2007-02-05 09:20:04 EST
(In reply to comment #8)
> Ralf Corsepius: cvs up
> 
> your are looking into an old revision.
I was looking at the revision, that was current ca 20 seconds before I sent the
mail.

> I removed the stuff from the %clean in the last commit.
Sorry, if this hit you, but unless RH finally starts to provide usable services,
I don't see any reason to continue any "merge-review".


Comment 10 Jindrich Novy 2007-02-05 09:54:41 EST
Ralf, I meant the 

gcc -Wall $RPM_OPT_FLAGS -g %{SOURCE0} -o hardlink

which was missing from the hardlink.spec for a while. I fixed the %clean section
after reading your comment.
Comment 11 Jindrich Novy 2007-04-23 08:52:57 EDT
ping?
Comment 12 manuel wolfshant 2007-04-23 09:34:10 EDT
The theory says that Source0 should be a full (downloadable) URL. Given the fact
that upstream is .. hugh.. you, I think that you could just add a comment with
instructions on how to get (a specific version) from CVS.


GOOD

rpmlint checks:
Source RPM:
W: hardlink unversioned-explicit-obsoletes kernel-utils
rpmlint of hardlink:
W: hardlink obsolete-not-provided kernel-utils
--> seems correct, the kernel-utils package has been replaced by a lot of other
smaller packages which include all the utilities, one par package
- package meets naming guidelines
- package meets packaging guidelines
- license (GPL) OK, matches source
- pec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all files that it creates; does not create any directories, does not take
ownership of other files or directories
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file 
- no static, .la, .pc files

SHOULD
- builds fine in mock/devel/i386 and x86_64
- runs as advertised

package is APPROVED but before importing please
- fix timestamp preserving of man page (install -pm hardlink.1)
- fix the %make step to take into account SMP flags (not that it would matter
much for this small program, but the guidelines request it)
- add to the package and include in the RPM as %doc the GPL license. It is
mentioned in the C source, but it would be wise to also include it in full
Comment 13 manuel wolfshant 2007-04-23 09:35:48 EDT
Jindrich, please do not assign the bug to you. The bug should be assigned to the
reviewer, not to the packager.
Comment 14 Jindrich Novy 2007-04-23 10:20:42 EDT
Thanks for the review!
Comment 15 manuel wolfshant 2007-06-14 13:50:53 EDT
Jindrich, could you please close this bug ?
Comment 16 Jindrich Novy 2007-06-14 13:52:04 EDT
Sure.

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