Bug 225667 - Merge Review: cryptsetup-luks
Merge Review: cryptsetup-luks
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 12:53 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-30 07:11:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 12:53:59 EST
Fedora Merge Review: cryptsetup-luks

http://cvs.fedora.redhat.com/viewcvs/devel/cryptsetup-luks/
Initial Owner: pjones@redhat.com
Comment 1 Aurelien Bompard 2007-02-28 09:20:45 EST
Just a few things I noticed : in the packaged %doc, there's a generic autotools
INSTALL file, and two empty files (NEWS and README).
Those three files should not be included in %doc, they're useless.
Comment 2 Kevin Fenzi 2007-03-06 00:15:18 EST
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 (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
e134b82b4706a28ba1d73b9176d5ad0c  cryptsetup-luks-1.0.3.tar.bz2
e134b82b4706a28ba1d73b9176d5ad0c  cryptsetup-luks-1.0.3.tar.bz2.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.

OK - Headers/static libs in -devel subpackage.
OK - Spec has needed ldconfig in post and postun
OK - .so files in -devel subpackage.
See below - -devel package Requires: %{name} = %{version}-%{release}
OK - .la files are removed.

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.
OK - final provides and requires are sane

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
See below - Should have subpackages require base package with fully versioned
depend.
OK - Should have dist tag
See below - Should package latest version
8 outstanding bugs - check for outstanding bugs on package.  

Issues:

1. Might have the devel subpackage
Requires: %{name} = %{version}-%{release}
instead of just
Requires: %{name} = %{version}

2. Ditto the docs comments from comment #2.

3. rpmlint says:
E: cryptsetup-luks statically-linked-binary /sbin/cryptsetup

Needs to be static per the bug mentioned in the changelog.

E: cryptsetup-luks zero-length /usr/share/doc/cryptsetup-luks-1.0.3/NEWS
E: cryptsetup-luks zero-length /usr/share/doc/cryptsetup-luks-1.0.3/README

Should be removed, per issue 2.

W: cryptsetup-luks-devel no-documentation

Can be ignored.

4. 1.0.4 is out, perhaps upgrade to that?

5. Should the static lib be shipped in the devel package here?
Is there anything that uses it?

6. 8 outstanding bugs. Might look at those. I think at least one should
be solved by upgrading to 1.0.4, possibly more.
Comment 3 Marcin Garski 2007-06-09 15:48:12 EDT
Now we have 1.0.5 release, where cryptsetup-luks becomes cryptsetup.

If you upgrade to this release bug #215199 and bug #217983 can be resolved.
Comment 4 Till Maas 2007-08-17 08:34:07 EDT
(In reply to comment #2)
> 1. Might have the devel subpackage
> Requires: %{name} = %{version}-%{release}
> instead of just
> Requires: %{name} = %{version}

committed in devel
 
> 2. Ditto the docs comments from comment #2.

commited in devel

> 3. rpmlint says:
> E: cryptsetup-luks statically-linked-binary /sbin/cryptsetup
> 
> Needs to be static per the bug mentioned in the changelog.

not anymore, it is build dynamically in devel

> E: cryptsetup-luks zero-length /usr/share/doc/cryptsetup-luks-1.0.3/NEWS
> E: cryptsetup-luks zero-length /usr/share/doc/cryptsetup-luks-1.0.3/README
> 
> Should be removed, per issue 2

commited in devel

> Can be ignored.
> 
> 4. 1.0.4 is out, perhaps upgrade to that?

Updated to 1.0.5 in devel
 
> 5. Should the static lib be shipped in the devel package here?
> Is there anything that uses it?

Static stuff has been removed in devel.

> 6. 8 outstanding bugs. Might look at those. I think at least one should
> be solved by upgrading to 1.0.4, possibly more.
 
done.
Comment 5 Kevin Fenzi 2007-08-28 19:25:40 EDT
Sorry for the delay, I've been swamped of late. 

I promise to try and finish this up this week... 
Comment 6 Marcin Garski 2007-08-29 17:31:21 EDT
FYI:
1. URL has changed to http://luks.endorphin.org/
2. License is GPLv2
3. ChangeLog needs to be recoded to UTF-8
Comment 7 Till Maas 2007-08-29 17:57:12 EDT
(In reply to comment #6)

> 1. URL has changed to http://luks.endorphin.org/
> 2. License is GPLv2
> 3. ChangeLog needs to be recoded to UTF-8

changed in cvs for devel
Comment 8 Kevin Fenzi 2007-08-29 22:11:47 EDT
Sorry again for the delay. 

Minor nitpick: any reason for not using %{?_smp_mflags} on your make ?

Do you plan to rename this over to cryptsetup? 

I see no further blockers, so this package is APPROVED. 
Feel free to close this rawhide. 
Comment 9 Till Maas 2007-08-30 07:11:25 EDT
(In reply to comment #8)
> Sorry again for the delay. 

No problem, thank you for your review.

> Minor nitpick: any reason for not using %{?_smp_mflags} on your make ?

I added it to cvs for devel and a scratch build worked fine, so I will keep it.
 
> Do you plan to rename this over to cryptsetup? 

Yes, I already asked how to do this but got no satisfactory reply:
https://www.redhat.com/archives/fedora-maintainers/2007-August/msg00419.html
The main issues are that there is already a cryptsetup Bugzilla component and
that I do not know, whether someone will really take care, that the
cryptsetup-luks and cryptsetup component in Bugzilla are merged. Also there
seems no way to move the acls in the package db yet. For me it is no a high
priority task to rename the package, therefore I want to wait until Fedora's
infrastructure really support renaming packages.

> I see no further blockers, so this package is APPROVED. 
> Feel free to close this rawhide. 

Thank you again. :-)
Comment 10 Kevin Fenzi 2007-08-30 12:59:46 EDT
Well, at least for now, we have decided to do package renames as follows: 

- Create the new package name. 
- Follow the end of life package guidelines on the old package in the branches
where it is being replaced. 
- Maintainer imports the package into the new name (with Obsoletes/Provides if
needed). 

The cvs history is maintained in the old package's cvs. 
Comment 11 Till Maas 2007-08-30 13:18:59 EDT
(In reply to comment #10)
> Well, at least for now, we have decided to do package renames as follows: 
> 
> - Create the new package name. 

How is this done? Just rebuild cryptsetup-luks with name cryptsetup and a
cryptsetup.spec?

And are you sure that this will not break anything because there is already a
cryptsetup component in Bugzilla? (There was a cryptsetup packege in FC 2 and 3)
according to http://cvs.fedora.redhat.com/viewcvs/rpms/cryptsetup/?root=core

And do I have to manually copy the acls from PackageDB into a fedora-cvs request
template?
Comment 12 Kevin Fenzi 2007-08-31 23:10:28 EDT
>How is this done? Just rebuild cryptsetup-luks with name cryptsetup and a
>cryptsetup.spec?

Well, you need to do a cvsrequest to make the new module, but then yes, you just
import the new name and spec. 

>And are you sure that this will not break anything because there is already a
>cryptsetup component in Bugzilla? (There was a cryptsetup packege in FC 2 and 3)
>according to http://cvs.fedora.redhat.com/viewcvs/rpms/cryptsetup/?root=core

Well, I don't think so since they are so old... are there any open bugs on that
name? You might have to deal with them... 

>And do I have to manually copy the acls from PackageDB into a fedora-cvs
>request template?

Yeah, at least as far as I know. I can ask if perhaps someone could copy it over
in the database or something if it's difficult to request. 
Comment 13 Till Maas 2007-09-01 04:29:47 EDT
(In reply to comment #12)

> Well, I don't think so since they are so old... are there any open bugs on that
> name? You might have to deal with them... 

There was one open bug which was meant to be filed against cryptsetup-luks and I
guess the reporter was confused with the two components. I only noticed it by
coincidence. This seems not to be well handled, two. E.g. there seems no way to
become initialCC of cryptsetup now, because it is not in the PackageDB. I guess
the same problem will occur the other way round when cryptsetup-luks will be
renamed.

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