[Mesa-dev] Thou shalt not use shell scripts on Windows (Was: Mesa (master): build systems: uniformize git_sha1.h generation)
Jose Fonseca
jfonseca at vmware.com
Tue Aug 1 13:04:33 UTC 2017
Sorry for being late to the party - I was on PTO when this review went out. Only now I was trying to diagnose why some of our builds were broken for very long time, and tracked down to this.
Let me start stating what I think it should be obvious for everybody: using a shell script on Windows is a awful awful idea. It's not admissible. That's the whole reason we use SCons instead of autotools and stuff.
The only reason this didn't break things completely for us is that Git for windows often bundles sh.exe and some times it's left in the Path. But that's a coincidence, which in fact creates problems of its own.
If you truly want to uniformize git_sha1.h generation then one must use cross platform scripting language as Python. Like all other scripts we have!
Honestly, I don't know how this pass through peer review unchallenged... It should have been stopped before it ever reached master.
Jose
________________________________
From: mesa-dev <mesa-dev-bounces at lists.freedesktop.org> on behalf of Brian Paul <brianp at vmware.com>
Sent: Saturday, July 1, 2017 00:19
To: mesa-dev at lists.freedesktop.org
Subject: Re: [Mesa-dev] Mesa (master): build systems: uniformize git_sha1.h generation
Hi Eric,
Shouldn't the new script file be put in the bin/ directory with the
other helper scripts?
-Brian
On 06/29/2017 09:52 AM, Eric Engeström wrote:
> Module: Mesa
> Branch: master
> Commit: 3fd425aed764fb771f2f49ddb6b30b389a114504
> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__cgit.freedesktop.org_mesa_mesa_commit_-3Fid-3D3fd425aed764fb771f2f49ddb6b30b389a114504&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=Ie7_encNUsqxbSRbqbNgofw0ITcfE8JKfaUjIQhncGA&m=C2QHFlT8FYNq-A29CoLU1hnnK3jQhP0A-EpQrqbU_GQ&s=ZjxcahOcytLNOgFBZuod2dWN2xvP8Ph32iP3dluOunQ&e=
>
> Author: Eric Engestrom <eric.engestrom at imgtec.com>
> Date: Tue Jun 27 12:08:41 2017 +0100
>
> build systems: uniformize git_sha1.h generation
>
> Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>
> Reviewed-by: Matt Turner <mattst88 at gmail.com>
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com>
>
> ---
>
> git_sha1_gen.sh | 12 ++++++++++++
> src/Makefile.am | 13 +------------
> src/SConscript | 28 ++++++++--------------------
> src/mesa/Android.libmesa_git_sha1.mk | 7 +------
> 4 files changed, 22 insertions(+), 38 deletions(-)
>
> diff --git a/git_sha1_gen.sh b/git_sha1_gen.sh
> new file mode 100755
> index 0000000000..20ab8df8ea
> --- /dev/null
> +++ b/git_sha1_gen.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +
> +# run git from the sources directory
> +cd "$(dirname "$0")"
> +
> +# don't print anything if git fails
> +if ! git_sha1=$(git --git-dir=.git rev-parse --short=10 HEAD 2>/dev/null)
> +then
> + exit
> +fi
> +
> +printf '#define MESA_GIT_SHA1 "git-%s"\n' "$git_sha1"
> diff --git a/src/Makefile.am b/src/Makefile.am
> index df912c442a..d8a2ee59fc 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -21,18 +21,7 @@
>
> .PHONY: git_sha1.h.tmp
> git_sha1.h.tmp:
> - @# Don't assume that $(top_srcdir)/.git is a directory. It may be
> - @# a gitlink file if $(top_srcdir) is a submodule checkout or a linked
> - @# worktree.
> - @# If we are building from a release tarball copy the bundled header.
> - @touch git_sha1.h.tmp
> - @if test -e $(top_srcdir)/.git; then \
> - if which git > /dev/null; then \
> - printf '#define MESA_GIT_SHA1 "git-%s"\n' \
> - `git --git-dir=$(top_srcdir)/.git rev-parse --short=10 HEAD` \
> - > git_sha1.h.tmp ; \
> - fi \
> - fi
> + @sh $(top_srcdir)/git_sha1_gen.sh > $@
>
> git_sha1.h: git_sha1.h.tmp
> @echo "updating git_sha1.h"
> diff --git a/src/SConscript b/src/SConscript
> index d861af8e4d..5e1171b524 100644
> --- a/src/SConscript
> +++ b/src/SConscript
> @@ -22,27 +22,15 @@ def write_git_sha1_h_file(filename):
> to retrieve the git hashid and write the header file. An empty file
> will be created if anything goes wrong."""
>
> - args = [ 'git', 'rev-parse', '--short=10', 'HEAD' ]
> - try:
> - (commit, foo) = subprocess.Popen(args, stdout=subprocess.PIPE).communicate()
> - except:
> - print "Warning: exception in write_git_sha1_h_file()"
> - # git log command didn't work
> - if not os.path.exists(filename):
> - dirname = os.path.dirname(filename)
> - if dirname and not os.path.exists(dirname):
> - os.makedirs(dirname)
> - # create an empty file if none already exists
> - f = open(filename, "w")
> - f.close()
> - return
> -
> - # note that commit[:-1] removes the trailing newline character
> - commit = '#define MESA_GIT_SHA1 "git-%s"\n' % commit[:-1]
> tempfile = "git_sha1.h.tmp"
> - f = open(tempfile, "w")
> - f.write(commit)
> - f.close()
> + with open(tempfile, "w") as f:
> + args = [ 'sh', Dir('#').abspath + '/git_sha1_gen.sh' ]
> + try:
> + subprocess.Popen(args, stdout=f)
> + except:
> + print "Warning: exception in write_git_sha1_h_file()"
> + return
> +
> if not os.path.exists(filename) or not filecmp.cmp(tempfile, filename):
> # The filename does not exist or it's different from the new file,
> # so replace old file with new.
> diff --git a/src/mesa/Android.libmesa_git_sha1.mk b/src/mesa/Android.libmesa_git_sha1.mk
> index 0fd176bf7d..a5a1ebb37f 100644
> --- a/src/mesa/Android.libmesa_git_sha1.mk
> +++ b/src/mesa/Android.libmesa_git_sha1.mk
> @@ -46,12 +46,7 @@ LOCAL_GENERATED_SOURCES += $(addprefix $(intermediates)/, git_sha1.h)
> $(intermediates)/git_sha1.h: $(wildcard $(MESA_TOP)/.git/logs/HEAD)
> @mkdir -p $(dir $@)
> @echo "GIT-SHA1: $(PRIVATE_MODULE) <= git"
> - $(hide) touch $@
> - $(hide) if which git > /dev/null; then \
> - git --git-dir $(MESA_TOP)/.git log -n 1 --oneline | \
> - sed 's/^\([^ ]*\) .*/#define MESA_GIT_SHA1 "git-\1"/' \
> - > $@; \
> - fi
> + $(hide) sh $(MESA_TOP)/git_sha1_gen.sh > $@
>
> LOCAL_EXPORT_C_INCLUDE_DIRS := $(intermediates)
>
>
> _______________________________________________
> mesa-commit mailing list
> mesa-commit at lists.freedesktop.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Dcommit&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=Ie7_encNUsqxbSRbqbNgofw0ITcfE8JKfaUjIQhncGA&m=C2QHFlT8FYNq-A29CoLU1hnnK3jQhP0A-EpQrqbU_GQ&s=W6LuBpaa2ybCgFRk5UR-YrT1lr2rk0XsV8K6c6Ii1AI&e=
>
_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170801/82921138/attachment-0001.html>
More information about the mesa-dev
mailing list