Fedora Merge Review: cryptsetup-luks http://cvs.fedora.redhat.com/viewcvs/devel/cryptsetup-luks/ Initial Owner: pjones
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.
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.
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.
(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.
Sorry for the delay, I've been swamped of late. I promise to try and finish this up this week...
FYI: 1. URL has changed to http://luks.endorphin.org/ 2. License is GPLv2 3. ChangeLog needs to be recoded to UTF-8
(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
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.
(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. :-)
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.
(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?
>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.
(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.