Re: [PATCH] Makefile: detect when PYTHON_PATH changes

From: Christian Couder <chriscool@tuxfamily.org>
Date: Dec 15, 2012, 20:09 UTC
Message-ID: <20121215.210950.1929890617364602070.chriscool@tuxfamily.org>
In-Reply-To: <7vehirustv.fsf@alter.siamese.dyndns.org>

From: Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] Makefile: detect when PYTHON_PATH changes
Date: Sat, 15 Dec 2012 09:29:48 -0800

Show 26 quoted lines

> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> @@ -2636,6 +2636,18 @@ GIT-GUI-VARS: FORCE
>>              fi
>>  endif
>>  
>> +### Detect Python interpreter path changes
>> +ifndef NO_PYTHON
>> +TRACK_VARS = $(subst ','\'',-DPYTHON_PATH='$(PYTHON_PATH_SQ)')
>> +
>> +GIT-PYTHON-VARS: FORCE
>> +	@VARS='$(TRACK_VARS)'; \
>> +	    if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
>> +		echo 1>&2 "    * new Python interpreter location"; \
>> +		echo "$$VARS" >$@; \
>> +            fi
>> +endif
> 
> Do we have the same issue when you decide to use /usr/local/bin/sh
> after building with /bin/sh set to SHELL_PATH?
> 
>  - If yes, I presume that there will be follow-up patches to this
>    one, for SHELL_PATH, PERL_PATH, and TCLTK_PATH (there may be
>    other languages but I didn't bother to check).  How would the use
>    of language agnostic looking TRACK_VARS variable in this patch
>    affect such follow-up patches?

Actually, just above the above hunk, there is:

### Detect Tck/Tk interpreter path changes ifndef NO_TCLTK TRACK_VARS = $(subst ','\'',-DTCLTK_PATH='$(TCLTK_PATH_SQ)')

GIT-GUI-VARS: FORCE
        @VARS='$(TRACK_VARS)'; \
            if test x"$$VARS" != x"`cat $@ 2>/dev/null`" ; then \
                echo 1>&2 "    * new Tcl/Tk interpreter location"; \
                echo "$$VARS" >$@; \
            fi
endif

But you are right, TRACK_VARS should probably be something like TRACK_TCLTK when used for TCLTK and TRACK_PYTHON when used for Python.

By the way it looks like GIT-GUI-VARS is not used, so perhaps the detection of Tck/Tk interpreter path changes above could be removed.

(The detection is done in git-gui/Makefile too.)

About shell, there is the following:

SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
        $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
        $(gitwebdir_SQ):$(PERL_PATH_SQ)

and then

GIT-SCRIPT-DEFINES: FORCE
        @FLAGS='$(SCRIPT_DEFINES)'; \
            if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
                echo 1>&2 "    * new script parameters"; \
                echo "$$FLAGS" >$@; \
            fi

$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
        $(QUIET_GEN)$(cmd_munge_script) && \
        chmod +x $@+ && \
        mv $@+ $@

$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
        $(QUIET_GEN)$(cmd_munge_script) && \
        mv $@+ $@

So it looks to me that at least for SHELL_PATH, things are done properly.

>  - If no (in other words, if we rebuild shell-scripted porcelains
>    when SHELL_PATH changes), wouldn't it be better to see how it is
>    done and hook into the same mechanism?

You would like me to just add $(PYTHON_PATH_SQ) in SCRIPT_DEFINES and then use GIT-SCRIPT-DEFINES instead of GIT-PYTHON-VARS to decide if python scripts should be rebuilt?

>  - If no, and if the approach taken in this patch is better than the
>    one used to rebuild shell scripts (it may limit the scope of
>    rebuilding when path changes, or something, but again, I didn't
>    bother to check),

Yeah, I think it is better because it limits the scope of rebuilding, and because nothing is done for Python, while there are some mechanisms in place for other languages.

> then again follow-up patches to this one to
>    describe dependencies to build scripts in other languages in a
>    similar way to this patch would come in the future.  The same
>    question regarding TRACK_VARS applies in this case.

I agree about TRACK_VARS. About other languages, I will have another look, but it seems that they already have what they need.

> By the way, "1>&2" is easier to read if written as just ">&2", I
> think.

Ok I will change this.

Thanks, Christian.