On Wed, 2011-08-10 at 18:07 +0800, Thomas Goirand wrote:> Hi,
>
> This email is mainly addressed at Waldi (Bastian Blank). I'm resending
it
> since the previous one is waiting for approval due to its size being
> bigger than 40k (I compressed the .diff this time, so it should be ok).
>
> I have worked on enhancing the current packaging of Xen 4.1.1 over the
> last weeks. The main goal was to package the OCaml libraries of Xen as
> separated binary packages, and doing so, I have also reduced a lot the
> output of lintian -Ii checkings. Here's the list of changes:
>
> * Now builds and ships libxen-ocaml and libxen-ocaml-dev.
> * Better long desc (fixes: I: xen source: duplicate-long-description)
> and more generaly reviewed package descriptions (Closes: #466683).
> * Bumped StandardVersion to 3.9.2.
> * Added (very short) manpages for the xenstore binaries.
> * Enabled Qemu audio drivers and SDL support (Closes: #635166).
> * Removed Julien Danjou <acid at debian.org> as per request (Closes:
#590439).
> * Added comments for each Quilt patches.
>
> I would love to see these changes be included and uploaded shortly to
> SID, if it fits. Please let me know ASAP if there's any issue in my
> changes, or if my patch can be applied.
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.
> 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!
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.
> diff -r -u old/xen-4.1.1/debian/control new/xen-4.1.1/debian/control
> --- old/xen-4.1.1/debian/control 2011-08-05 21:59:23.000000000 +0000
> +++ new/xen-4.1.1/debian/control 2011-08-08 04:06:44.000000000 +0000
> @@ -17,11 +17,40 @@
> includes a description interface (both the API, and a nice explanation of
> how XEN works).
>
> +Package: libxen-ocaml
> +Architecture: any
> +Section: ocaml
> +Provides: ${ocaml:Provides}
> +Depends: ocaml-base-nox-${F:OCamlABI}, ${shlibs:Depends}, ${misc:Depends},
${ocaml:Depends}
> +Description: interface for remotely configuring and controlling guests
(runtime)
> + The Xen Management API is an interface for remotely configuring and
> + controlling virtualised guests running on a Xen-enabled host. This
> + 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.
> +Package: libxen-ocaml-dev
> +Architecture: any
> +Section: ocaml
> +Provides: ${ocaml:Provides}
> +Depends: ocaml-nox-${F:OCamlABI}, libxen-ocaml (= ${binary:Version}),
${shlibs:Depends}, ocaml-findlib (>= 1.1), ${misc:Depends}, ${ocaml:Depends}
> +Description: interface for remotely configuring and controlling guests
(development files)
> + The Xen Management API is an interface for remotely configuring and
> + controlling virtualised guests running on a Xen-enabled host. This
> + package containst the development files for the OCaml library.
Same typo and comment wrt the Xen Management API.
> +
> Package: libxenstore3.0
> Architecture: any
> Section: libs
> Depends: ${shlibs:Depends}, ${misc:Depends}
> 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>
> @@ -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.
> 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?
> 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).
> +Forwarded: not-needed
> +Author: Unkown for the moment.
"Unknown".
I think the "for the moment" is a bit optimistic too ;-)
> +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?
> @@ -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?
> 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?
I noticed a couple of these sorts of typo-in-debug fixes in this patch,
I'm not convinced they are worth backporting and maintaining in the
packaging.
> diff -r -u old/xen-4.1.1/qemu/xen-hooks.mak
new/xen-4.1.1/qemu/xen-hooks.mak
> --- old/xen-4.1.1/qemu/xen-hooks.mak 2011-08-07 01:02:50.000000000 +0000
> +++ new/xen-4.1.1/qemu/xen-hooks.mak 2011-08-07 07:40:06.000000000 +0000
> @@ -67,6 +67,8 @@
> $(info === PCI passthrough capability has been enabled ===)
> endif
>
> +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.
--
Ian Campbell
"Well hello there Charlie Brown, you blockhead."
-- Lucy Van Pelt