[Mesa-dev] [PATCH mesa 1/2] buildsys: move file regeneration logic to the script itself

[Eric Engestrom]

On Thursday, 2017-10-26 10:10:19 -0700, Dylan Baker wrote:
> I have a few tiny nits, but otherwise this seems fine:
> Reviewed-by: Dylan Baker <dylan at pnwbakers.com>
> 
> Quoting Eric Engestrom (2017-10-26 08:40:56)
> > Suggested-by: Jordan Justen <jordan.l.justen at intel.com>
> > Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>
> > ---
> >  bin/git_sha1_gen.py                  | 13 ++++++++++++-
> >  src/Makefile.am                      | 15 ++++-----------
> >  src/SConscript                       | 22 ++++++----------------
> >  src/mesa/Android.libmesa_git_sha1.mk |  4 ++--
> >  4 files changed, 24 insertions(+), 30 deletions(-)
> > 
> > diff --git a/bin/git_sha1_gen.py b/bin/git_sha1_gen.py
> > index c75dba101acf4ffc85fb..7b9267b59e9d1d897cc4 100755
> > --- a/bin/git_sha1_gen.py
> > +++ b/bin/git_sha1_gen.py
> > @@ -6,6 +6,7 @@
> >  """
> >  
> >  
> > +import argparse
> >  import os
> >  import os.path
> >  import subprocess
> > @@ -27,10 +28,20 @@ def get_git_sha1():
> >          git_sha1 = ''
> >      return git_sha1
> >  
> > +parser = argparse.ArgumentParser()
> > +parser.add_argument('--output', help='File to write the #define in',
> > +        required=True)
> > +args = parser.parse_args()
> >  
> >  git_sha1 = os.environ.get('MESA_GIT_SHA1_OVERRIDE', get_git_sha1())[:10]
> >  if git_sha1:
> >      git_sha1_h_in_path = os.path.join(os.path.dirname(sys.argv[0]),
> >              '..', 'src', 'git_sha1.h.in')
> >      with open(git_sha1_h_in_path , 'r') as git_sha1_h_in:
> > -        sys.stdout.write(git_sha1_h_in.read().replace('@VCS_TAG@', git_sha1))
> > +        new_sha1 = git_sha1_h_in.read().replace('@VCS_TAG@', git_sha1)
> > +        if os.path.isfile(args.output):
> > +            with open(args.output, 'r') as git_sha1_h:
> > +                if git_sha1_h.read() == new_sha1:
> > +                    quit()
> > +        with open(args.output, 'w') as git_sha1_h:
> > +            git_sha1_h.write(new_sha1)
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 5ef2d4f55eacc470a7a9..1de4fca6a1216e7662b1 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -19,17 +19,10 @@
> >  # FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >  # IN THE SOFTWARE.
> >  
> > -.PHONY: git_sha1.h.tmp
> > -git_sha1.h.tmp:
> > -       @$(PYTHON2) $(top_srcdir)/bin/git_sha1_gen.py > $@
> > -
> > -git_sha1.h: git_sha1.h.tmp
> > -       @echo "updating git_sha1.h"
> > -       @if ! cmp -s git_sha1.h.tmp git_sha1.h; then \
> > -               mv git_sha1.h.tmp git_sha1.h ;\
> > -       else \
> > -               rm git_sha1.h.tmp ;\
> > -       fi
> > +.PHONY: git_sha1.h
> > +git_sha1.h: $(top_srcdir)/src/git_sha1.h.in
> > +       @echo "updating $@"
> > +       @$(PYTHON2) $(top_srcdir)/bin/git_sha1_gen.py --output $@
> >  
> >  BUILT_SOURCES = git_sha1.h
> >  CLEANFILES = $(BUILT_SOURCES)
> > diff --git a/src/SConscript b/src/SConscript
> > index a277e8b79254588e8fd4..820c3c01289e7544a57e 100644
> > --- a/src/SConscript
> > +++ b/src/SConscript
> > @@ -24,22 +24,12 @@ 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."""
> >  
> > -    tempfile = "git_sha1.h.tmp"
> > -    with open(tempfile, "w") as f:
> > -        args = [ python_cmd, Dir('#').abspath + '/bin/git_sha1_gen.py' ]
> > -        try:
> > -            subprocess.Popen(args, stdout=f).wait()
> > -        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.
> > -        if os.path.exists(filename):
> > -            os.remove(filename)
> > -        os.rename(tempfile, filename)
> > -    return
> > +    args = [ python_cmd, Dir('#').abspath + '/bin/git_sha1_gen.py', '--output', filename]
> 
> space after the opening brace but none before the closing. I don't konw what the
> scons style is, but you should probably be consistent

scons style is to have spaces inside [], which I added back now.
thanks for spotting that :)

> 
> > +    try:
> > +        subprocess.Popen(args).wait()
> 
> how about subprocess.call() instead of Popen().wait()?

indeed, Popen() is not needed anymore.
replaced with call(), tested and pushed :)

> 
> > +    except:
> > +        print("Warning: exception in write_git_sha1_h_file()")
> > +        return
> >  
> >  
> >  # Create the git_sha1.h header file
> > diff --git a/src/mesa/Android.libmesa_git_sha1.mk b/src/mesa/Android.libmesa_git_sha1.mk
> > index ddb30c428af739ee9fe5..d27923074dd289a115de 100644
> > --- a/src/mesa/Android.libmesa_git_sha1.mk
> > +++ b/src/mesa/Android.libmesa_git_sha1.mk
> > @@ -43,10 +43,10 @@ $(intermediates)/dummy.c:
> >  
> >  LOCAL_GENERATED_SOURCES += $(addprefix $(intermediates)/, git_sha1.h)
> >  
> > -$(intermediates)/git_sha1.h: $(wildcard $(MESA_TOP)/.git/logs/HEAD)
> > +$(intermediates)/git_sha1.h: $(MESA_TOP)/src/git_sha1.h.in $(wildcard $(MESA_TOP)/.git/logs/HEAD)
> >         @mkdir -p $(dir $@)
> >         @echo "GIT-SHA1: $(PRIVATE_MODULE) <= git"
> > -       $(hide) $(MESA_PYTHON2) $(MESA_TOP)/bin/git_sha1_gen.py > $@
> > +       $(hide) $(MESA_PYTHON2) $(MESA_TOP)/bin/git_sha1_gen.py --output $@
> >  
> >  LOCAL_EXPORT_C_INCLUDE_DIRS := $(intermediates)
> >  
> > -- 
> > Cheers,
> >   Eric
> >