[PATCH 0/2] Build fixes

[PATCH 0/2] Build fixes

[Felipe Contreras]

Felipe Contreras (2):
  build: generate test scripts
  build: do not install git-remote-testgit

 Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
1.8.3.698.g079b096

[PATCH 1/2] build: generate test scripts

[Felipe Contreras]

Commit 416fda6 (build: do not install git-remote-testpy) made it so
git-remote-testpy is not only not installed, but also not generated by
default, let's make sure all the scripts are generated.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 03524d0..126be01 100644
--- a/Makefile
+++ b/Makefile
@@ -531,6 +531,11 @@ SCRIPTS = $(SCRIPT_SH_INS) \
 	  $(SCRIPT_PYTHON_INS) \
 	  git-instaweb
 
+SCRIPTS_GEN = $(SCRIPT_SH_GEN) \
+	  $(SCRIPT_PERL_GEN) \
+	  $(SCRIPT_PYTHON_GEN) \
+	  git-instaweb
+
 ETAGS_TARGET = TAGS
 
 # Empty...
@@ -1647,7 +1652,7 @@ all:: profile-clean
 endif
 endif
 
-all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: $(ALL_PROGRAMS) $(SCRIPTS_GEN) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
 	$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
 endif
-- 
1.8.3.698.g079b096

[PATCH 2/2] build: do not install git-remote-testgit

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 126be01..02e3d43 100644
--- a/Makefile
+++ b/Makefile
@@ -491,6 +491,7 @@ SCRIPT_PERL += git-svn.perl
 SCRIPT_PYTHON += git-remote-testpy.py
 SCRIPT_PYTHON += git-p4.py
 
+NO_INSTALL += git-remote-testgit
 NO_INSTALL += git-remote-testpy
 
 # Generated files for scripts
-- 
1.8.3.698.g079b096

Re: [PATCH 1/2] build: generate test scripts

[Felipe Contreras]

On Fri, Jun 7, 2013 at 5:03 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> -all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
> +all:: $(ALL_PROGRAMS) $(SCRIPTS_GEN) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS

Alternatively, we could add $(NO_INSTALL) here.

-- 
Felipe Contreras

Re: [PATCH 1/2] build: generate test scripts

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Fri, Jun 7, 2013 at 5:03 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
>> -all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
>> +all:: $(ALL_PROGRAMS) $(SCRIPTS_GEN) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
>
> Alternatively, we could add $(NO_INSTALL) here.

As ALL_PROGRAMS overlap with most of SCRIPTS_GEN, the above looks
overly heavy-fisted.  I tend to agree that a separate

	all:: $(NO_INSTALL)

would be much better, assuming that NO_INSTALL will mean "We always
want to build these, but we never do not want to install them"
forever (which I am OK to assume).

Also

	make clean
        make --test=5800 test

did not fail for me, and it turns out that "clean" somehow fails to
clean git-remote-testpy script.

As git-remote-testpy is only for testing, another possibility is to
do

    -all:: $(TEST_PROGRAMS) $(test_bindir_programs)
    +all:: $(TEST_PROGRAMS) $(test_bindir_programs) git-remote-testpy

but I think $(NO_INSTALL) is the cleanest.

Perhaps like this?

 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index a748133..03fda50 100644
--- a/Makefile
+++ b/Makefile
@@ -2239,6 +2239,7 @@ endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
+all:: $(NO_INSTALL)
 all:: $(TEST_PROGRAMS) $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
