Re: [PATCH] perf tools: Fix build error on read only source.

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Tue, Aug 17, 2010 at 11:17:54AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Aug 17, 2010 at 11:08:40PM +0900, Kusanagi Kouichi escreveu:
> > --- a/tools/perf/feature-tests.mak
> > +++ b/tools/perf/feature-tests.mak
> > @@ -113,7 +113,7 @@ endef
> >  # try-cc
> >  # Usage: option = $(call try-cc, source-to-build, cc-options)
> >  try-cc = $(shell sh -c						  \
> > -	'TMP="$(TMPOUT).$$$$";			 		  \
> > +	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
> >  	 echo "$(1)" |						  \
> >  	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
> >  	 rm -f "$$TMP"')
> 
> Much better! Will test and merge, thanks!

Still has issues:

[acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
[acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -s -j9 -C tools/perf install
PERF_VERSION = 2.6.36.rc1.2.g19bad
    * new build flags or prefix
[acme@emilia linux-2.6-tip]$ patch -p1 < Kouichi.patch 
patching file tools/perf/Makefile
patching file tools/perf/feature-tests.mak
[acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
[acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -s -j9 -C tools/perf install
Makefile:503: No libdw.h found or old libdw.h found or elfutils is older than 0.138, disables dwarf support. Please install new elfutils-devel/libdw-dev
Makefile:534: *** No gnu/libc-version.h found, please install glibc-dev[el]/glibc-static.  Stop.
[acme@emilia linux-2.6-tip]$

Can you please investigate?

Thanks,

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Kusanagi Kouichi]

On 2010-08-17 15:18:35 -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Aug 17, 2010 at 11:17:54AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Aug 17, 2010 at 11:08:40PM +0900, Kusanagi Kouichi escreveu:
> > > --- a/tools/perf/feature-tests.mak
> > > +++ b/tools/perf/feature-tests.mak
> > > @@ -113,7 +113,7 @@ endef
> > >  # try-cc
> > >  # Usage: option = $(call try-cc, source-to-build, cc-options)
> > >  try-cc = $(shell sh -c						  \
> > > -	'TMP="$(TMPOUT).$$$$";			 		  \
> > > +	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
> > >  	 echo "$(1)" |						  \
> > >  	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
> > >  	 rm -f "$$TMP"')
> > 
> > Much better! Will test and merge, thanks!
> 
> Still has issues:
> 
> [acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
> [acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -s -j9 -C tools/perf install
> PERF_VERSION = 2.6.36.rc1.2.g19bad
>     * new build flags or prefix
> [acme@emilia linux-2.6-tip]$ patch -p1 < Kouichi.patch 
> patching file tools/perf/Makefile
> patching file tools/perf/feature-tests.mak
> [acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
> [acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -s -j9 -C tools/perf install
> Makefile:503: No libdw.h found or old libdw.h found or elfutils is older than 0.138, disables dwarf support. Please install new elfutils-devel/libdw-dev
> Makefile:534: *** No gnu/libc-version.h found, please install glibc-dev[el]/glibc-static.  Stop.
> [acme@emilia linux-2.6-tip]$
> 
> Can you please investigate?
> 
> Thanks,
> 
> - Arnaldo

With POSIX shell patch, $(OUTPUT) has not been created yet when it is
used by try-cc. This fixes the issue anyway.

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index dcb9700..5b1c12b 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -281,6 +281,7 @@ endif

 -include feature-tests.mak

+$(shell mkdir -p $(OUTPUT) 2> /dev/null)
 ifeq ($(call try-cc,$(SOURCE_HELLO),-Werror -fstack-protector-all),y)
 	CFLAGS := $(CFLAGS) -fstack-protector-all
 endif

Re: [PATCH] perf tools: Fix build error on read only source.

[Peter Zijlstra]

On Wed, 2010-08-18 at 21:37 +0900, Kusanagi Kouichi wrote:

> With POSIX shell patch, $(OUTPUT) has not been created yet when it is
> used by try-cc. This fixes the issue anyway.
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index dcb9700..5b1c12b 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -281,6 +281,7 @@ endif
> 
>  -include feature-tests.mak
> 
> +$(shell mkdir -p $(OUTPUT) 2> /dev/null)
>  ifeq ($(call try-cc,$(SOURCE_HELLO),-Werror -fstack-protector-all),y)
>  	CFLAGS := $(CFLAGS) -fstack-protector-all
>  endif


So you're wanting to provide a non-existing O=foo ?

I don't think we should, simply bail when it doesn't exist, kernel
builds do the same:

# make O=plop-build bzImage
/bin/sh: line 0: cd: plop-build: No such file or directory
Makefile:117: *** output directory "plop-build" does not exist.  Stop.

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Wed, Aug 18, 2010 at 02:40:24PM +0200, Peter Zijlstra escreveu:
> On Wed, 2010-08-18 at 21:37 +0900, Kusanagi Kouichi wrote:
> 
> > With POSIX shell patch, $(OUTPUT) has not been created yet when it is
> > used by try-cc. This fixes the issue anyway.
> > 
> > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> > index dcb9700..5b1c12b 100644
> > --- a/tools/perf/Makefile
> > +++ b/tools/perf/Makefile
> > @@ -281,6 +281,7 @@ endif
> > 
> >  -include feature-tests.mak
> > 
> > +$(shell mkdir -p $(OUTPUT) 2> /dev/null)
> >  ifeq ($(call try-cc,$(SOURCE_HELLO),-Werror -fstack-protector-all),y)
> >  	CFLAGS := $(CFLAGS) -fstack-protector-all
> >  endif

> So you're wanting to provide a non-existing O=foo ?
> 
> I don't think we should, simply bail when it doesn't exist, kernel
> builds do the same:
> 
> # make O=plop-build bzImage
> /bin/sh: line 0: cd: plop-build: No such file or directory
> Makefile:117: *** output directory "plop-build" does not exist.  Stop.

Agreed that making it work like the kernel brings less surprises, but
isn't it convenient to be able to just do:

rm -rf ~/build
make O=~/build

and have everything work? Saving some keystrokes 8)

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Peter Zijlstra]

On Wed, 2010-08-18 at 11:16 -0300, Arnaldo Carvalho de Melo wrote:
> rm -rf ~/build
> make O=~/build
> 
> and have everything work? Saving some keystrokes 8)

The turn side is that if you typo the output dir you'll probably not
ever find it again.. and you could have typed: rm -rf ~/build/* instead.

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Wed, Aug 18, 2010 at 04:27:45PM +0200, Peter Zijlstra escreveu:
> On Wed, 2010-08-18 at 11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > rm -rf ~/build
> > make O=~/build
> > 
> > and have everything work? Saving some keystrokes 8)
> 
> The turn side is that if you typo the output dir you'll probably not
> ever find it again.. and you could have typed: rm -rf ~/build/* instead.

Not something I'll argue too much :)

I'll apply the patch as it fixes a bug and later make the perf Makefile
complain about the output dir not existing, i.e. providing a message
similar to what the kernel proper does instead of the cryptic message we
get today.

If Kouichi-san or Bernd could provide that patch for perf/core, even
better! ;-)

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Wed, Aug 18, 2010 at 01:56:42PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 18, 2010 at 04:27:45PM +0200, Peter Zijlstra escreveu:
> > On Wed, 2010-08-18 at 11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > > rm -rf ~/build
> > > make O=~/build
> > > 
> > > and have everything work? Saving some keystrokes 8)
> > 
> > The turn side is that if you typo the output dir you'll probably not
> > ever find it again.. and you could have typed: rm -rf ~/build/* instead.
> 
> Not something I'll argue too much :)
> 
> I'll apply the patch as it fixes a bug and later make the perf Makefile
> complain about the output dir not existing, i.e. providing a message
> similar to what the kernel proper does instead of the cryptic message we
> get today.

[acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
[acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -C tools/perf 
make: Entering directory `/home/acme/git/linux-2.6-tip/tools/perf'
Makefile:503: No libdw.h found or old libdw.h found or elfutils is older than 0.138, disables dwarf support. Please install new elfutils-devel/libdw-dev
Makefile:534: *** No gnu/libc-version.h found, please install glibc-dev[el]/glibc-static.  Stop.
make: Leaving directory `/home/acme/git/linux-2.6-tip/tools/perf'
[acme@emilia linux-2.6-tip]$

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Wed, Aug 18, 2010 at 02:01:58PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 18, 2010 at 01:56:42PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Aug 18, 2010 at 04:27:45PM +0200, Peter Zijlstra escreveu:
> > > On Wed, 2010-08-18 at 11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > > > rm -rf ~/build
> > > > make O=~/build
> > > > 
> > > > and have everything work? Saving some keystrokes 8)
> > > 
> > > The turn side is that if you typo the output dir you'll probably not
> > > ever find it again.. and you could have typed: rm -rf ~/build/* instead.
> > 
> > Not something I'll argue too much :)

Ok, here it is, stolen directly from the toplevel kernel Makefile, will push to
Ingo today:

[acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf
[acme@emilia linux-2.6-tip]$ make -j9 O=~/git/build/perf -C tools/perf
/bin/sh: line 0: cd: /home/acme/git/build/perf/: No such file or directory
make: Entering directory `/home/acme/git/linux-2.6-tip/tools/perf'
Makefile:12: *** output directory "/home/acme/git/build/perf/" does not exist.  Stop.
make: Leaving directory `/home/acme/git/linux-2.6-tip/tools/perf'
[acme@emilia linux-2.6-tip]$ mkdir ~/git/build/perf 
[acme@emilia linux-2.6-tip]$ make -j9 O=~/git/build/perf -C tools/perf
make: Entering directory `/home/acme/git/linux-2.6-tip/tools/perf'
    MKDIR /home/acme/git/build/perf/bench
<SNIP>
PERF_VERSION = 2.6.36.rc1.2.g033a273.dirty
make: Leaving directory `/home/acme/git/linux-2.6-tip/tools/perf'
make: Entering directory `/home/acme/git/linux-2.6-tip/tools/perf'
    GEN /home/acme/git/build/perf/common-cmds.h
    * new build flags or prefix
    GEN perf-archive
    CC /home/acme/git/build/perf/builtin-annotate.o
<SNIP>
    AR /home/acme/git/build/perf/libperf.a
    LINK /home/acme/git/build/perf/perf
make: Leaving directory `/home/acme/git/linux-2.6-tip/tools/perf'
[acme@emilia linux-2.6-tip]$

- Arnaldo

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index dcb9700..4f1fa77 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -5,6 +5,12 @@ endif
 # The default target of this Makefile is...
 all::
 
+ifneq ($(OUTPUT),)
+# check that the output directory actually exists
+OUTDIR := $(shell cd $(OUTPUT) && /bin/pwd)
+$(if $(OUTDIR),, $(error output directory "$(OUTPUT)" does not exist))
+endif
+
 # Define V=1 to have a more verbose compile.
 # Define V=2 to have an even more verbose compile.
 #
@@ -931,15 +937,15 @@ $(OUTPUT)common-cmds.h: $(wildcard Documentation/perf-*.txt)
 	$(QUIET_GEN). util/generate-cmdlist.sh > $@+ && mv $@+ $@
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN)$(RM) $(OUTPUT)$@ $(OUTPUT)$@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
 	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 	    -e 's/@@PERF_VERSION@@/$(PERF_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    $@.sh >$@+ && \
-	chmod +x $@+ && \
-	mv $@+ $(OUTPUT)$@
+	    $@.sh > $(OUTPUT)$@+ && \
+	chmod +x $(OUTPUT)$@+ && \
+	mv $(OUTPUT)$@+ $(OUTPUT)$@
 
 configure: configure.ac
 	$(QUIET_GEN)$(RM) $@ $<+ && \
diff --git a/tools/perf/feature-tests.mak b/tools/perf/feature-tests.mak
index ddb68e6..7a7b608 100644
--- a/tools/perf/feature-tests.mak
+++ b/tools/perf/feature-tests.mak
@@ -113,7 +113,7 @@ endef
 # try-cc
 # Usage: option = $(call try-cc, source-to-build, cc-options)
 try-cc = $(shell sh -c						  \
-	'TMP="$(TMPOUT).$$$$";			 		  \
+	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
 	 echo "$(1)" |						  \
 	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
 	 rm -f "$$TMP"')

Re: [PATCH] perf tools: Fix build error on read only source.

[Bernd Petrovitsch]

On Mit, 2010-08-18 at 16:09 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 18, 2010 at 02:01:58PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Aug 18, 2010 at 01:56:42PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Wed, Aug 18, 2010 at 04:27:45PM +0200, Peter Zijlstra escreveu:
> > > > On Wed, 2010-08-18 at 11:16 -0300, Arnaldo Carvalho de Melo wrote:
> > > > > rm -rf ~/build
> > > > > make O=~/build
> > > > > 
> > > > > and have everything work? Saving some keystrokes 8)
> > > > 
> > > > The turn side is that if you typo the output dir you'll probably not
> > > > ever find it again.. and you could have typed: rm -rf ~/build/* instead.

Well, usually interactive shells have a history. But since the other
Makefiles also expect existing destination top directory ...

> > > Not something I'll argue too much :)
> 
> Ok, here it is, stolen directly from the toplevel kernel Makefile, will push to

ACK, it's the same problem so the same solution is the best.

> Ingo today:
> 
> [acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf
> [acme@emilia linux-2.6-tip]$ make -j9 O=~/git/build/perf -C tools/perf
> /bin/sh: line 0: cd: /home/acme/git/build/perf/: No such file or directory

ACK, that should make the "problem" pretty clear.

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Wed, Aug 18, 2010 at 09:37:12PM +0900, Kusanagi Kouichi escreveu:
> On 2010-08-17 15:18:35 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Aug 17, 2010 at 11:17:54AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Aug 17, 2010 at 11:08:40PM +0900, Kusanagi Kouichi escreveu:
> > > > +++ b/tools/perf/feature-tests.mak
> > > > @@ -113,7 +113,7 @@ endef
> > > >  try-cc = $(shell sh -c						  \
> > > > -	'TMP="$(TMPOUT).$$$$";			 		  \
> > > > +	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
> > > >  	 echo "$(1)" |						  \

> > > Much better! Will test and merge, thanks!

> > Still has issues:
> > [acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
> > [acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -s -j9 -C tools/perf install
> > Makefile:503: No libdw.h found or old libdw.h found or elfutils is older than 0.138, disables dwarf support. Please install new elfutils-devel/libdw-dev
> > Makefile:534: *** No gnu/libc-version.h found, please install glibc-dev[el]/glibc-static.  Stop.
> > [acme@emilia linux-2.6-tip]$

> > Can you please investigate?
> 
> With POSIX shell patch, $(OUTPUT) has not been created yet when it is
> used by try-cc. This fixes the issue anyway.
> 
> diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> index dcb9700..5b1c12b 100644
> --- a/tools/perf/Makefile
> +++ b/tools/perf/Makefile
> @@ -281,6 +281,7 @@ endif
> 
>  -include feature-tests.mak
> 
> +$(shell mkdir -p $(OUTPUT) 2> /dev/null)

But with this we're back using $(shell mkdir), Bernd, ideas on how to
properly fix this, probably the feature tests have to be triggered by
the first rule and probably be something like:

$(phony feature-tests): 
-include feature-tests.mak

DIRECTORY_DEPS = $(LIB_OBJS) $(BUILTIN_OBJS) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)common-cmds.h
$(DIRECTORY_DEPS): $(sort $(dir $(DIRECTORY_DEPS))) feature-tests

completely untested! Unsure if this would trigger the mkdir first tho.

Right?

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Bernd Petrovitsch]

On Mit, 2010-08-18 at 11:14 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 18, 2010 at 09:37:12PM +0900, Kusanagi Kouichi escreveu:
> > On 2010-08-17 15:18:35 -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Tue, Aug 17, 2010 at 11:17:54AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Tue, Aug 17, 2010 at 11:08:40PM +0900, Kusanagi Kouichi escreveu:
> > > > > +++ b/tools/perf/feature-tests.mak
> > > > > @@ -113,7 +113,7 @@ endef
> > > > >  try-cc = $(shell sh -c						  \
> > > > > -	'TMP="$(TMPOUT).$$$$";			 		  \
> > > > > +	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
> > > > >  	 echo "$(1)" |						  \
> 
> > > > Much better! Will test and merge, thanks!
> 
> > > Still has issues:
> > > [acme@emilia linux-2.6-tip]$ rm -rf ~/git/build/perf/
> > > [acme@emilia linux-2.6-tip]$ make O=~/git/build/perf -s -j9 -C tools/perf install
> > > Makefile:503: No libdw.h found or old libdw.h found or elfutils is older than 0.138, disables dwarf support. Please install new elfutils-devel/libdw-dev
> > > Makefile:534: *** No gnu/libc-version.h found, please install glibc-dev[el]/glibc-static.  Stop.
> > > [acme@emilia linux-2.6-tip]$
> 
> > > Can you please investigate?
> > 
> > With POSIX shell patch, $(OUTPUT) has not been created yet when it is
> > used by try-cc. This fixes the issue anyway.
> > 
> > diff --git a/tools/perf/Makefile b/tools/perf/Makefile
> > index dcb9700..5b1c12b 100644
> > --- a/tools/perf/Makefile
> > +++ b/tools/perf/Makefile
> > @@ -281,6 +281,7 @@ endif
> > 
> >  -include feature-tests.mak
> > 
> > +$(shell mkdir -p $(OUTPUT) 2> /dev/null)
> 
> But with this we're back using $(shell mkdir), Bernd, ideas on how to
> properly fix this, probably the feature tests have to be triggered by
> the first rule and probably be something like:
> 
> $(phony feature-tests): 
First, this should probably have been:
.PHONY: feature-tests

> -include feature-tests.mak
> 
> DIRECTORY_DEPS = $(LIB_OBJS) $(BUILTIN_OBJS) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)common-cmds.h
> $(DIRECTORY_DEPS): $(sort $(dir $(DIRECTORY_DEPS))) feature-tests
>
> completely untested! Unsure if this would trigger the mkdir first tho.
>
> Right?

This doesn't trigger it. I played around a little bit and didn't find
any way to trigger any rule.
The "problem" is IMHO that "try-cc" is used in "ifeq" clauses and these
are evaluated as `make` goes through the Makefile before the first
(real) rule (like "all") is triggered.

So ATM I see only 3 possibilities:
1) The old (aka ugly) "solution" from above.
2) Add to try-cc something like
	[ -d "$(OUTPUT)" ] || mkdir -p "$(OUTPUT)" 2>/dev/null;
before the line with the "echo".
The '[ -d "$(OUTPUT)" ]' is not really necessary as `mkdir -p` is a
no-op on existing directories.
3) Another possibility would be to move the temporary file into the /tmp
directory as into
----  snip  ----
try-cc = $(shell sh -c						  \
	'TMP="/tmp/$(TMPOUT).$$$$";		 		  \
	 echo "$(1)" |						  \
	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
	 rm -f "$$TMP"')
----  snip  ----
If yes, we could use (and thus depend on) `mktemp` - which is usual
shell script way to generate unique temporary filename - as in
----  snip  ----
try-cc = $(shell sh -c   \
	'TMP="$$(mktemp)";   \
	 echo "$(1)" |   \
	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
	 rm -f "$$TMP"')
----  snip  ----

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Wed, Aug 18, 2010 at 05:25:41PM +0200, Bernd Petrovitsch escreveu:
> On Mit, 2010-08-18 at 11:14 -0300, Arnaldo Carvalho de Melo wrote:
> > But with this we're back using $(shell mkdir), Bernd, ideas on how to
> > properly fix this, probably the feature tests have to be triggered by
> > the first rule and probably be something like:
> > 
> > $(phony feature-tests): 
> First, this should probably have been:
> .PHONY: feature-tests

Humm, isn't $(phony feature-tests): variation valid and shorter, humm,
yeah, seems to be for !GNU make :-\

(http://makepp.sourceforge.net/1.18/t_phony.html)

> > -include feature-tests.mak
> > 
> > DIRECTORY_DEPS = $(LIB_OBJS) $(BUILTIN_OBJS) $(OUTPUT)PERF-VERSION-FILE $(OUTPUT)common-cmds.h
> > $(DIRECTORY_DEPS): $(sort $(dir $(DIRECTORY_DEPS))) feature-tests
> >
> > completely untested! Unsure if this would trigger the mkdir first tho.
> >
> > Right?
> 
> This doesn't trigger it. I played around a little bit and didn't find
> any way to trigger any rule.
> The "problem" is IMHO that "try-cc" is used in "ifeq" clauses and these
> are evaluated as `make` goes through the Makefile before the first
> (real) rule (like "all") is triggered.
> 
> So ATM I see only 3 possibilities:
> 1) The old (aka ugly) "solution" from above.
> 2) Add to try-cc something like
> 	[ -d "$(OUTPUT)" ] || mkdir -p "$(OUTPUT)" 2>/dev/null;
> before the line with the "echo".
> The '[ -d "$(OUTPUT)" ]' is not really necessary as `mkdir -p` is a
> no-op on existing directories.

Probably 2) is the best, since we'll end up creating that dir anyway.
But as PeterZ pointed out, this is all moot since the kernel behaviour
for O= is to expect the directory specified to already exist.

We're left with the task of checking that and providing a meaningful
answer if not, like this:

[acme@emilia linux-2.6-tip]$ ls -la ~/git/build/foo
ls: cannot access /home/acme/git/build/foo: No such file or directory
[acme@emilia linux-2.6-tip]$ make O=~/git/build/foo
/bin/sh: line 0: cd: /home/acme/git/build/foo: No such file or directory
Makefile:117: *** output directory "/home/acme/git/build/foo" does not exist.  Stop.
[acme@emilia linux-2.6-tip]$

> 3) Another possibility would be to move the temporary file into the /tmp
> directory as into
> ----  snip  ----
> try-cc = $(shell sh -c						  \
> 	'TMP="/tmp/$(TMPOUT).$$$$";		 		  \
> 	 echo "$(1)" |						  \
> 	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
> 	 rm -f "$$TMP"')
> ----  snip  ----
> If yes, we could use (and thus depend on) `mktemp` - which is usual
> shell script way to generate unique temporary filename - as in

There is no use of mktemp in Kbuild, that I try hard to use as reference
here.

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Bernd Petrovitsch]

On Mit, 2010-08-18 at 14:19 -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Aug 18, 2010 at 05:25:41PM +0200, Bernd Petrovitsch escreveu:
> > On Mit, 2010-08-18 at 11:14 -0300, Arnaldo Carvalho de Melo wrote:
[...]
> > > $(phony feature-tests): 
> > First, this should probably have been:
> > .PHONY: feature-tests
> 
> Humm, isn't $(phony feature-tests): variation valid and shorter, humm,
> yeah, seems to be for !GNU make :-\
> 
> (http://makepp.sourceforge.net/1.18/t_phony.html)

Funny, I didn't knew GNU-make's "$(phony target): " construct.

Hmm, what GNU-make versions and other `make`s are considered (or should
be) supported?

[...]
> There is no use of mktemp in Kbuild, that I try hard to use as reference
> here.

FWIW fine with me - I just threw an idea in.

	Bernd
-- 
mobile: +43 664 4416156              http://www.sysprog.at/
    Linux Software Development, Consulting and Services

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Thu, Aug 19, 2010 at 12:31:48PM +0200, Bernd Petrovitsch escreveu:
> On Mit, 2010-08-18 at 14:19 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Aug 18, 2010 at 05:25:41PM +0200, Bernd Petrovitsch escreveu:
> > > On Mit, 2010-08-18 at 11:14 -0300, Arnaldo Carvalho de Melo wrote:
> [...]
> > > > $(phony feature-tests): 
> > > First, this should probably have been:
> > > .PHONY: feature-tests
> > 
> > Humm, isn't $(phony feature-tests): variation valid and shorter, humm,
> > yeah, seems to be for !GNU make :-\
> > 
> > (http://makepp.sourceforge.net/1.18/t_phony.html)
> 
> Funny, I didn't knew GNU-make's "$(phony target): " construct.

it doesn't have AFAIK, its just this makepp, that I never had heard
about before, that has it, probably it introduced the idea. Quite
sensible, it seems.
 
> Hmm, what GNU-make versions and other `make`s are considered (or should
> be) supported?

I think only GNU make is supported.
 
> [...]
> > There is no use of mktemp in Kbuild, that I try hard to use as reference
> > here.
> 
> FWIW fine with me - I just threw an idea in.

Right, and I considered it, using Kbuild usage as a validation gauge to
say we probably shouldn't use it :)

- Arnaldo

[PATCH] perf tools: Fix build error on read only source.

[Kusanagi Kouichi]

Signed-off-by: Kusanagi Kouichi <slash@ac.auone-net.jp>
---
 tools/perf/Makefile          |    8 ++++----
 tools/perf/feature-tests.mak |    6 ++----
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 41abb90..31b528f 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -935,15 +935,15 @@ $(OUTPUT)common-cmds.h: $(wildcard Documentation/perf-*.txt)
 	$(QUIET_GEN). util/generate-cmdlist.sh > $@+ && mv $@+ $@
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN)$(RM) $(OUTPUT)$@ $(OUTPUT)$@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
 	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 	    -e 's/@@PERF_VERSION@@/$(PERF_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    $@.sh >$@+ && \
-	chmod +x $@+ && \
-	mv $@+ $(OUTPUT)$@
+	    $@.sh > $(OUTPUT)$@+ && \
+	chmod +x $(OUTPUT)$@+ && \
+	mv $(OUTPUT)$@+ $(OUTPUT)$@
 
 configure: configure.ac
 	$(QUIET_GEN)$(RM) $@ $<+ && \
diff --git a/tools/perf/feature-tests.mak b/tools/perf/feature-tests.mak
index ddb68e6..cd75798 100644
--- a/tools/perf/feature-tests.mak
+++ b/tools/perf/feature-tests.mak
@@ -113,7 +113,5 @@ endef
 # try-cc
 # Usage: option = $(call try-cc, source-to-build, cc-options)
 try-cc = $(shell sh -c						  \
-	'TMP="$(TMPOUT).$$$$";			 		  \
-	 echo "$(1)" |						  \
-	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
-	 rm -f "$$TMP"')
+	'echo "$(1)" |						  \
+	 $(CC) -x c - $(2) -o /dev/null > /dev/null 2>&1 && echo y')
-- 
1.7.1

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Mon, Aug 16, 2010 at 09:40:13PM +0900, Kusanagi Kouichi escreveu:
... 
> diff --git a/tools/perf/feature-tests.mak b/tools/perf/feature-tests.mak
> @@ -113,7 +113,5 @@ endef
>  # try-cc
>  # Usage: option = $(call try-cc, source-to-build, cc-options)
>  try-cc = $(shell sh -c						  \
> -	'TMP="$(TMPOUT).$$$$";			 		  \
> -	 echo "$(1)" |						  \
> -	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
> -	 rm -f "$$TMP"')
> +	'echo "$(1)" |						  \
> +	 $(CC) -x c - $(2) -o /dev/null > /dev/null 2>&1 && echo y')

Please read:

 f9af3a4c1f59753bdd5a49e3a34263005f96972e
 8b2c551f9635bf1c5c2d38de300137998915478f
 1703f2c321a8a531c393e137a82602e16c6061cb

To see why we can't use '-o /dev/null' like that.

- Arnaldo

Re: [PATCH] perf tools: Fix build error on read only source.

[Kusanagi Kouichi]

On 2010-08-16 12:24:10 -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 16, 2010 at 09:40:13PM +0900, Kusanagi Kouichi escreveu:
> ... 
> > diff --git a/tools/perf/feature-tests.mak b/tools/perf/feature-tests.mak
> > @@ -113,7 +113,5 @@ endef
> >  # try-cc
> >  # Usage: option = $(call try-cc, source-to-build, cc-options)
> >  try-cc = $(shell sh -c						  \
> > -	'TMP="$(TMPOUT).$$$$";			 		  \
> > -	 echo "$(1)" |						  \
> > -	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
> > -	 rm -f "$$TMP"')
> > +	'echo "$(1)" |						  \
> > +	 $(CC) -x c - $(2) -o /dev/null > /dev/null 2>&1 && echo y')
> 
> Please read:
> 
>  f9af3a4c1f59753bdd5a49e3a34263005f96972e
>  8b2c551f9635bf1c5c2d38de300137998915478f
>  1703f2c321a8a531c393e137a82602e16c6061cb
> 
> To see why we can't use '-o /dev/null' like that.
> 
> - Arnaldo

I see. I will try another way.

[PATCH] perf tools: Fix build error on read only source.

[Kusanagi Kouichi]

Signed-off-by: Kusanagi Kouichi <slash@ac.auone-net.jp>
---
 tools/perf/Makefile          |    8 ++++----
 tools/perf/feature-tests.mak |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 41abb90..31b528f 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -935,15 +935,15 @@ $(OUTPUT)common-cmds.h: $(wildcard Documentation/perf-*.txt)
 	$(QUIET_GEN). util/generate-cmdlist.sh > $@+ && mv $@+ $@
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
-	$(QUIET_GEN)$(RM) $@ $@+ && \
+	$(QUIET_GEN)$(RM) $(OUTPUT)$@ $(OUTPUT)$@+ && \
 	sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
 	    -e 's|@@PERL@@|$(PERL_PATH_SQ)|g' \
 	    -e 's/@@PERF_VERSION@@/$(PERF_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
-	    $@.sh >$@+ && \
-	chmod +x $@+ && \
-	mv $@+ $(OUTPUT)$@
+	    $@.sh > $(OUTPUT)$@+ && \
+	chmod +x $(OUTPUT)$@+ && \
+	mv $(OUTPUT)$@+ $(OUTPUT)$@
 
 configure: configure.ac
 	$(QUIET_GEN)$(RM) $@ $<+ && \
diff --git a/tools/perf/feature-tests.mak b/tools/perf/feature-tests.mak
index ddb68e6..7a7b608 100644
--- a/tools/perf/feature-tests.mak
+++ b/tools/perf/feature-tests.mak
@@ -113,7 +113,7 @@ endef
 # try-cc
 # Usage: option = $(call try-cc, source-to-build, cc-options)
 try-cc = $(shell sh -c						  \
-	'TMP="$(TMPOUT).$$$$";			 		  \
+	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
 	 echo "$(1)" |						  \
 	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
 	 rm -f "$$TMP"')
-- 
1.7.1

Re: [PATCH] perf tools: Fix build error on read only source.

[Arnaldo Carvalho de Melo]

Em Tue, Aug 17, 2010 at 11:08:40PM +0900, Kusanagi Kouichi escreveu:
> --- a/tools/perf/feature-tests.mak
> +++ b/tools/perf/feature-tests.mak
> @@ -113,7 +113,7 @@ endef
>  # try-cc
>  # Usage: option = $(call try-cc, source-to-build, cc-options)
>  try-cc = $(shell sh -c						  \
> -	'TMP="$(TMPOUT).$$$$";			 		  \
> +	'TMP="$(OUTPUT)$(TMPOUT).$$$$";				  \
>  	 echo "$(1)" |						  \
>  	 $(CC) -x c - $(2) -o "$$TMP" > /dev/null 2>&1 && echo y; \
>  	 rm -f "$$TMP"')

Much better! Will test and merge, thanks!

- Arnaldo