Spec URL: http://smani.fedorapeople.org/review/devscripts.spec SRPM URL: http://smani.fedorapeople.org/review/devscripts-2.13.3-1.fc21.src.rpm Description: Scripts for Debian Package maintainers Fedora Account System Username: smani
Spec URL: http://smani.fedorapeople.org/review/devscripts.spec SRPM URL: http://smani.fedorapeople.org/review/devscripts-2.13.3-2.fc21.src.rpm * Thu Sep 19 2013 Sandro Mani <manisandro> - 2.13.3-2 - Require rpmdevtools and drop scripts which are in rpmdevtools
Spec URL: http://smani.fedorapeople.org/review/devscripts.spec SRPM URL: http://smani.fedorapeople.org/review/devscripts-2.13.3-3.fc21.src.rpm * Fri Sep 20 2013 Sandro Mani <manisandro> - 2.13.3-3 - Revert: Require rpmdevtools and drop scripts which are in rpmdevtools - Add conflicts from rpmdevtools < 8.3-6
Spec URL: http://smani.fedorapeople.org/review/devscripts.spec SRPM URL: http://smani.fedorapeople.org/review/devscripts-2.13.3-4.fc21.src.rpm * Fri Sep 20 2013 Sandro Mani <manisandro> - 2.13.3-4 - Conflict with rpmdevtools < 8.4
Not a formal review, just some remarks based on brief visual inspection of your spec: - You should not BR: these: BuildRequires: perl-DB_File BuildRequires: perl-File-DesktopEntry BuildRequires: perl-Parse-DebControl BuildRequires: perl-Pod-Checker but BR: the perl modules which are being used (i.e. use BR: perl(...)) - BR: dpkg-perl I am haven't looked into details, but I'd assume this package doesn't actually require "dpkg-perl" but some perl modules provided by dpkg-perl. I.e. the same consideration as above also applies here. - A typo ("builroot", note the missing "d"): rm -rf %{builroot}%{_datadir}/doc
Thanks Ralf Spec URL: http://smani.fedorapeople.org/review/devscripts.spec SRPM URL: http://smani.fedorapeople.org/review/devscripts-2.13.3-5.fc21.src.rpm * Sat Sep 21 2013 Sandro Mani <manisandro> - 2.13.3-5 - Fix typo builroot -> buildroot - Require perl modules instead of the providing packages
I have a question about the sed command: ``` # Search for libvfork in %%{_libdir}/%%{name} sed -i 's|/usr/lib/devscripts/libvfork.so.0|%{_libdir}/%{name}/libvfork.so.0|g' scripts/dpkg-depcheck.pl ``` Is this package multilib-capable ? I wonder whether the LIBDIR in the Makefile is meant to be changeable to /usr/lib64 or just to /usr/local/lib. With a `grep -F /usr/lib/ -r devscripts-2.13.3/` I can see that apparently dpkg either strictly uses /usr/lib (like systemd) or you should also patch `debchange.pl'. Which brings me to the next question, why sed instead of a patch wrapped in an %if statement ? With the same grep command, I've spotted a deb `postinst' script. There's also a `postrm'. Shouldn't you have similar %post and %postun scriptlets in your spec ?
Debian only uses /usr/lib (that is, before multiarch[1]), so the reason /usr/lib is used is probably because the package targets debian. On Fedora OTOH we have /usr/lib64, so I'd say that is where the shared library should go. As far as debchange.pl is concerned, /usr/lib appears only in a comment, so hardly worth patching it. Sed vs patch + %if: I guess when a patch should be preferred over a shell command in the spec is a matter of taste for very small changes. In this case I feel that a sed command is simpler and easier to maintain (i.e. much like adjusting line endings is also done with shell commands). The postinst and postrm scripts: - postinst upgrades /etc/devscripts.conf with new options added with new releases of the package. I think in Fedora we usually just ship the newest default config file, specify the config files as %config(noreplace), which then causes rpm to install the files with the .rpmnew suffix, and it is up to the user to adapt the old config file. So postinst in unnecessary. - postrm: this removes the config file when --purge is passed to apt-get. rpm does not have purge, but automatically saves the config files to .rpmsave. So this scriplet also is unnecessary. [1] https://wiki.debian.org/Multiarch
I've tried a review (with fedora-review) which obviously failed due to bug 1009999. You could also use the %{name} macro in both the replace and search pattern in the sed command. The package looks fine, I'll review it once perl-Parse-DebControl is available.
(In reply to Dridi Boukelmoune from comment #8) > I've tried a review (with fedora-review) which obviously failed due to bug > 1009999. My view: This "Conflicts:" renders it impossible to install this package. => This package does not comply to one of basic requirements of a package review. In other words: Instead of letting this package Conflict, I'd suggest to change this package shall be changed in such a way it (at least temporarily) uses tools provided by rpmdevtools (AFAICT, they are compatible) and switch the deps later for those Fedora release which switch to a newer rpmdevtools. Another surplus of this step would be, devscripts could also be provided for older Fedora releases, for which switching to rpmdevtools >= 8.4 would be an arguable idea.
How does the conflict render the package non-installable? From my test (using a rpmdevtools-8.4 with the conflicting files removed), everything works as expected.
(In reply to Sandro Mani from comment #10) > How does the conflict render the package non-installable? All versions of Fedora provide rpmdevtools-8.3: ./development/rawhide/source/SRPMS/r/rpmdevtools-8.3-5.fc20.src.rpm ./development/rawhide/x86_64/os/Packages/r/rpmdevtools-8.3-5.fc20.noarch.rpm ./development/rawhide/i386/os/Packages/r/rpmdevtools-8.3-5.fc20.noarch.rpm ./development/20/source/SRPMS/r/rpmdevtools-8.3-5.fc20.src.rpm ./development/20/x86_64/os/Packages/r/rpmdevtools-8.3-5.fc20.noarch.rpm ./development/20/i386/os/Packages/r/rpmdevtools-8.3-5.fc20.noarch.rpm ./releases/18/Everything/source/SRPMS/r/rpmdevtools-8.3-1.fc18.src.rpm ./releases/18/Everything/x86_64/os/Packages/r/rpmdevtools-8.3-1.fc18.noarch.rpm ./releases/18/Everything/i386/os/Packages/r/rpmdevtools-8.3-1.fc18.noarch.rpm ./releases/19/Everything/source/SRPMS/r/rpmdevtools-8.3-3.fc19.src.rpm ./releases/19/Everything/x86_64/os/Packages/r/rpmdevtools-8.3-3.fc19.noarch.rpm ./releases/19/Everything/i386/os/Packages/r/rpmdevtools-8.3-3.fc19.noarch.rpm > From my test > (using a rpmdevtools-8.4 with the conflicting files removed), everything > works as expected. Exactly this is the point: There currently are not rpmdevtools-8.4 packages anywhere in Fedora, ie. your package-as-is cannot be installed on any Fedora.
Ah ok I see what you mean. So basically discussing this in bug #920163, Ville Skyttä suggested I notified him as soon as the review is complete, and that he'll take care of pushing rpmdevtools-8.4. Note that I'm open to other suggestions on how to proceed, but maybe it's worth mentioning those in bug #920163 so that we can hear Ville's opinion also.
(In reply to Sandro Mani from comment #12) > but maybe it's worth mentioning those in bug > #920163 so that we can hear Ville's opinion also. No need, I'm following this review. I still stick with my original opinion (conflicts + new rpmdevtools pulling this in for the time being), I think it's a good and simple enough approach. I don't intend to introduce any changes to rpmdevtools 8.4 that would make upgrading to it questionable in current Fedora releases so IMO that point is moot.
(In reply to Ville Skyttä from comment #13) > I still stick with my original opinion > (conflicts + new rpmdevtools pulling this in for the time being), I think > it's a good and simple enough approach. Well, then let me be more explicit and directly: It's impossible for reviewers to have tested this devscripts package due to the conflict. All reviewers who claim to have done so, must be cheating (e.g. by having packages from outside of Fedora installed) or lying (e.g. by not have tested this package at all). > I don't intend to introduce any > changes to rpmdevtools 8.4 that would make upgrading to it questionable in > current Fedora releases so IMO that point is moot. Isn't this what you are are doing? AFAIU, you want to introduce an rpmdevtools with the shared tools removed on Fedora < 21. This means a fundamental CLI and feature change to rpmdevtools. IMO, this not necessarily a wise move.
(In reply to Ralf Corsepius from comment #14) > It's impossible for reviewers to have tested this devscripts package due to > the conflict. No, it's not. "rpm -e rpmdevtools ; rpm -i devscripts"
(In reply to Ville Skyttä from comment #15) > (In reply to Ralf Corsepius from comment #14) > > It's impossible for reviewers to have tested this devscripts package due to > > the conflict. > > No, it's not. "rpm -e rpmdevtools ; rpm -i devscripts" Feel free to continue to review this package. - I definitely will not break my installations to test for the bugs of some very possibly half-broken Debian tools and definitely will not continue this review. I have a local Debian package building infrastructure based on older Debian tools and I know these were affected by subtile bugs.
perl-Parse-DebControl is now available, scratch build of devscripts here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5998608
(In reply to Ralf Corsepius from comment #11) > (In reply to Sandro Mani from comment #10) > > (using a rpmdevtools-8.4 with the conflicting files removed), everything > > works as expected. > Exactly this is the point: There currently are not rpmdevtools-8.4 packages > anywhere in Fedora, ie. your package-as-is cannot be installed on any Fedora. That is a good new, know that conflicting files have been removed when they build rpmdevtools-8.4 for F21 (In reply to Ralf Corsepius from comment #16) > > No, it's not. "rpm -e rpmdevtools ; rpm -i devscripts" > > Feel free to continue to review this package. - I definitely will not break > my installations to test for the bugs of some very possibly half-broken > Debian tools and definitely will not continue this review. > > I have a local Debian package building infrastructure based on older Debian > tools and I know these were affected by subtile bugs. why you don't install rpmdevtools-8.4 , definitely devscripts will require rpmdevtools-8.4 to enter in Fedora.
(In reply to Sergio Monteiro Basto from comment #18) > why you don't install rpmdevtools-8.4 Note that it doesn't exist yet. I'm planning to release it soon though. BTW what distro versions do you plan to ship devscripts for? Due to the required rpmdevtools packaging changes I recommend F-21+ only, or additionally F-20 if you can get it in before the F-20 release.
If the review gets completed rapidly, I'd include F20 also. Otherwise, F21+ I guess.
Ok then, I'll retry a full review ASAP :) But there's one thing I don't understand. If we want this package in f20, since it's a new package, we only need rpmdevtools to be updated to the right version in f20, don't we ? Once all dependencies are available, a new package can arrive at any time in a Fedora release ?
Yes, it can arrive at any time, but it is usually better to do more complex changes in unreleased versions to avoid surprises to users running stable releases.
Any chance of finishing this review?
I check the package all seems fine and ready to be approved, but version devscripts_2.13.3.tar.xz has disappear from debian repos [1]. Should we update the version of devscripts in this review ? [1] devscripts.src: W: invalid-url Source0: http://ftp.debian.org/debian/pool/main/d/devscripts/devscripts_2.13.3.tar.xz HTTP Error 404: Not Found
(In reply to Dridi Boukelmoune from comment #21) > Ok then, I'll retry a full review ASAP :) > > But there's one thing I don't understand. If we want this package in f20, > since it's a new package, we only need rpmdevtools to be updated to the > right version in f20, don't we ? Once all dependencies are available, a new > package can arrive at any time in a Fedora release ? if rpmdevtools-8.4 lands in F20 , we can also add devscripts to F20. I don't see any problem. Dridi , could you finish this review , or may I take over this review for approve it ?
The debian ftp only stores the latest version. Suboptimal, but I cannot find a more stable SOURCE URL... Spec URL: http://smani.fedorapeople.org/review/devscripts.spec SRPM URL: http://smani.fedorapeople.org/review/devscripts-2.13.4-1.fc21.src.rpm * Mon Oct 07 2013 Sandro Mani <manisandro> - 2.13.4-1 - Update to 2.13.4 - Drop devscripts_item.patch - Drop devscripts_spurious-pod.patch
Package APPROVED ! Dridi I hope you don't mind , I think we should move on ... Sandro you may do next step SCM requests Thanks,
Thanks Sergio! New Package SCM Request ======================= Package Name: devscripts Short Description: Scripts for Debian Package maintainers Owners: smani Branches: f19 f20 InitialCC:
(In reply to Sandro Mani from comment #28) > Branches: f19 f20 Sandro, what happened to comment 20, please confirm that you are really planning to push this for F-19 too (I hope not)?
Oh gasp, that was not my intention. Thanks for catching this! New Package SCM Request ======================= Package Name: devscripts Short Description: Scripts for Debian Package maintainers Owners: smani Branches: f20 InitialCC:
Git done (by process-git-requests).
Ville, I've now imported the SRPM into the git repo. How shall we coordinate the devscipts + rpmdevtools push? Since you are a provenpackager, a simple solution might be that you do a chain-build of devscripts and the new rpmdevtools?
Will look into it, rolling rpmdevtools 8.4 as I write this. I suppose a chain build isn't needed, but simply a single F-20 update that includes both this package and the new rpmdevtools.
rpmdevtools-8.4-1.fc20,devscripts-2.13.4-2.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/rpmdevtools-8.4-1.fc20,devscripts-2.13.4-2.fc20
rpmdevtools-8.4-1.fc20, devscripts-2.13.4-2.fc20 has been pushed to the Fedora 20 testing repository.