Ubuntu Wiki

Dev Week -- Learning from mistakes - REVU reviewing best practices -- mok0 -- Thu Sep 3rd, 2009

UTC

(02:04:53 PM) mok0: OK, so this class is "Learning from mistakes - REVU reviewing best practices"
(02:05:03 PM) mok0: Can we have a count of hands, please?
(02:09:00 PM) mok0: Please go to #ubuntu-classroom-chat
(02:19:27 PM) mok0: there
(02:19:40 PM) mok0: OK, so we'll carry on here, sorry for the confusion
(02:19:50 PM) AntoineLeclair: are you at http://revu.ubuntuwire.com/p/php5 ?
(02:19:54 PM) frandieguez_: yes
(02:20:03 PM) c_korn: yes
(02:20:16 PM) AntoineLeclair: I was asking to mok0, hehe
(02:20:37 PM) mok0: So, let's combine this tutorial with the useful and let uploaders benefit by getting their packages reviewed. Therefore, we will leave our comments once we have reviewed the package(s).
(02:20:54 PM) mok0: I'm open to suggestions :-)
(02:21:24 PM) mok0: It should be a NEW package, i.e. one that hasn't been reviewed before
(02:22:26 PM) c_korn: celtx
(02:22:39 PM) mok0: OK, I found that too...
(02:22:40 PM) frandieguez_: yes celtx
(02:23:01 PM) mok0: The first thing I generally do before spending time on a package, is to check if it's already been uploaded to Debian, or if there's been an ITP bug filed. That indicates that someone else might be working on the package, in which case there might be a conflict of interest and/or a duplicate effort == waste of someones time.
(02:23:23 PM) mok0: So let's look here: http://ftp-master.debian.org/new.html and http://www.de.debian.org/devel/wnpp/being_packaged and just do a search for the package name in the browser.
(02:24:03 PM) mok0: What's the verdict?
(02:24:12 PM) frandieguez_: isn't there
(02:24:24 PM) c_korn: nothing found
(02:24:27 PM) mok0: :)
(02:24:29 PM) ScottTesterman: not found
(02:24:46 PM) mok0: The next thing I do is to du a cursory check if this software can be distributed at all. Otherwise, there's no need spending time on it. We absolutely need a file -- normally called COPYING -- that grants Ubuntu permission to distribute the software. So let's browse down REVU's html pages into the directory and see if this permission is present.
(02:26:10 PM) mok0: Alright, so this code is derived from mozilla it seems
(02:26:17 PM) frandieguez_: yes
(02:26:21 PM) frandieguez_: http://revu.ubuntuwire.com/revu1-incoming/celtx-0908260521/celtx-2.0.1/mozilla/
(02:26:44 PM) ScottTesterman: It's under the CePL
(02:26:51 PM) mok0: There's a file called LICENSE
(02:27:06 PM) mok0: ScottTesterman: GPL?
(02:27:16 PM) mok0: I see Mozilla Public License in there
(02:27:17 PM) frandieguez_: MPL!
(02:27:38 PM) ScottTesterman: Under debian/ there's a copyright file.
(02:27:43 PM) ScottTesterman: It's says CePL.
(02:27:52 PM) ScottTesterman: The Celtix Public License.
(02:28:16 PM) mok0: ScottTesterman: Ah, you're  way ahead of me :-)
(02:28:25 PM) ScottTesterman: Woops, sorry!
(02:28:32 PM) mok0: But let's look at debian/copyright, then
(02:29:53 PM) mok0: Well, this is a complicated license situation
(02:30:01 PM) frandieguez_: of course...
(02:30:33 PM) ScottTesterman: It looks like it's fine to use as long as Ubuntu a) changes the name of the product, and b) rips out all the Celtx logos and names from the product.
(02:30:35 PM) mok0: It is necessary to spend time reading all this stuff to figure out if the copyright file is OK
(02:31:37 PM) mok0: ScottTesterman: Right, so one job for the reviewing is to see that this is done
(02:32:14 PM) mok0: My next step is generally to download the package. I find the link to the relevant .dsc file, right click -> copy the link. Then I move into a terminal, and type "dget -ux " + right-click -> paste.
(02:33:10 PM) mok0: Yuc, it's huge
(02:33:23 PM) mok0: :-)
(02:33:54 PM) mok0: So... have you guys got a pbuilder or something like that?
(02:34:07 PM) frandieguez_: yes
(02:34:18 PM) dinxter_: yep
(02:34:18 PM) frandieguez_: from previous lectures
(02:34:19 PM) ScottTesterman: yes
(02:34:23 PM) AntoineLeclair: same here
(02:34:30 PM) mok0: Cool, so let's see if it builds
(02:34:30 PM) c_korn: yes
(02:35:26 PM) mok0: ... If the build fails, the review is usually quite short :-P
(02:36:43 PM) mok0: As you probably know, a source package is mainly composed of the pristine tarball and a diff.gz file containing the work of the packager. While it is possible for the diff.gz file to patch everything in the source tree, the current paradigm is that nothing outside the debian/ directory must be touched.
(02:37:10 PM) mok0: So, while this is building, let's check to see that nothing else is in the .diff.gz file:
(02:37:10 PM) mok0: lsdiff -z <package>.diff.gz
(02:37:39 PM) c_korn: fail
(02:38:08 PM) mok0: c_korn: you mean the two files in $topdir?
(02:38:24 PM) c_korn: and the files in the mozilla directory
(02:38:34 PM) c_korn: config.log e.g.
(02:38:35 PM) mok0: c_korn: oh yes didn't see those at first
(02:38:42 PM) mok0: tsk tsk tsk
(02:39:06 PM) mok0: I need a volunteer to write the review... ;-)
(02:39:26 PM) mok0: I generally do it in a text file and copy/paste that later into REVU
(02:40:07 PM) mok0: Another thing is that celtx.desktop is duplicated, it's also in debian/
(02:40:11 PM) c_korn: well, I am logged in...
(02:40:43 PM) mok0: c_korn: thanks
(02:40:59 PM) mok0: So, how do you like celtx.1 ?
(02:41:19 PM) c_korn: should propably in debian/ too ?
(02:41:25 PM) c_korn: +be
(02:41:25 PM) mok0: c_korn: yes
(02:41:37 PM) mok0: I am talking about the content of it
(02:41:42 PM) mok0: ?
(02:41:48 PM) mok0: Pretty useless if you ask me.
(02:42:22 PM) c_korn: oh, yes. that too :)
(02:43:01 PM) mok0: I generally refer people to the Linux Man page Howto: http://tldp.org/HOWTO/Man-Page
(02:43:18 PM) mok0: Citation: "The DESCRIPTION section ...eloquently explains why your sequence of 0s and 1s is worth anything at all. Here's where you write down all your knowledge. This is the Hall Of Fame. Win other programmers' and users' admiration by making this section the source of reliable and detailed information. Explain what the arguments are for, the file format, what algorithms do the dirty jobs."
(02:43:59 PM) mok0: The man page is for people wanting a bit more information than is given in the package description
(02:44:42 PM) mok0: Next we do a cursory check of the files in debian/. We need at least five files to be present there: control, changelog, copyright, config and rules. Otherwise, the package won't build!
(02:45:21 PM) c_korn: config ?
(02:45:28 PM) mok0: We are reviewing for karmic+1 at this point, and StandarsdVersion is 3.8.3
(02:45:36 PM) mok0: compat
(02:45:37 PM) mok0: sorry
(02:45:42 PM) c_korn: conok
(02:45:44 PM) c_korn: -con
(02:46:18 PM) c_korn: everything there
(02:46:22 PM) mok0: Yes
(02:46:43 PM) mok0: control looks good
(02:47:03 PM) mok0: except for Standards-Version: 3.8.3
(02:47:24 PM) mok0: What about changelog?
(02:48:07 PM) c_korn: hm, shouldn't the revision be 0ubuntu1 or whatever ?
(02:48:24 PM) mok0: YES!
(02:48:29 PM) frandieguez_: c_korn i think the same
(02:48:38 PM) c_korn: good :)
(02:48:53 PM) mok0: And all changelog entries should be collapsed to 1
(02:48:59 PM) frandieguez_: and there is a lot of no usefull change lines
(02:49:05 PM) frandieguez_: xD
(02:49:34 PM) mok0: And this is important:
(02:50:11 PM) mok0: The changelog should document EVERYTHING the packager has done that makes the package different from upstreams tarball
(02:50:23 PM) mok0: In this case, he has written a manpage
(02:51:16 PM) frandieguez_: mok0 the pattern of file_changed: explication will be great for all the changes
(02:51:21 PM) mok0: Another thing you would normally document in changelog is patches needed to get the thing to compile or customized for Ubuntu
(02:51:31 PM) frandieguez_:  * file_changed: explication
(02:51:50 PM) mok0: frandieguez, what do you mean?
(02:52:22 PM) frandieguez_: the explications of changes on the source, this is better with the lines formated
(02:52:26 PM) frandieguez_:  * file_changed: explication
(02:52:40 PM) mok0: frandieguez, ah, yes
(02:53:09 PM) mok0: So, what about debian/dirs ??
(02:53:24 PM) frandieguez_: pufff, is the default file
(02:53:26 PM) c_korn: looks like the sample
(02:53:37 PM) mok0: yes, it should go
(02:53:39 PM) c_korn: usr/sbin usually isn't touched
(02:53:46 PM) mok0: exactly
(02:54:08 PM) mok0: dirs is only for creating empty dirs that the app needs
(02:54:19 PM) mok0: for example for plugins or something