Bug 1010000 - Review Request: devscripts - Scripts for Debian Package maintainers
Summary: Review Request: devscripts - Scripts for Debian Package maintainers
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Sergio Basto
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 920163 1009999
Blocks: 969718
TreeView+ depends on / blocked
 
Reported: 2013-09-19 16:36 UTC by Sandro Mani
Modified: 2013-11-14 09:44 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-11-14 09:44:35 UTC
Type: ---
sergio: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Sandro Mani 2013-09-19 16:36:41 UTC
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

Comment 1 Sandro Mani 2013-09-19 17:25:01 UTC
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@gmail.com> - 2.13.3-2
- Require rpmdevtools and drop scripts which are in rpmdevtools

Comment 2 Sandro Mani 2013-09-20 09:34:00 UTC
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@gmail.com> - 2.13.3-3
- Revert: Require rpmdevtools and drop scripts which are in rpmdevtools
- Add conflicts from rpmdevtools < 8.3-6

Comment 3 Sandro Mani 2013-09-20 09:53:03 UTC
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@gmail.com> - 2.13.3-4
- Conflict with rpmdevtools < 8.4

Comment 4 Ralf Corsepius 2013-09-20 17:43:34 UTC
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

Comment 5 Sandro Mani 2013-09-20 23:22:26 UTC
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@gmail.com> - 2.13.3-5
- Fix typo builroot -> buildroot
- Require perl modules instead of the providing packages

Comment 6 Dridi Boukelmoune 2013-09-21 09:31:09 UTC
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 ?

Comment 7 Sandro Mani 2013-09-21 18:12:32 UTC
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

Comment 8 Dridi Boukelmoune 2013-09-22 12:38:24 UTC
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.

Comment 9 Ralf Corsepius 2013-09-22 14:40:18 UTC
(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.

Comment 10 Sandro Mani 2013-09-22 16:00:02 UTC
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.

Comment 11 Ralf Corsepius 2013-09-22 17:06:06 UTC
(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.

Comment 12 Sandro Mani 2013-09-22 18:32:49 UTC
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.

Comment 13 Ville Skyttä 2013-09-23 07:40:01 UTC
(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.

Comment 14 Ralf Corsepius 2013-09-23 08:02:33 UTC
(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.

Comment 15 Ville Skyttä 2013-09-23 08:49:43 UTC
(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"

Comment 16 Ralf Corsepius 2013-09-23 12:38:56 UTC
(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.

Comment 17 Sandro Mani 2013-09-28 20:26:08 UTC
perl-Parse-DebControl is now available, scratch build of devscripts here: http://koji.fedoraproject.org/koji/taskinfo?taskID=5998608

Comment 18 Sergio Basto 2013-10-01 02:55:43 UTC
(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.

Comment 19 Ville Skyttä 2013-10-01 07:04:01 UTC
(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.

Comment 20 Sandro Mani 2013-10-01 07:44:13 UTC
If the review gets completed rapidly, I'd include F20 also. Otherwise, F21+ I guess.

Comment 21 Dridi Boukelmoune 2013-10-01 09:03:51 UTC
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 ?

Comment 22 Sandro Mani 2013-10-01 09:07:58 UTC
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.

Comment 23 Sandro Mani 2013-10-05 20:38:42 UTC
Any chance of finishing this review?

Comment 24 Sergio Basto 2013-10-07 00:58:45 UTC
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

Comment 25 Sergio Basto 2013-10-07 01:02:57 UTC
(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 ?

Comment 26 Sandro Mani 2013-10-07 01:11:17 UTC
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@gmail.com> - 2.13.4-1
- Update to 2.13.4
- Drop devscripts_item.patch
- Drop devscripts_spurious-pod.patch

Comment 27 Sergio Basto 2013-10-07 01:38:13 UTC
Package APPROVED ! 

Dridi I hope you don't mind , I think we should move on ... 

Sandro you may do next step SCM requests 

Thanks,

Comment 28 Sandro Mani 2013-10-07 01:39:25 UTC
Thanks Sergio!

New Package SCM Request
=======================
Package Name: devscripts
Short Description: Scripts for Debian Package maintainers
Owners: smani
Branches: f19 f20
InitialCC:

Comment 29 Ville Skyttä 2013-10-07 06:59:51 UTC
(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)?

Comment 30 Sandro Mani 2013-10-07 08:53:31 UTC
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:

Comment 31 Gwyn Ciesla 2013-10-07 12:13:22 UTC
Git done (by process-git-requests).

Comment 32 Sandro Mani 2013-10-07 17:13:48 UTC
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?

Comment 33 Ville Skyttä 2013-10-07 20:14:43 UTC
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.

Comment 34 Fedora Update System 2013-10-07 21:40:04 UTC
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

Comment 35 Fedora Update System 2013-10-09 14:16:09 UTC
rpmdevtools-8.4-1.fc20, devscripts-2.13.4-2.fc20 has been pushed to the Fedora 20 testing repository.


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