This series removes the hacked up version of figlet from our source tree. Patch 1 replaces the use of figlet in the build system. Patch 2 has been deliberately omitted for brevity. It consistes of removing "xen/tools/figlet/figlet" from .gitignore, and deleting all files in the directory "xen/tools/figlet/" The series as a whole can be found on the ''remove-figlet-v3'' branch of http://xenbits.xen.org/git-http/people/andrewcoop/xen.git George: This is purely a build change, so I request that it be included for 4.4 Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> CC: Keir Fraser <keir@xen.org> CC: Jan Beulich <JBeulich@suse.com> CC: Ian Campbell <Ian.Campbell@citrix.com> CC: Ian Jackson <Ian.Jackson@eu.citrix.com> CC: George Dunlap <george.dunlap@eu.citrix.com>
Andrew Cooper
2013-Nov-25 11:00 UTC
[Patch v3 1/2] xen/build: Use a distro version of figlet
It is quite inappropriate to keep a hacked up versions of figlet in our source
tree, especially when the purpose of the hackary is just to provide a text to
octal conversion.
This version of figlet contributes a surprisingly large proportion of the
Coverity issues found under xen/ (and therefore attributed against Xen)
Figlet can be found in all distros, so make use of it. We keep xen.flf (being
the Xen figlet font) and replace the hacked up octal transform with a short
python script.
The Xen Makefile has been tweaked in such a way that it still prints the
figlet banner for the build.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
Changes in v3:
* Split into two patches
* Update README
---
.gitignore | 3 +--
README | 1 +
xen/Makefile | 16 +++++++++-------
xen/tools/Makefile | 2 --
xen/tools/fig-to-oct.py | 18 ++++++++++++++++++
xen/tools/{figlet => }/xen.flf | 0
6 files changed, 29 insertions(+), 11 deletions(-)
create mode 100644 xen/tools/fig-to-oct.py
rename xen/tools/{figlet => }/xen.flf (100%)
diff --git a/.gitignore b/.gitignore
index f88e431..47f92a4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -292,7 +292,7 @@ tools/xm-test/lib/XmTestLib/config.py
tools/xm-test/lib/XmTestReport/xmtest.py
tools/xm-test/tests/*.test
tools/ocaml-xenstored*
-xen/.banner*
+xen/.banner
xen/System.map
xen/arch/arm/asm-offsets.s
xen/arch/arm/xen.lds
@@ -315,7 +315,6 @@ xen/include/linux
xen/include/public/public
xen/include/xen/*.new
xen/include/xen/acm_policy.h
-xen/include/xen/banner.h
xen/include/xen/compile.h
xen/tools/figlet/figlet
xen/tools/symbols
diff --git a/README b/README
index 8689ce1..4148a26 100644
--- a/README
+++ b/README
@@ -71,6 +71,7 @@ disabled at compile time:
includes the alternative ocaml xenstored.
* cmake (if building vtpm stub domains)
* markdown
+ * figlet (for generating the traditional Xen start of day banner)
Second, you need to acquire a suitable kernel for use in domain 0. If
possible you should use a kernel provided by your OS distributor. If
diff --git a/xen/Makefile b/xen/Makefile
index 597972d..1ea2717 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -86,7 +86,7 @@ _clean: delete-unfresh-files
$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) clean
rm -f include/asm *.o $(TARGET) $(TARGET).gz $(TARGET)-syms *~ core
rm -f include/asm-*/asm-offsets.h
- [ -d tools/figlet ] && rm -f .banner*
+ rm -f .banner
.PHONY: _distclean
_distclean: clean
@@ -114,10 +114,12 @@ delete-unfresh-files:
fi
.banner: Makefile
- $(MAKE) -C tools
- @tools/figlet/figlet -d tools/figlet Xen $(XEN_FULLVERSION) 2>$@2 >$@1
- @cat $@1 $@2 >$@
- @rm -f $@1 $@2
+ @if which figlet >/dev/null 2>&1 ; then \
+ echo " Xen $(XEN_FULLVERSION)" | figlet -f tools/xen.flf >
$@.tmp; \
+ else \
+ echo " Xen $(XEN_FULLVERSION)" > $@.tmp; \
+ fi
+ @mv -f $@.tmp $@
# compile.h contains dynamic build info. Rebuilt on every
''make'' invocation.
include/xen/compile.h: include/xen/compile.h.in .banner
@@ -132,8 +134,8 @@ include/xen/compile.h: include/xen/compile.h.in .banner
-e ''s/@@extraversion@@/$(XEN_EXTRAVERSION)/g'' \
-e ''s!@@changeset@@!$(shell tools/scmversion $(XEN_ROOT) || echo
"unavailable")!g'' \
< include/xen/compile.h.in > $@.new
- @grep \" .banner >> $@.new
- @grep -v \" .banner
+ @cat .banner
+ @$(PYTHON) tools/fig-to-oct.py < .banner >> $@.new
@mv -f $@.new $@
include/asm-$(TARGET_ARCH)/asm-offsets.h: arch/$(TARGET_ARCH)/asm-offsets.s
diff --git a/xen/tools/Makefile b/xen/tools/Makefile
index 612e36d..e940939 100644
--- a/xen/tools/Makefile
+++ b/xen/tools/Makefile
@@ -3,12 +3,10 @@ include $(XEN_ROOT)/Config.mk
.PHONY: default
default:
- [ -d figlet ] && $(MAKE) -C figlet
$(MAKE) symbols
.PHONY: clean
clean:
- [ -d figlet ] && $(MAKE) -C figlet clean
rm -f *.o symbols
symbols: symbols.c
diff --git a/xen/tools/fig-to-oct.py b/xen/tools/fig-to-oct.py
new file mode 100644
index 0000000..db4fd32
--- /dev/null
+++ b/xen/tools/fig-to-oct.py
@@ -0,0 +1,18 @@
+#!/usr/bin/env python
+import sys
+
+chars_per_line = 18
+chars_so_far = 0
+
+sys.stdout.write(''"'')
+
+for char in sys.stdin.read():
+
+ sys.stdout.write("\\%03o" % ord(char))
+ chars_so_far = chars_so_far + 1
+
+ if chars_so_far == chars_per_line:
+ chars_so_far = 0
+ sys.stdout.write(''" \\\n"'')
+
+sys.stdout.write(''"\n'')
diff --git a/xen/tools/figlet/xen.flf b/xen/tools/xen.flf
similarity index 100%
rename from xen/tools/figlet/xen.flf
rename to xen/tools/xen.flf
--
1.7.10.4
On Mon, 2013-11-25 at 11:00 +0000, Andrew Cooper wrote:> This series removes the hacked up version of figlet from our source tree. > > Patch 1 replaces the use of figlet in the build system. > > Patch 2 has been deliberately omitted for brevity. It consistes of removing > "xen/tools/figlet/figlet" from .gitignore, and deleting all files in the > directory "xen/tools/figlet/" > > The series as a whole can be found on the ''remove-figlet-v3'' branch of > http://xenbits.xen.org/git-http/people/andrewcoop/xen.gitfatal: http://xenbits.xen.org/git-http/people/andrewcoop/xen.git/info/refs not found: did you run git update-server-info on the server? git://xenbits.xen.org/people/andrewcoop/xen.git works though. Ian.
On 25/11/13 11:21, Ian Campbell wrote:> On Mon, 2013-11-25 at 11:00 +0000, Andrew Cooper wrote: >> This series removes the hacked up version of figlet from our source tree. >> >> Patch 1 replaces the use of figlet in the build system. >> >> Patch 2 has been deliberately omitted for brevity. It consistes of removing >> "xen/tools/figlet/figlet" from .gitignore, and deleting all files in the >> directory "xen/tools/figlet/" >> >> The series as a whole can be found on the ''remove-figlet-v3'' branch of >> http://xenbits.xen.org/git-http/people/andrewcoop/xen.git > fatal: http://xenbits.xen.org/git-http/people/andrewcoop/xen.git/info/refs not found: did you run git update-server-info on the server? > > git://xenbits.xen.org/people/andrewcoop/xen.git works though. > > Ian. >Done - and some refs have appeared. ~Andrew
On 11/25/2013 11:00 AM, Andrew Cooper wrote:> This series removes the hacked up version of figlet from our source tree. > > Patch 1 replaces the use of figlet in the build system. > > Patch 2 has been deliberately omitted for brevity. It consistes of removing > "xen/tools/figlet/figlet" from .gitignore, and deleting all files in the > directory "xen/tools/figlet/" > > The series as a whole can be found on the ''remove-figlet-v3'' branch of > http://xenbits.xen.org/git-http/people/andrewcoop/xen.git > > George: > This is purely a build change, so I request that it be included for 4.4It''s not purely a build change -- figlet is not installed by default on a number of distros (Ubuntu didn''t have it); and on those systems, this will silently change remove the figlet-like lettering from boot. And in any case, a build change still introduces a risk (however small) that there will be a bug in the build under certain distros / configurations which will have to be fixed, and may slip the release. Remember our criteria: 1. A bug-free release 2. An awesome release 3. An on-time release Your freeze exception analyses tend to focus exclusively on #2 -- moreover, on the positives of your patch ("w00t, no crappy figlet fork in-tree!") and not on the potential negatives ([User doesn''t notice new figlet requirement] "That''s weird, why did the banner go away? It was kind of cool."). Please stop and think carefully about *each* of the criteria before making a recommendation. From a release perspective, I think the benefits out-weigh the risks, so: Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com> From a patch-review perspective: Although it''s clear we need to remove figlet from the tree, I''m afraid that the effect of this on most people will simply be to have the banner silently disappear. There''s no way we could have something in the build that would prompt people, is there? I can''t really think of a non-intrusive way at the moment, but maybe someone has some ideas... -George
On Mon, 2013-11-25 at 12:21 +0000, George Dunlap wrote:> From a patch-review perspective: Although it''s clear we need to remove > figlet from the tree, I''m afraid that the effect of this on most people > will simply be to have the banner silently disappear. There''s no way we > could have something in the build that would prompt people, is there? I > can''t really think of a non-intrusive way at the moment, but maybe > someone has some ideas...It doesn''t quite disappear it is just printed as normal ASCII text instead. We could include a static "Xen" banner figlet output and just print the version in normal ASCII text. Or we could check in the figlet banner and make it part of the release process to update it (liable to forgetting though, so I''m not in favour of this). Ian.
George Dunlap writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from
the source tree"):> On 11/25/2013 11:00 AM, Andrew Cooper wrote:
> > George:
> > This is purely a build change, so I request that it be included for
4.4
>
> It''s not purely a build change -- figlet is not installed by
default on
> a number of distros (Ubuntu didn''t have it); and on those systems,
this
> will silently change remove the figlet-like lettering from boot.
I agree with these criticisms of Andrew''s claims for the patch.
But I think the inclusion of a fork of figlet in our tree is totally
crazy (and hence a bug). I wouldn''t want to introduce this patch
without a release ack but I think the release ack should be granted.
Thanks,
Ian.
Ian Campbell writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the
source tree"):> It doesn''t quite disappear it is just printed as normal ASCII text
> instead.
>
> We could include a static "Xen" banner figlet output and just
print the
> version in normal ASCII text. Or we could check in the figlet banner and
> make it part of the release process to update it (liable to forgetting
> though, so I''m not in favour of this).
I far prefer the approach in Andrew''s patch.
Ian.
On 11/25/2013 02:38 PM, Ian Jackson wrote:> George Dunlap writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the source tree"): >> On 11/25/2013 11:00 AM, Andrew Cooper wrote: >>> George: >>> This is purely a build change, so I request that it be included for 4.4 >> It''s not purely a build change -- figlet is not installed by default on >> a number of distros (Ubuntu didn''t have it); and on those systems, this >> will silently change remove the figlet-like lettering from boot. > I agree with these criticisms of Andrew''s claims for the patch. > > But I think the inclusion of a fork of figlet in our tree is totally > crazy (and hence a bug). I wouldn''t want to introduce this patch > without a release ack but I think the release ack should be granted.If you read to the end, you''ll see that it was. :-) -George
George Dunlap writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from
the source tree"):> On 11/25/2013 02:38 PM, Ian Jackson wrote:
> > I agree with these criticisms of Andrew''s claims for the
patch.
> >
> > But I think the inclusion of a fork of figlet in our tree is totally
> > crazy (and hence a bug). I wouldn''t want to introduce this
patch
> > without a release ack but I think the release ack should be granted.
>
> If you read to the end, you''ll see that it was. :-)
Yes, indeed, I did. Sorry that that wasn''t clear. I was agreeing.
Ian.
On 25/11/2013 14:38, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote:> Ian Campbell writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the > source tree"): >> It doesn''t quite disappear it is just printed as normal ASCII text >> instead. >> >> We could include a static "Xen" banner figlet output and just print the >> version in normal ASCII text. Or we could check in the figlet banner and >> make it part of the release process to update it (liable to forgetting >> though, so I''m not in favour of this). > > I far prefer the approach in Andrew''s patch.Personally I think we should just get rid of the banner entirely. -- Keir
On Tue, Nov 26, 2013 at 8:45 AM, Keir Fraser <keir.xen@gmail.com> wrote:> On 25/11/2013 14:38, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote: > >> Ian Campbell writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the >> source tree"): >>> It doesn''t quite disappear it is just printed as normal ASCII text >>> instead. >>> >>> We could include a static "Xen" banner figlet output and just print the >>> version in normal ASCII text. Or we could check in the figlet banner and >>> make it part of the release process to update it (liable to forgetting >>> though, so I''m not in favour of this). >> >> I far prefer the approach in Andrew''s patch. > > Personally I think we should just get rid of the banner entirely.Aww. I was gonna submit a replacement shell one-liner, like this: $ banner () { f=`mktemp`; for c in `echo -n "$1" | od -An -b`; do tail -n+$(((0$c - 32) * $3 + $4)) $2 | head -n6 | cut -b2- | paste -d@ $f -> $f.; mv $f{.,}; done; tr -d @ < $f | tr $ '' ''; rm $f; }$ banner "Xen 4.4-blah" xen/tools/figlet/xen.flf 6 16 __ __ _ _ _ _ _ _ _ \ \/ / ___ _ __ | || | | || | | |__ | | __ _ | |__ \ / / _ \| ''_ \ | || |_ | || |_ __ | ''_ \ | | / _` || ''_ \ / \ | __/| | | | |__ _| _ |__ _||__|| |_) || || (_| || | | | /_/\_\ \___||_| |_| |_| (_) |_| |_.__/ |_| \__,_||_| |_| - Matthew
On Tue, 2013-11-26 at 14:25 +1300, Matthew Daley wrote:> Aww. I was gonna submit a replacement shell one-liner, like this: > > $ banner () { f=`mktemp`; for c in `echo -n "$1" | od -An -b`; do tail > -n+$(((0$c - 32) * $3 + $4)) $2 | head -n6 | cut -b2- | paste -d@ $f - > > $f.; mv $f{.,}; done; tr -d @ < $f | tr $ '' ''; rm $f; } > $ banner "Xen 4.4-blah" xen/tools/figlet/xen.flf 6 16 > __ __ _ _ _ _ _ _ _ > \ \/ / ___ _ __ | || | | || | | |__ | | __ _ | |__ > \ / / _ \| ''_ \ | || |_ | || |_ __ | ''_ \ | | / _` || ''_ \ > / \ | __/| | | | |__ _| _ |__ _||__|| |_) || || (_| || | | | > /_/\_\ \___||_| |_| |_| (_) |_| |_.__/ |_| \__,_||_| |_|Wowza! Put the keyboard down and step away from the shell! ;-) Ian.
On Mon, 2013-11-25 at 19:45 +0000, Keir Fraser wrote:> On 25/11/2013 14:38, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote: > > > Ian Campbell writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the > > source tree"): > >> It doesn''t quite disappear it is just printed as normal ASCII text > >> instead. > >> > >> We could include a static "Xen" banner figlet output and just print the > >> version in normal ASCII text. Or we could check in the figlet banner and > >> make it part of the release process to update it (liable to forgetting > >> though, so I''m not in favour of this). > > > > I far prefer the approach in Andrew''s patch. > > Personally I think we should just get rid of the banner entirely.In the absence of such a patch I think Andrew''s series is a step in the right direction. Any objections to me merging it? Ian.
On 29/11/2013 10:43, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:> On Mon, 2013-11-25 at 19:45 +0000, Keir Fraser wrote: >> On 25/11/2013 14:38, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote: >> >>> Ian Campbell writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the >>> source tree"): >>>> It doesn''t quite disappear it is just printed as normal ASCII text >>>> instead. >>>> >>>> We could include a static "Xen" banner figlet output and just print the >>>> version in normal ASCII text. Or we could check in the figlet banner and >>>> make it part of the release process to update it (liable to forgetting >>>> though, so I''m not in favour of this). >>> >>> I far prefer the approach in Andrew''s patch. >> >> Personally I think we should just get rid of the banner entirely. > > In the absence of such a patch I think Andrew''s series is a step in the > right direction. > > Any objections to me merging it?Not from me. -- Keir> Ian. >
On Fri, 2013-11-29 at 20:15 +0000, Keir Fraser wrote:> On 29/11/2013 10:43, "Ian Campbell" <Ian.Campbell@citrix.com> wrote: > > > On Mon, 2013-11-25 at 19:45 +0000, Keir Fraser wrote: > >> On 25/11/2013 14:38, "Ian Jackson" <Ian.Jackson@eu.citrix.com> wrote: > >> > >>> Ian Campbell writes ("Re: [Xen-devel] [Patch v3 0/2] Remove figlet from the > >>> source tree"): > >>>> It doesn''t quite disappear it is just printed as normal ASCII text > >>>> instead. > >>>> > >>>> We could include a static "Xen" banner figlet output and just print the > >>>> version in normal ASCII text. Or we could check in the figlet banner and > >>>> make it part of the release process to update it (liable to forgetting > >>>> though, so I''m not in favour of this). > >>> > >>> I far prefer the approach in Andrew''s patch. > >> > >> Personally I think we should just get rid of the banner entirely. > > > > In the absence of such a patch I think Andrew''s series is a step in the > > right direction. > > > > Any objections to me merging it? > > Not from me.... in it goes. Ian.