Re: [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 
> 
> Quoting Eric Engestrom (2017-10-26 08:40:56)
> > Suggested-by: Jordan Justen 
> > Signed-off-by: Eric Engestrom 
> > ---
> >  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 += 

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

[Jordan Justen]

Both Reviewed-by: Jordan Justen 

Cc: Dylan

On 2017-10-26 08:40:56, Eric Engestrom wrote:
> Suggested-by: Jordan Justen 
> Signed-off-by: Eric Engestrom 
> ---
>  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]
> +try:
> +subprocess.Popen(args).wait()
> +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
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Dylan Baker]

I have a few tiny nits, but otherwise this seems fine:
Reviewed-by: Dylan Baker 

Quoting Eric Engestrom (2017-10-26 08:40:56)
> Suggested-by: Jordan Justen 
> Signed-off-by: Eric Engestrom 
> ---
>  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

> +try:
> +subprocess.Popen(args).wait()

how about subprocess.call() instead of Popen().wait()?

> +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 :=