[pacman-dev] About makepkg dlagents
About download-agents for makepkg. (This is why I've bothered the developers of pacman this morning with compilation issues... :)) So I wanted to be able to make PKGBUILD and makepkg extract sources for Git repositories. And after asking on the arch-general, I was pointed to the following bug: http://bugs.archlinux.org/task/7816 As a consequence I've started hacking and commed up with the following patches (put at the end). (I've ended up with four pathes from git format-patch... Should I put only the diff between maint branch and my branch?) Also below is the description on the bug-tracker: ~~~~ I've made some hacking staring from pacman Git repository. My work is available at (branch patches/git-dlagent): http://gitorious.org/~ciprian.craciun/pacman/ciprian-craciun-patches In summary: * added a script: /usr/bin/git-dlagent that takes exactly two arguments: URL and output. * updated /etc/makepkg.conf to include the following URL's: git://, git+file://, git+http://, git+https://, git+ssh://, git+rsync; * a Git url looks-like: git+git://projects.archlinux.org/pacman.git#master##pacman.tar where: * what is before the first `#` is the URL used to (maybe) clone the Git repository. * what is after the first `#` and before `##` is the refspec (head / tag / etc.) to export; * what is after `##` (optional this part is) it can be used to make the file-part of the URL look like a tar file; How git-dlagent it works: * in case git+file:// is given `git --git-dir=... archive` is done; * else it tries to do `git archive --remote=...`; (usually this doesn't work unless the repository is git and the barkend is enabled); * but anyway it fallbacks to `git clone ...`, and then a local git archive; ~~~~ (My approach just uses the Git / SVN / etc tools to create a .tar or .zip file, and leave the rest up to makepkg. I've also patched makepkg to ignore those hashes that are equal to `--`.) ~~~~ I would say it integrated prety nice with the whole makepkg, and also does not impact the source code by much. An example of an URL used in PKGBUILD would be the following: git+file:///stores/erebus-1/ciprian/workbench/pacman/.git#patches/master##/$pkgname-$pkgver.tar git://projects.archlinux.org/pacman.git#maint#/pacman-v3.3.2.tar Is anyone interested in adopting this patch? Any comments? ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From c08594be518814646f7e9d9224a0068e9e4d6f34 Mon Sep 17 00:00:00 2001 From: Ciprian Dorin, Craciun <ciprian@volution.ro> Date: Mon, 19 Oct 2009 22:53:06 +0300 Subject: [PATCH 1/4] Added option to ignore a source integrity hash. (We just replace the hash with `--`.) Added makepkg download-agent for git. (git-dlagent.bash)
--- scripts/git-dlagent.sh.in | 78 +++++++++++++++++++++++++++++++++++++++++++++ scripts/makepkg.sh.in | 14 +++++--- 2 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 scripts/git-dlagent.sh.in diff --git a/scripts/git-dlagent.sh.in b/scripts/git-dlagent.sh.in new file mode 100644 index 0000000..a2240a7 --- /dev/null +++ b/scripts/git-dlagent.sh.in @@ -0,0 +1,78 @@ +#!/bin/bash + +set -e -u -o pipefail || exit 1 +test "${#}" -eq 2 + +uri="${1}" +output="${2}" + +git_archive_method=unknown + + +if [[ "${uri}" =~ ^git://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +then + uri="${uri/%##*}" + reference="${uri/#*#}" + uri="${uri/%#*}" + + git_archive_method=remote + +elif [[ "${uri}" =~ ^(git\+)?file:///[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +then + uri="${uri/%##*}" + reference="${uri/#*#}" + uri="${uri/%#*}" + uri="${uri/#git+file:\/\/}" + + git_archive_method=local + +elif [[ "${uri}" =~ ^(git\+)?(ssh|http|https|rsync)://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +then + uri="${uri/%##*}" + reference="${uri/#*#}" + uri="${uri/%#*}" + uri="${uri/#git+}" + + git_archive_method=remote + +else + echo "[ee] \`${uri}\` is not a Git URI." >&2 + exit 1 +fi + + +case "${git_archive_method}" in + + ( local ) + + git --git-dir="${uri}" archive --format=tar --output="${output}" "${reference}" + + exit 0 + ;; + + ( remote ) + + git archive --format=tar --output="${output}" --remote="${uri}" "${reference}" \ + && exit 0 + + echo "[ww] \`${uri}\` does not support git-archive; going after clone." >&2 + + clone="$( mktemp -d )" + + git clone --bare "${uri}" "${clone}" + + git --git-dir="${clone}" archive --format=tar --output="${output}" "${reference}" + + find "${clone}" -delete + + exit 0 + ;; + + ( * ) + echo "[ee] unknown git-archive-method." >&2 + exit 1 + ;; + +esac + +exit 1 diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 3662228..3cdf2aa 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -591,12 +591,16 @@ check_checksums() { if [ $found -gt 0 ] ; then local expectedsum="$(echo ${integrity_sums[$idx]} | tr '[A-F]' '[a-f]')" - local realsum="$(openssl dgst -${integ} "$file" | awk '{print $NF}')" - if [ "$expectedsum" = "$realsum" ]; then - echo "$(gettext "Passed")" >&2 + if [ "$expectedsum" = '--' ]; then + echo "$(gettext "Skipped")" >&2 else - echo "$(gettext "FAILED")" >&2 - errors=1 + local realsum="$(openssl dgst -${integ} "$file" | awk '{print $NF}')" + if [ "$expectedsum" = "$realsum" ]; then + echo "$(gettext "Passed")" >&2 + else + echo "$(gettext "FAILED")" >&2 + errors=1 + fi fi fi -- 1.6.3.1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From 47c0d7fef2eefa69081c809dd285fa2fcf88eb36 Mon Sep 17 00:00:00 2001 From: Ciprian Dorin, Craciun <ciprian@volution.ro> Date: Mon, 19 Oct 2009 22:57:53 +0300 Subject: [PATCH 2/4] Integrated git-dlagent into the build system.
--- etc/makepkg.conf.in | 8 +++++++- scripts/Makefile.am | 7 +++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/etc/makepkg.conf.in b/etc/makepkg.conf.in index 1368ff1..29f6e4e 100644 --- a/etc/makepkg.conf.in +++ b/etc/makepkg.conf.in @@ -12,7 +12,13 @@ DLAGENTS=('ftp::/usr/bin/wget -c --passive-ftp -t 3 --waitretry=3 -O %o %u' 'http::/usr/bin/wget -c -t 3 --waitretry=3 -O %o %u' 'https::/usr/bin/wget -c -t 3 --waitretry=3 --no-check-certificate -O %o %u' 'rsync::/usr/bin/rsync -z %u %o' - 'scp::/usr/bin/scp -C %u %o') + 'scp::/usr/bin/scp -C %u %o' + 'git::/usr/bin/git-dlagent %u %o' + 'git+file::/usr/bin/git-dlagent %u %o' + 'git+http::/usr/bin/git-dlagent %u %o' + 'git+https::/usr/bin/git-dlagent %u %o' + 'git+ssh::/usr/bin/git-dlagent %u %o' + 'git+rsync::/usr/bin/git-dlagent %u %o') # Other common tools: # /usr/bin/snarf diff --git a/scripts/Makefile.am b/scripts/Makefile.am index 5d65653..524710c 100644 --- a/scripts/Makefile.am +++ b/scripts/Makefile.am @@ -10,14 +10,16 @@ OURSCRIPTS = \ pacman-optimize \ pkgdelta \ rankmirrors \ - repo-add + repo-add \ + git-dlagent EXTRA_DIST = \ makepkg.sh.in \ pacman-optimize.sh.in \ pkgdelta.sh.in \ rankmirrors.py.in \ - repo-add.sh.in + repo-add.sh.in \ + git-dlagent.sh.in # Files that should be removed, but which Automake does not know. MOSTLYCLEANFILES = $(bin_SCRIPTS) *.tmp @@ -67,5 +69,6 @@ repo-add: $(srcdir)/repo-add.sh.in repo-remove: $(srcdir)/repo-add.sh.in rm -f repo-remove $(LN_S) repo-add repo-remove +git-dlagent: $(srcdir)/git-dlagent.sh.in # vim:set ts=2 sw=2 noet: -- 1.6.3.1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From 8fc53a663e7b7521133a05b8e28d9fd3f15efaea Mon Sep 17 00:00:00 2001 From: Ciprian Dorin, Craciun <ciprian@volution.ro> Date: Mon, 19 Oct 2009 23:48:51 +0300 Subject: [PATCH 3/4] Updated git-dlagent. git-archive is usable only for file:// or git:// or git+ssh:// repositories.
--- scripts/git-dlagent.sh.in | 23 +++++++++++++++++------ 1 files changed, 17 insertions(+), 6 deletions(-) diff --git a/scripts/git-dlagent.sh.in b/scripts/git-dlagent.sh.in index a2240a7..993c7b5 100644 --- a/scripts/git-dlagent.sh.in +++ b/scripts/git-dlagent.sh.in @@ -26,7 +26,7 @@ then git_archive_method=local -elif [[ "${uri}" =~ ^(git\+)?(ssh|http|https|rsync)://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +elif [[ "${uri}" =~ ^(git\+)?ssh://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] then uri="${uri/%##*}" reference="${uri/#*#}" @@ -35,6 +35,15 @@ then git_archive_method=remote +elif [[ "${uri}" =~ ^(git\+)?(http|https|rsync)://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +then + uri="${uri/%##*}" + reference="${uri/#*#}" + uri="${uri/%#*}" + uri="${uri/#git+}" + + git_archive_method=remote-force + else echo "[ee] \`${uri}\` is not a Git URI." >&2 exit 1 @@ -50,12 +59,14 @@ case "${git_archive_method}" in exit 0 ;; - ( remote ) - - git archive --format=tar --output="${output}" --remote="${uri}" "${reference}" \ - && exit 0 + ( remote | remote-force ) - echo "[ww] \`${uri}\` does not support git-archive; going after clone." >&2 + if test "${git_archive_method}" == remote + then + git archive --format=tar --output="${output}" --remote="${uri}" "${reference}" \ + && exit 0 + echo "[ww] \`${uri}\` does not support git-archive; going after clone." >&2 + fi clone="$( mktemp -d )" -- 1.6.3.1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
From 31a684f8fc561d3b4ef6dc20ec05b6089e771bbd Mon Sep 17 00:00:00 2001 From: Ciprian Dorin, Craciun <ciprian@volution.ro> Date: Tue, 20 Oct 2009 09:03:23 +0300 Subject: [PATCH 4/4] Fixed git-dlagent (failure to work without the `git+` prefix).
Modified the way the URL is interpretted: git+<url-part>#refspec##/<name>.tar And now the archive should contain a folder with the name <name>. (The `##/<name>.tar` is optional.) --- scripts/git-dlagent.sh.in | 29 +++++++++++++++++++++-------- 1 files changed, 21 insertions(+), 8 deletions(-) diff --git a/scripts/git-dlagent.sh.in b/scripts/git-dlagent.sh.in index 993c7b5..7b7ec24 100644 --- a/scripts/git-dlagent.sh.in +++ b/scripts/git-dlagent.sh.in @@ -9,25 +9,35 @@ output="${2}" git_archive_method=unknown -if [[ "${uri}" =~ ^git://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +if [[ "${uri}" =~ ^git://[^/#]+[^#]+#[^#]+(##/[a-zA-Z0-9_.-]+\.tar)?$ ]] then + + prefix="${uri/#*##\/}" + prefix="${prefix/%.tar}" uri="${uri/%##*}" reference="${uri/#*#}" uri="${uri/%#*}" git_archive_method=remote -elif [[ "${uri}" =~ ^(git\+)?file:///[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +elif [[ "${uri}" =~ ^(git\+)?file:///[^/#]+[^#]+#[^#]+(##/[a-zA-Z0-9_.-]+\.tar)?$ ]] then + + prefix="${uri/#*##\/}" + prefix="${prefix/%.tar}" uri="${uri/%##*}" reference="${uri/#*#}" uri="${uri/%#*}" - uri="${uri/#git+file:\/\/}" + uri="${uri/#git+}" + uri="${uri/#file:\/\/}" git_archive_method=local -elif [[ "${uri}" =~ ^(git\+)?ssh://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +elif [[ "${uri}" =~ ^(git\+)?ssh://[^/#]+[^#]+#[^#]+(##/[a-zA-Z0-9_.-]+\.tar)?$ ]] then + + prefix="${uri/#*##\/}" + prefix="${prefix/%.tar}" uri="${uri/%##*}" reference="${uri/#*#}" uri="${uri/%#*}" @@ -35,8 +45,11 @@ then git_archive_method=remote -elif [[ "${uri}" =~ ^(git\+)?(http|https|rsync)://[^/#]+[^#]+#[^#]+(##[a-zA-Z0-9_.-]+\.tar)?$ ]] +elif [[ "${uri}" =~ ^(git\+)?(http|https|rsync)://[^/#]+[^#]+#[^#]+(##/[a-zA-Z0-9_.-]+\.tar)?$ ]] then + + prefix="${uri/#*##\/}" + prefix="${prefix/%.tar}" uri="${uri/%##*}" reference="${uri/#*#}" uri="${uri/%#*}" @@ -54,7 +67,7 @@ case "${git_archive_method}" in ( local ) - git --git-dir="${uri}" archive --format=tar --output="${output}" "${reference}" + git --git-dir="${uri}" archive --format=tar --output="${output}" --prefix="${prefix}/" "${reference}" exit 0 ;; @@ -63,7 +76,7 @@ case "${git_archive_method}" in if test "${git_archive_method}" == remote then - git archive --format=tar --output="${output}" --remote="${uri}" "${reference}" \ + git archive --format=tar --output="${output}" --remote="${uri}" --prefix="${prefix}/" "${reference}" \ && exit 0 echo "[ww] \`${uri}\` does not support git-archive; going after clone." >&2 fi @@ -72,7 +85,7 @@ case "${git_archive_method}" in git clone --bare "${uri}" "${clone}" - git --git-dir="${clone}" archive --format=tar --output="${output}" "${reference}" + git --git-dir="${clone}" archive --format=tar --output="${output}" --prefix="${prefix}/" "${reference}" find "${clone}" -delete -- 1.6.3.1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Ciprian Dorin, Craciun wrote:
About download-agents for makepkg. (This is why I've bothered the developers of pacman this morning with compilation issues... :))
So I wanted to be able to make PKGBUILD and makepkg extract sources for Git repositories. And after asking on the arch-general, I was pointed to the following bug: http://bugs.archlinux.org/task/7816
As a consequence I've started hacking and commed up with the following patches (put at the end). (I've ended up with four pathes from git format-patch... Should I put only the diff between maint branch and my branch?)
Also below is the description on the bug-tracker:
~~~~ I've made some hacking staring from pacman Git repository. My work is available at (branch patches/git-dlagent): http://gitorious.org/~ciprian.craciun/pacman/ciprian-craciun-patches
In summary: * added a script: /usr/bin/git-dlagent that takes exactly two arguments: URL and output. * updated /etc/makepkg.conf to include the following URL's: git://, git+file://, git+http://, git+https://, git+ssh://, git+rsync;
<snip> Hi, Thanks for tackling this. There are some interesting ideas in your patch. From my point of view, this is far too complex for inclusion into makepkg. The addition of six new download URLs to makepkg (and that is just for git and not cvs,svn,hg,...) and requiring a download script just seems overboard. Taking a look at git PKGBUILDs on the AUR and the git prototype distributed with the Arch from the ABS package, what is really wanted is to reduce this: --start-- _gitroot="GITURL" _gitname="MODENAME" build() { cd "$srcdir" msg "Connecting to GIT server...." if [ -d $_gitname ] ; then cd $_gitname && git pull origin msg "The local files are updated." else git clone $_gitroot fi msg "GIT checkout done or server timeout" msg "Starting make..." rm -rf "$srcdir/$_gitname-build" git clone "$srcdir/$_gitname" "$srcdir/$_gitname-build" cd "$srcdir/$_gitname-build" --end-- with a "source=(git...)" line. I'm sure that you have covered some edge cases that can not be handled very easily in this fashion and I see some added functionality in your changes. However, I am not sure we need that complexity in 99% of use cases. Your patch has pointed out some interesting issues. Especially the issue of checksums and how they are handled when the source is not a real source file. Allan
On Mon, Oct 26, 2009 at 9:16 AM, Allan McRae <allan@archlinux.org> wrote:
Ciprian Dorin, Craciun wrote:
About download-agents for makepkg. (This is why I've bothered the developers of pacman this morning with compilation issues... :))
So I wanted to be able to make PKGBUILD and makepkg extract sources for Git repositories. And after asking on the arch-general, I was pointed to the following bug: http://bugs.archlinux.org/task/7816
As a consequence I've started hacking and commed up with the following patches (put at the end). (I've ended up with four pathes from git format-patch... Should I put only the diff between maint branch and my branch?)
Also below is the description on the bug-tracker:
~~~~ I've made some hacking staring from pacman Git repository. My work is available at (branch patches/git-dlagent): http://gitorious.org/~ciprian.craciun/pacman/ciprian-craciun-patches
In summary: * added a script: /usr/bin/git-dlagent that takes exactly two arguments: URL and output. * updated /etc/makepkg.conf to include the following URL's: git://, git+file://, git+http://, git+https://, git+ssh://, git+rsync;
<snip>
Hi,
Thanks for tackling this. There are some interesting ideas in your patch.
I'm glad you've loked over my proposal. (I was woried for a while. :) )
From my point of view, this is far too complex for inclusion into makepkg. The addition of six new download URLs to makepkg (and that is just for git and not cvs,svn,hg,...) and requiring a download script just seems overboard.
Its unfortunately that my proposal is not accepted in the current form, but I can't (completley) agree with the reasons: * "the addition of six new download URLs to makepkg config": I see why this is a problem -- there would be a lot of boiler-plate configuration code in there -- but this is not a problem of the extensions, but of the architecture of the configuration file; why couldn't we just source a config file for each download agent? or perhaps split the config file in multiple config files: this has other benefits also, by focusing on concerns: dlagent parameters, compile parameters, etc. * "requiring a download script": today makepkg already depends on curl / wget, rsync, or scp; why would another executable (or script in this case) be a problem?
Taking a look at git PKGBUILDs on the AUR and the git prototype distributed with the Arch from the ABS package, what is really wanted is to reduce this:
--start-- _gitroot="GITURL" _gitname="MODENAME"
build() { cd "$srcdir" msg "Connecting to GIT server...."
if [ -d $_gitname ] ; then cd $_gitname && git pull origin msg "The local files are updated." else git clone $_gitroot fi
msg "GIT checkout done or server timeout" msg "Starting make..."
rm -rf "$srcdir/$_gitname-build" git clone "$srcdir/$_gitname" "$srcdir/$_gitname-build" cd "$srcdir/$_gitname-build" --end--
with a "source=(git...)" line. I'm sure that you have covered some edge cases that can not be handled very easily in this fashion and I see some added functionality in your changes. However, I am not sure we need that complexity in 99% of use cases.
Before answering, please allow me a to present my context. Although I'm not an user of either ArchLinux or Pacman for too long (only about a month), I've been srugling with build systems on other distributions. At one moment in time I've even created my own build system in PLT Scheme (now I'm trying to port the ideas over Python). So based on what I've seen a build system has the following responsabilities: * compile-time dependency checking; (like ./configure does, but without the actual configuration;) * build environment preparation; (this includes downloading needed source code, checking it, patching, etc.;) * configuring the build according to the package specification; (invoking ./configure;) * building the package; (make;) * creating a deployable package; Now getting back with the problem where we want to reduce the snipet of code you've highlighted. We can see that the purpose of that code is to "prepare the build environment", more exactly fetching the source code, but we are forced to showell it inside the "build stage" function. So the reason why I've created the patch is because I want to tread source code comming from Git just like any other source-code bundle, without beeing forced to put it inside the build function.
Your patch has pointed out some interesting issues. Especially the issue of checksums and how they are handled when the source is not a real source file.
Allan
So once again thank you for your feedback. Ciprian.
On Mon, Oct 26, 2009 at 3:03 PM, Ciprian Dorin, Craciun <ciprian.craciun@gmail.com> wrote:
So based on what I've seen a build system has the following responsabilities: * compile-time dependency checking; (like ./configure does, but without the actual configuration;) * build environment preparation; (this includes downloading needed source code, checking it, patching, etc.;) * configuring the build according to the package specification; (invoking ./configure;) * building the package; (make;) * creating a deployable package;
Now getting back with the problem where we want to reduce the snipet of code you've highlighted. We can see that the purpose of that code is to "prepare the build environment", more exactly fetching the source code, but we are forced to showell it inside the "build stage" function.
So the reason why I've created the patch is because I want to tread source code comming from Git just like any other source-code bundle, without beeing forced to put it inside the build function.
That's a very good point, and an idea which has been floating for a while would be to introduce a prepare() function. I think this is actually a requirement for makepkg -e to work in many cases, where patches are applied or a scm is used. Then, for limiting code duplication, we could use another idea which has been floating for a while : library functions. http://bugs.archlinux.org/task/10375 Just an example of how this could work : prepare() { source $libdir/scm gitprepare $_gitroot $_gitname }
On Mon, Oct 26, 2009 at 9:15 AM, Xavier <shiningxc@gmail.com> wrote:
On Mon, Oct 26, 2009 at 3:03 PM, Ciprian Dorin, Craciun <ciprian.craciun@gmail.com> wrote:
So based on what I've seen a build system has the following responsabilities: * compile-time dependency checking; (like ./configure does, but without the actual configuration;) * build environment preparation; (this includes downloading needed source code, checking it, patching, etc.;) * configuring the build according to the package specification; (invoking ./configure;) * building the package; (make;) * creating a deployable package;
Now getting back with the problem where we want to reduce the snipet of code you've highlighted. We can see that the purpose of that code is to "prepare the build environment", more exactly fetching the source code, but we are forced to showell it inside the "build stage" function.
So the reason why I've created the patch is because I want to tread source code comming from Git just like any other source-code bundle, without beeing forced to put it inside the build function.
That's a very good point, and an idea which has been floating for a while would be to introduce a prepare() function.
I think this is actually a requirement for makepkg -e to work in many cases, where patches are applied or a scm is used.
Then, for limiting code duplication, we could use another idea which has been floating for a while : library functions. http://bugs.archlinux.org/task/10375
Just an example of how this could work :
prepare() { source $libdir/scm gitprepare $_gitroot $_gitname }
Xavier found the thing I was looking for. I've actually thought about this idea for a while as being a good one, it just needs to be done right as once a function gets out there it is hard to pull it back. I'm a much bigger fan of the prepare() approach (and providing some solid utility functions) as opposed to hacking the source array. While the simple case sometimes works, see this example for a time when a bash function would be a lot easier to use: http://repos.archlinux.org/wsvn/packages/libfetch/repos/core-i686/PKGBUILD We're getting close to the point where a few more standard function names could come into play: things like prepare(), build(), package(), test(). With that said, I like your effort you have put into this. It is an area we don't do the best right now, but providing solid footing for development packages is always a good idea. -Dan
participants (4)
-
Allan McRae
-
Ciprian Dorin, Craciun
-
Dan McGee
-
Xavier