Bug 210790 - Review Request: dar - Collection of scripts for making/restoring CD/DVD backups
Summary: Review Request: dar - Collection of scripts for making/restoring CD/DVD backups
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-10-15 02:52 UTC by Chris Petersen
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-11-15 22:04:28 UTC
Type: ---
Embargoed:
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Chris Petersen 2006-10-15 02:52:36 UTC
Spec URL: http://rpm.forevermore.net/dar/dar.spec
SRPM URL: http://rpm.forevermore.net/dar/dar-2.3.1-1.src.rpm

Description:

DAR is a command line tool to backup a directory tree and files. DAR is
able to make differential backups, split them over a set of disks or files
of a given size, use compression, filter files or subtrees to be saved or
not saved, directly access and restore given files. DAR is also able
to handle extented attributes, and can make remote backups through an
ssh session for example. Finally, DAR handles save and restore of hard
and symbolic links.

Comment 1 Mamoru TASAKA 2006-10-19 16:45:14 UTC
1.
Well, static binary is strictly forbidden in fedora.
1-A please explain why it is needed (note: even if you explain why,
     perhaps most reviewers including me will not accept it).
1-B I don't know well about NSS service, however, static binary in this
     package seems meaningless anyway because you can see in the build log:

/builddir/build/BUILD/dar-2.3.1/src/libdar/.libs/libdar.a(tools.o): In function
`libdar::tools_name_of_gid(unsigned short)':
/builddir/build/BUILD/dar-2.3.1/src/libdar/tools.cpp:416: warning: Using
'getgrgid' in statically linked applications requires at runtime the sh
ared libraries from the glibc version used for linking
/builddir/build/BUILD/dar-2.3.1/src/libdar/.libs/libdar.a(tools.o): In function
`libdar::tools_name_of_uid(unsigned short)':
/builddir/build/BUILD/dar-2.3.1/src/libdar/tools.cpp:402: warning: Using
'getpwuid' in statically linked applications requires at runtime the sh
ared libraries from the glibc version used for linking

