[Buildroot] [PATCH 2/2] xen: new package
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Thu Jul 2 10:00:02 UTC 2015
Hello,
On Thu, 2 Jul 2015 11:43:08 +0200, Maxime Ripard wrote:
> > > + select BR2_PACKAGE_DTC
> > > + select BR2_PACKAGE_LIBAIO
> > > + select BR2_PACKAGE_LIBGLIB2
> > > + select BR2_PACKAGE_NCURSES
> > > + select BR2_PACKAGE_OPENSSL
> > > + select BR2_PACKAGE_PIXMAN
> > > + select BR2_PACKAGE_UTIL_LINUX
> > > + select BR2_PACKAGE_UTIL_LINUX_LIBUUID
> > > + select BR2_PACKAGE_YAJL
> >
> > Are all these dependencies needed when you don't install the Xen tools?
>
> Probably not, the hypervisor is completely standalone, so I don't
> think it would require any of these.
Ok, so all those select can be moved to the "tools" option, then.
Also, and maybe this is something Ian could clarify: do we really need
pixman or ncurses when you just want to start text-only Xen VMs ?
> BTW, I just found out that I left something out of this patch. The
> initscripts are using some bashism that don't work with busybox'
> ash. What's the way Buildroot deal with this usually? Add a dependency
> on bash?
We generally prefer to have the scripts fixed so that they don't use
bashisms. If that's not doable, then selecting bash is a possibility
(but don't add it to XEN_DEPENDENCIES, since it's a runtime dependency
only). Of course, make sure the scripts use bash explicitly in their
shebang, because you're not guaranteed that bash will be the default
shell.
> > Also, if neither the hypervisor nor the tools are enabled, what gets
> > built? Nothing? If so, maybe you want:
> >
> > select BR2_PACKAGE_XEN_HYPERVISOR if !BR2_PACKAGE_XEN_TOOLS
> >
> > in the top-level option.
>
> I think it would make more sense to have the tools built by default,
> which would reverse the select statement I guess?
Correct.
> > Missing LICENSE + LICENSE_FILES.
>
> Ah, right.
>
> It uses the GPL as its license, with some 2c and 3c BSD
> exceptions. How is that usually dealt with ?
>
> Using
> LICENSE = GPLv2 with exceptions
>
> Or making an exhaustive list of the licenses used?
GPLv2 with expections means "the license is the text of the GPLv2 to
which some additional clauses/wording has been added". Which is not
really what you have IIUC.
Maybe:
LICENSE = GPLv2, BSD-2c, BSD-3c
or better (if doable):
LICENSE = GPLv2 (this), BSD-2c (that), BSD-3c (this other thing)
> > > + CROSS_COMPILE=$(TARGET_CROSS) \
> > > + CXXFLAGS="$(TARGET_CXXFLAGS) -D_FILE_OFFSET_BITS=64" \
> > > + CFLAGS="$(TARGET_CFLAGS) -D_FILE_OFFSET_BITS=64" \
> >
> > Why do you pass -D_FILE_OFFSET_BITS=64 ? It is already in
> > TARGET_CFLAGS / TARGET_CXXFLAGS.
>
> It was required during the make invocation, while Buildroot only
> passes it during the configure.
Alright. Xen's build system could use some improvement, but I guess
that outside of the scope of Buildroot packaging :-)
> > > + PKG_CONFIG=$(PKG_CONFIG_HOST_BINARY)
> > > +
> > > +XEN_INSTALL_TARGET_OPTS = DESTDIR=$(TARGET_DIR)
> >
> > This seems weird, you're losing the "install" target.
>
> It's actually on purpose. Xen has a specific install target for the
> tools (install-target), that is added a bit later when compiling the
> tools.
Then why not do it just in one go, since you're using install-tools
only?
Like:
XEN_INSTALL_TARGET_OPTS = DESTDIR=$(TARGET_DIR) install-tools
Or maybe it's because you don't want "make install" to be called when
only the hypervisor is enabled. In this case, just do
XEN_INSTALL_TARGET = NO.
ifeq ($(...hypervisor...),y)
XEN_INSTALL_IMAGES = YES
XEN_INSTALL_TARGET = NO
...
else ifeq ($(...tools...),y)
XEN_INSTALL_TARGET_OPTS = DESTDIR=$(TARGET_DIR) install-tools
...
endif
> > Ah, so here it comes. So maybe instead you want to do
> > XEN_INSTALL_TARGET = NO by default, and do XEN_INSTALL_TARGET_OPTS =
> > DESTDIR=$(TARGET_DIR) install-tools when BR2_PACKAGE_XEN_TOOLS=y.
>
> Now I see that I forgot that as well, but the hypervisor can be
> installed in /boot using install-xen.
>
> Maybe I could just add a new option to ask whether we want to install
> it to /boot, and add install-xen to INSTALL_TARGET_OPTS if relevant.
Right. So you would have to change the above proposal, though :)
> > > +define XEN_RENAME_INIT_SCRIPTS
> > > + mv $(TARGET_DIR)/etc/init.d/xencommons $(TARGET_DIR)/etc/init.d/S50xencommons
> > > + mv $(TARGET_DIR)/etc/init.d/xen-watchdog $(TARGET_DIR)/etc/init.d/S50xen-watchdog
> > > + mv $(TARGET_DIR)/etc/init.d/xendomains $(TARGET_DIR)/etc/init.d/S60xendomains
> >
> > Don't know if it's better or not:
> >
> > (cd $(TARGET_DIR)/etc/init.d; \
> > mv xencommons S50xencommons; \
> > mv ... ; \
> > mv ...)
>
> I don't really have a preference. I'll change this if you want.
Maybe your proposal is simpler. You can keep it as is.
>
> > > +endef
> > > +else
> > > +XEN_CONF_OPTS += --disable-tools
> > > +endif
> > > +
> > > +XEN_POST_INSTALL_TARGET_HOOKS += XEN_RENAME_INIT_SCRIPTS
> >
> > Should be part of the ifeq BR2_PACKAGE_XEN_TOOLS=y.
>
> Is it? My understanding was that the macro wouldn't be defined and it
> would call an empty one in such a case?
Correct: it works fine. But we generally prefer to register the hook
only when needed.
> > You need to change:
> >
> > AC_CHECK_LIB([yajl], [yajl_alloc], [],
> > [AC_MSG_ERROR([Could not find yajl])])
> >
> > to:
> >
> > AC_CHECK_LIB([yajl], [yajl_alloc], [],
> > [AC_MSG_ERROR([Could not find yajl])], [m])
>
> Ack.
Note that I haven't tested this change, it was purely based on a quick
reading of Xen's configure.ac.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
More information about the buildroot
mailing list