[Patch v3 0/2] Remove figlet from the source tree

[Patch v3 0/2] Remove figlet from the source tree

[Andrew Cooper]

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>

[Patch v3 1/2] xen/build: Use a distro version of figlet

[Andrew Cooper]

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

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Campbell]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[Andrew Cooper]

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

Re: [Patch v3 0/2] Remove figlet from the source tree

[George Dunlap]

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.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.

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

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Campbell]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Jackson]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[Keir Fraser]

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

Re: [Patch v3 0/2] Remove figlet from the source tree

[Matthew Daley]

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

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Campbell]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Campbell]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[Keir Fraser]

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.
> 

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Campbell]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Jackson]

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.

Re: [Patch v3 0/2] Remove figlet from the source tree

[George Dunlap]

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

Re: [Patch v3 0/2] Remove figlet from the source tree

[Ian Jackson]

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.