@@ -2489,7 +2490,7 @@ clean: profile-clean coverage-clean
 	$(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
 		builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
-	$(RM) $(TEST_PROGRAMS)
+	$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
 	$(RM) -r bin-wrappers $(dep_dirs)
 	$(RM) -r po/build/
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*

Re: [PATCH 1/2] build: generate test scripts

[Felipe Contreras]

On Fri, Jun 7, 2013 at 5:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Fri, Jun 7, 2013 at 5:03 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>
>>> -all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
>>> +all:: $(ALL_PROGRAMS) $(SCRIPTS_GEN) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
>>
>> Alternatively, we could add $(NO_INSTALL) here.
>
> As ALL_PROGRAMS overlap with most of SCRIPTS_GEN, the above looks
> overly heavy-fisted.  I tend to agree that a separate
>
>         all:: $(NO_INSTALL)
>
> would be much better, assuming that NO_INSTALL will mean "We always
> want to build these, but we never do not want to install them"
> forever (which I am OK to assume).
>
> Also
>
>         make clean
>         make --test=5800 test
>
> did not fail for me, and it turns out that "clean" somehow fails to
> clean git-remote-testpy script.
>
> As git-remote-testpy is only for testing, another possibility is to
> do
>
>     -all:: $(TEST_PROGRAMS) $(test_bindir_programs)
>     +all:: $(TEST_PROGRAMS) $(test_bindir_programs) git-remote-testpy
>
> but I think $(NO_INSTALL) is the cleanest.
>
> Perhaps like this?
>
>  Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index a748133..03fda50 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2239,6 +2239,7 @@ endif
>
>  test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
>
> +all:: $(NO_INSTALL)
>  all:: $(TEST_PROGRAMS) $(test_bindir_programs)
>
>  bin-wrappers/%: wrap-for-bin.sh
> @@ -2489,7 +2490,7 @@ clean: profile-clean coverage-clean
>         $(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
>                 builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
>         $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
> -       $(RM) $(TEST_PROGRAMS)
> +       $(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
>         $(RM) -r bin-wrappers $(dep_dirs)
>         $(RM) -r po/build/
>         $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*

Looks good to me.

-- 
Felipe Contreras

Re: [PATCH 1/2] build: generate test scripts

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> Perhaps like this?
>>
>>  Makefile | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index a748133..03fda50 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2239,6 +2239,7 @@ endif
>>
>>  test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
>>
>> +all:: $(NO_INSTALL)
>>  all:: $(TEST_PROGRAMS) $(test_bindir_programs)
>>
>>  bin-wrappers/%: wrap-for-bin.sh
>> @@ -2489,7 +2490,7 @@ clean: profile-clean coverage-clean
>>         $(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
>>                 builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
>>         $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
>> -       $(RM) $(TEST_PROGRAMS)
>> +       $(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
>>         $(RM) -r bin-wrappers $(dep_dirs)
>>         $(RM) -r po/build/
>>         $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
>
> Looks good to me.

Actually the above would not work well.  This is because...

>> ..., assuming that NO_INSTALL will mean "We always
>> want to build these, but we never do not want to install them"
>> forever (which I am OK to assume).

... the assumption does *not* hold already with git-remote-testgit,
which is (or will be with patch 2/2) NO_INSTALL and we do not want
to install it, but it is not built, hence we do not want to remove
it, either.

Re: [PATCH 1/2] build: generate test scripts

[Felipe Contreras]

On Fri, Jun 7, 2013 at 5:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> Perhaps like this?
>>>
>>>  Makefile | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Makefile b/Makefile
>>> index a748133..03fda50 100644
>>> --- a/Makefile
>>> +++ b/Makefile
>>> @@ -2239,6 +2239,7 @@ endif
>>>
>>>  test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
>>>
>>> +all:: $(NO_INSTALL)
>>>  all:: $(TEST_PROGRAMS) $(test_bindir_programs)
>>>
>>>  bin-wrappers/%: wrap-for-bin.sh
>>> @@ -2489,7 +2490,7 @@ clean: profile-clean coverage-clean
>>>         $(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
>>>                 builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
>>>         $(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
>>> -       $(RM) $(TEST_PROGRAMS)
>>> +       $(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
>>>         $(RM) -r bin-wrappers $(dep_dirs)
>>>         $(RM) -r po/build/
>>>         $(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
>>
>> Looks good to me.
>
> Actually the above would not work well.  This is because...
>
>>> ..., assuming that NO_INSTALL will mean "We always
>>> want to build these, but we never do not want to install them"
>>> forever (which I am OK to assume).
>
> ... the assumption does *not* hold already with git-remote-testgit,
> which is (or will be with patch 2/2) NO_INSTALL and we do not want
> to install it, but it is not built, hence we do not want to remove
> it, either.

It is generated, in next. If it's not generated, there's no need to
add it to NO_INSTALL.

-- 
Felipe Contreras

Re: [PATCH 1/2] build: generate test scripts

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> It is generated, in next. If it's not generated, there's no need to
> add it to NO_INSTALL.

OK, that makes sense.
Thanks.