* Similarly, please explain why %{_libdir}/*.a is necessary (as said above,
  even if you explained why, it won't be accepted unless there is quite a
  reasonable reason).

Comment 2 Chris Petersen 2006-10-19 16:51:50 UTC
The static binary is required because this is a backup program; the static
binary is copied to the backup disks for ease of restoration on a system that
can't/doesn't have dar otherwise installed (also why it's a separate package,
since this is an optional feature encouraged by upstream).  I don't know what's
up with the dynamic linking, but it sounds like something that should be fixed
(I'll ask upstream assuming the previous "why" is a worthy answer).

I don't know what .a files are, so they're included.

Comment 3 Chris Petersen 2006-10-19 16:53:36 UTC
Let me ammend that:

I don't know what .a files are, so they're included.  If they shouldn't be, I'll
remove them.

Comment 4 Mamoru TASAKA 2006-10-19 17:14:25 UTC
(In reply to comment #3)
> I don't know what .a files are, so they're included.  
> If they shouldn't be, I'll remove them.

*.a files are static archives and  except for a few exceptions
these files should be removed, especially when symlinks against dynamic 
libraries are available (i.e. *.so).

(In reply to comment #2)
> I don't know what's
> up with the dynamic linking, but it sounds like something that should be fixed
> (I'll ask upstream assuming the previous "why" is a worthy answer).

By google, this means that dar_static needs glibc libraries used when
building this. If static binary is needed, this needs fixing.


Comment 5 Mamoru TASAKA 2006-10-21 11:22:34 UTC
Well, would you contact upstream about the warning I have
commented?

Comment 6 Chris Petersen 2006-10-23 22:16:06 UTC
And what makes you think I haven't?   It just took awhile to get a reply, which
I am pasting below at his request:

Hello,

Chris has well explained the reason why a statically linked version of dar is
built, and why it is provided by default.

If statically linked binary as well as static library is not allowed, you can
avoid having them being built using the following configuration options:

./configure --disable-dar-static --disable-static

the first option disables the building of dar_static, the second disables the
building of a static version of libdar.

Please however inform users that the dar_static version (mentionned in the
documentation) is not built due to Fedora internal policy.

Kind Regards,
Denis.

Comment 7 Chris Petersen 2006-10-23 22:20:24 UTC
And on that note, Denis has also pointed out that the problem appears to be a
bug (or feature?) in glibc that just prevents it from making proper static
binaries, so unless fedora has an ability to build with an older glibc, I'll
just make the -static subpackage an option for building from srpm for those who
want it.

Comment 8 Mamoru TASAKA 2006-10-25 08:09:12 UTC
(In reply to comment #7)
> And on that note, Denis has also pointed out that the problem appears to be a
> bug (or feature?) in glibc that just prevents it from making proper static
> binaries, so unless fedora has an ability to build with an older glibc, I'll
> just make the -static subpackage an option for building from srpm for those who
> want it.

The problem appears not for "building" static dar binary, but for "using"
static binary as using NSS service requires the same glibc library used
which is used when building static-dar.

And this is NSS feature and not specified to fedora. Debian seems to
have static dar binary, however I think this is wrong anyway. 
So I think if using static dar is preferable, the source code of dar
used for static mode surely needs fixing so as not to use NSS service.

My current opinion is that this is dar-static bug and dar-static should
not be included unless source code is fixed properly.

Comment 9 Mamoru TASAKA 2006-10-31 14:43:53 UTC
ping?

Comment 10 Chris Petersen 2006-10-31 16:47:07 UTC
Been busy.  Will get to this when I have time.

Comment 11 Chris Petersen 2006-11-04 08:28:02 UTC
ok, updated files:

http://rpm.forevermore.net/dar/dar-2.3.1-2.src.rpm
http://rpm.forevermore.net/dar/dar.spec

Disabled the static build by default, esp. since the current glibc seems to be
broken and won't actually produce a real static binary, anyway.

I can't retest this in mock at the moment since I'm still in the middle of
setting up fc6, but it should still work fine.

Comment 12 Mamoru TASAKA 2006-11-04 14:19:22 UTC
Okay, first full review of this package (dar).
Almost okay.

1. From http://fedoraproject.org/wiki/Packaging/Guidelines :
*  rpmlint
   rpmlint is not silent.

W: dar file-not-utf8 /usr/share/man/man1/dar.1.gz
   dar.1.gz contains ISO-8859-1 character (around the line 341).
--------------------------------------------------------------------
   This allows one to merge two archive in a single one. See also -$, 
  -<THIS CHARACTER> and -% for other options concerning auxiliary 
   archive of reference
--------------------------------------------------------------------
   Please change this character MANUALLY (iconv does not work for
   this case as this character seems to be tilde, while iconv tries
   to change this character to English pound character.

* Requires:
  - For libdar-devel
    Usually the dependency for main package (for this package, it
    is libdar) is release-specific. i.e.
    Requires: libdar = %{version}-%{release}

  - Provides like
    Provides:   libdar = %{version}-%{release}
    are all unnecessary as rpm always provides these implicitly.

* BuildRequires:
  - Redundant BuildRequires is found.
    * zlib-devel <- this is required by openssl-devel

2. http://fedoraproject.org/wiki/Packaging/ReviewGuidelines = Nothing.

3. Other things I have noticed:
* README.Fedora
  - Okay, this document is very preferable. Also I recommend you add 
    your name and the date when you wrote this.


Comment 13 Mamoru TASAKA 2006-11-06 05:13:01 UTC
Note: I will be busy till this Thursday (in Japan: EST+14h) and I may not
be able to check your srpm by that day.

Comment 14 Mamoru TASAKA 2006-11-11 15:57:35 UTC
Now I am alive and please submit a new srpm when you
create it.

Comment 15 Chris Petersen 2006-11-15 04:37:26 UTC
Updated again

http://rpm.forevermore.net/dar/dar-2.3.1-3.src.rpm
http://rpm.forevermore.net/dar/dar.spec

- Fix/standardize Requires/Provides for libdar and libdar-devel
- Remove redundant zlib-devel (covered by openssl-devel)
- Update README.Fedora with my name/date, as requested in the ticket
- Add a patch to fix a funky character in man/dar.1


Comment 16 Mamoru TASAKA 2006-11-15 07:14:42 UTC
Okay.

----------------------------------------------
  This package (dar) is APPROVED by me.

Comment 17 Chris Petersen 2007-05-28 20:19:58 UTC
Package Change Request
======================
Package Name: dar
New Branches: EL-5 F-7

Comment 18 Kevin Fenzi 2007-06-22 19:46:39 UTC
cvs done. 
Note that this package already has a F-7 branch. 


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