<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> 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 <div style="color: rgb(0, 0, 0);"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: mesa-dev <mesa-dev-bounces@lists.freedesktop.org> on behalf of Brian Paul <brianp@vmware.com> Sent: Saturday, July 1, 2017 00:19 To: mesa-dev@lists.freedesktop.org Subject: Re: [Mesa-dev] Mesa (master): build systems: uniformize git_sha1.h generation <div> </div> </div> </div> <div class="PlainText">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: <a href="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="> 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=</a> > > Author: Eric Engestrom <eric.engestrom@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@imgtec.com> > Reviewed-by: Matt Turner <mattst88@gmail.com> > Reviewed-by: Emil Velikov <emil.velikov@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@lists.freedesktop.org > <a href="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="> 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=</a> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div> </div> </div> </body> </html>