[Pkg-xen-devel] Xen 4.1.1 packaging enhancement

Thu Aug 18 09:53:32 UTC 2011

Hi Ian,

Thanks a lot for the below review.

On 08/17/2011 06:03 PM, Ian Campbell wrote:
> I think it would be better to send each change as a separate patch, that
> would allow waldi to cherry pick the non-controversial ones more easily
> without blocking on anything which he doesn't like. It also make review
> easier and fits under the mailing list limits without compression (which
> is another barrier to review) etc.

Ok, I will do that.

>> Currently, Xen seems to be maintained using SVN. Would it be possible to
>> move it to Git? I know nothing about SVN, I've switched directly from
>> CVS to Git few years ago, and I never learned SVN. I don't wish to learn
>> it, as I know it's something of the past.
> 
> Well, I think it's a bit premature to start asking a maintainer to
> switch his working practices to suit you -- imagine if everyone
> requested that everyone else change to their preferred VCS!

That's not my intention, sorry if I didn't write it correctly. My
intention is to use Git for the new packages, like libblktap,
blktap-dkms, xen-api-libs, xen-api and so on. As much as I understand,
the only way is to change the Alioth project to Git, and I'm not admin...

> As it happens waldi is driving the kernel team's transition from svn to
> git so I wouldn't be surprised if pkg-xen eventually follows. In the
> meantime I suggest you checkout the git-svn package which lets you
> checkout an svn repo into a local git tree.

Ok.

>> diff -r -u old/xen-4.1.1/debian/control new/xen-4.1.1/debian/control
>> [...]
>> + package containst the runtime OCaml library.
>                     ^oops
> 
> The ocaml libraries supplied by Xen have much to do with the Xen
> Management API as such, other than happening to be underlying libraries
> which xapi uses.

Thanks for spotting that one, I'll fix it.

>>  Description: Xenstore communications library for Xen
>> + XenStore is a database, hosted by domain 0,
> 
> Not necessarily dom0 -- it is possible to run xenstore in another domain
> (although only really prototypes of this have been done). </pendant>

Can you provide a better wording then? Because currently, we have a
single line long description, which isn't good for neither my taste and
the one of Lintian. :)

>> @@ -64,6 +105,9 @@
>>   In order to boot a XEN system along with this package you also need a
>>   kernel specifically crafted to work as the Domain 0, mediating hardware
>>   access for XEN itself.
>> + .
>> + This package is for AMD64 hardware.  If your computer supports amd64 arch,
>> + then it's always best to use it.
> 
> "it" == the amd64 flavour i386.deb package? Better to be explicit about
> that I think.

I'll fix that too.

>> diff -r -u old/xen-4.1.1/debian/patches/series new/xen-4.1.1/debian/patches/series
>> --- old/xen-4.1.1/debian/patches/series	2011-08-05 21:58:31.000000000 +0000
>> +++ new/xen-4.1.1/debian/patches/series	2011-08-08 04:06:20.000000000 +0000
>> @@ -54,3 +54,6 @@
>>  tools-xenstore-compatibility.diff
>>  upstream-23044:d4ca456c0c25
>>  upstream-23104:1976adbf2b80
>> +
>> +activate-qemu-options.diff
>> +spelling-typo.diff
> 
> I didn't see these patches actually added under debian/patches in this patch?

They were, but I didn't use the -N option of diff, which is why you
didn't see it. I'll send multiple new patches as you suggested.

>> diff -r -u old/xen-4.1.1/debian/patches/tools-libxl-prefix.diff new/xen-4.1.1/debian/patches/tools-libxl-prefix.diff
>> --- old/xen-4.1.1/debian/patches/tools-libxl-prefix.diff	2011-03-16 16:18:07.000000000 +0000
>> +++ new/xen-4.1.1/debian/patches/tools-libxl-prefix.diff	2011-08-07 01:12:35.000000000 +0000
>> @@ -1,3 +1,6 @@
>> +Description: Fixes libxenlight install.
> 
> I'm not sure that adding this level of comment actually adds anything
> which isn't immediately obvious from the patch itself. (generally true
> of all the ones I trimmed too).

Reading this one line of comment is faster than reading the full diff. I
was also hoping for authors of the patches to re-write better things.

>> +Forwarded: not-needed
>> +Author: Unkown for the moment.
> 
> "Unknown".
> 
> I think the "for the moment" is a bit optimistic too ;-)

That's a problem, really. How come we have patches without authors?
Shouldn't waldi know who did what? Maybe he did most of them?

>> +install-lib-ocaml_$(ARCH): DIR = $(BUILD_DIR)/install-utils_$(ARCH)
>> +install-lib-ocaml_$(ARCH): PACKAGE_NAME = libxen-ocaml
>> +install-lib-ocaml_$(ARCH): DH_OPTIONS = -p$(PACKAGE_NAME)
>> +install-lib-ocaml_$(ARCH): $(STAMPS_DIR)/install-utils_$(ARCH)
>> +	dh_testdir
>> +	dh_testroot
>> +	dh_prep
>> +	install -D -m 0644 $(BUILD_DIR)/install-utils_$(ARCH)/usr/lib/ocaml/eventchn/dlleventchn_stubs.so $(CURDIR)/debian/$(PACKAGE_NAME)/$(OCAML_DLL_DIR)/dlleventchn_stubs.so
>> +	install -D -m 0644 $(BUILD_DIR)/install-utils_$(ARCH)/usr/lib/ocaml/mmap/dllmmap_stubs.so $(CURDIR)/debian/$(PACKAGE_NAME)/$(OCAML_DLL_DIR)/dllmmap_stubs.so
>> +	install -D -m 0644 $(BUILD_DIR)/install-utils_$(ARCH)/usr/lib/ocaml/xb/dllxb_stubs.so $(CURDIR)/debian/$(PACKAGE_NAME)/$(OCAML_DLL_DIR)/dllxb_stubs.so
>> +	install -D -m 0644 $(BUILD_DIR)/install-utils_$(ARCH)/usr/lib/ocaml/xc/dllxc_stubs.so $(CURDIR)/debian/$(PACKAGE_NAME)/$(OCAML_DLL_DIR)/dllxc_stubs.so
>> +	install -D -m 0644 $(BUILD_DIR)/install-utils_$(ARCH)/usr/lib/ocaml/xl/dllxl_stubs.so $(CURDIR)/debian/$(PACKAGE_NAME)/$(OCAML_DLL_DIR)/dllxl_stubs.so
> 
> All these open-coded install lines (and the many which follow) can't be
> right. Why can't dh_install be used for this stuff?

The point is to use $(OCAML_DLL_DIR) from /usr/share/ocaml/ocamlvars.mk,
which dh_install can't use. But it's wrong as well, best would be to use
"ocamlfind install". I'll try working on that.

>> @@ -157,6 +228,7 @@
>>  	install -D -m644 debian/xen-utils.README.Debian $(PACKAGE_DIR)/usr/share/doc/$(PACKAGE_NAME)/README.Debian
>>  	install -D -m644 debian/xen-utils-$(VERSION).lintian-overrides $(PACKAGE_DIR)/usr/share/lintian/overrides/$(PACKAGE_NAME)
>>  	dh_install --sourcedir=$(DIR) usr/lib/xen-$(VERSION)
>> +	strip -s --remove-section=.note --remove-section=.comment $(PACKAGE_DIR)/usr/lib/xen-$(VERSION)/boot/hvmloader
> 
> What is this for?

Isn't that obvious? Stripping hvmloader which was *not* stripped as
reported by lintian... I was expecting that dh_stirp would do it, but
it's not. It's a bit awkward to do it this way, but it works! :)

>> diff -r -u old/xen-4.1.1/qemu/audio/alsaaudio.c new/xen-4.1.1/qemu/audio/alsaaudio.c
>> --- old/xen-4.1.1/qemu/audio/alsaaudio.c	2011-04-28 07:38:36.000000000 +0000
>> +++ new/xen-4.1.1/qemu/audio/alsaaudio.c	2011-08-07 07:40:06.000000000 +0000
>> @@ -475,7 +475,7 @@
>>          (obt->fmt != req->fmt ||
>>           obt->nchannels != req->nchannels ||
>>           obt->freq != req->freq)) {
>> -        dolog ("Audio paramters for %s\n", typ);
>> +        dolog ("Audio parameters for %s\n", typ);
>>          alsa_dump_info (req, obt);
>>      }
>>
> 
> Should this (and all the following changes to the qemu subtree) be
> patches under debian/patches instead of applied directly?

Same as above. It WAS patched in debian/patches, but I didn't add the -N
option to diff, and new files didn't show up.

>> +LIBS += -lpulse -lSDL
> 
> Should rather be a fix to the configure script?
> 
>>  BAD_OBJS += gdbstub.o acpi.o apic.o
>>  BAD_OBJS += vmmouse.o vmport.o tcg* helper.o vmware_vga.o virtio-balloon.o
>>  
>> diff -r -u old/xen-4.1.1/tools/Makefile new/xen-4.1.1/tools/Makefile
>> --- old/xen-4.1.1/tools/Makefile	2011-08-07 01:02:50.000000000 +0000
>> +++ new/xen-4.1.1/tools/Makefile	2011-08-07 07:40:06.000000000 +0000
>> @@ -109,7 +109,7 @@
>>  		$(absolutify_xen_root); \
>>  		$(buildmakevars2shellvars); \
>>  		cd ioemu-dir; \
>> -		$(QEMU_ROOT)/xen-setup $(IOEMU_CONFIGURE_CROSS)
>> +		$(QEMU_ROOT)/xen-setup $(IOEMU_CONFIGURE_CROSS) --audio-drv-list="pa oss alsa sdl esd" --audio-card-list="ac97 es1370 sb16 cs4231a adlib gus" --enable-mixemu
> 
> It'd be better to add a IOEMU_CONFIG_OPTS variable to the top-level
> Config.mk and use it in the packaging. That sort of patch could be
> upstreamed I think.
> 
> Ian.

Can you explain better how to do that? Really, we do need the audio and
SDL options, so it'd be nice to get to know how to do it the best way.

Thomas