Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Jiri Olsa]

On Thu, Jun 18, 2015 at 01:00:32PM +0200, Lukas Wunner wrote:
> Invoking Makefile.perf with prefix= breaks the build since Makefile.perf
> hands that variable down to Makefile.build where it overrides
>     prefix       := $(subst ./,,$(OUTPUT)$(dir)/)
> 
> leading to errors like this:
>     No rule to make target '/usrabspath.o', needed by '/usrlibperf-in.o'

hum, what specific make command is failing?

jirka

> 
> Fixes: c819e2cf2eb6f65d3208d195d7a0edef6108d5
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  tools/build/Makefile.build | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build
> index 10df572..98cfc38 100644
> --- a/tools/build/Makefile.build
> +++ b/tools/build/Makefile.build
> @@ -94,12 +94,12 @@ obj-y        := $(patsubst %/, %/$(obj)-in.o, $(obj-y))
>  subdir-obj-y := $(filter %/$(obj)-in.o, $(obj-y))
>  
>  # '$(OUTPUT)/dir' prefix to all objects
> -prefix       := $(subst ./,,$(OUTPUT)$(dir)/)
> -obj-y        := $(addprefix $(prefix),$(obj-y))
> -subdir-obj-y := $(addprefix $(prefix),$(subdir-obj-y))
> +objprefix    := $(subst ./,,$(OUTPUT)$(dir)/)
> +obj-y        := $(addprefix $(objprefix),$(obj-y))
> +subdir-obj-y := $(addprefix $(objprefix),$(subdir-obj-y))
>  
>  # Final '$(obj)-in.o' object
> -in-target := $(prefix)$(obj)-in.o
> +in-target := $(objprefix)$(obj)-in.o
>  
>  PHONY += $(subdir-y)
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Lukas Wunner]

Hi Jirka,

On Thu, Jun 18, 2015 at 09:29:32PM +0200, Jiri Olsa wrote:
> On Thu, Jun 18, 2015 at 01:00:32PM +0200, Lukas Wunner wrote:
> > Invoking Makefile.perf with prefix= breaks the build since Makefile.perf
> > hands that variable down to Makefile.build where it overrides
> >     prefix       := $(subst ./,,$(OUTPUT)$(dir)/)
> > 
> > leading to errors like this:
> >     No rule to make target '/usrabspath.o', needed by '/usrlibperf-in.o'
> hum, what specific make command is failing?

Makefile.perf may be invoked with a "prefix" parameter:

	@echo '  HINT: use "prefix" or "DESTDIR" to install to a particular'
	@echo '        path like "make prefix=/usr/local install install-doc"'

Source: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/tools/perf/Makefile.perf#n414


E.g. the Debian "linux-tools" package makes use of that feature:

	MAKE_PERF := $(MAKE) prefix=/usr V=1 ARCH=$(KERNEL_ARCH_PERF) EXTRA_WARNINGS=-Wno-error

Source: http://anonscm.debian.org/viewvc/kernel/dists/trunk/linux-tools/debian/build/tools/perf/Makefile?view=markup (line 29)


The "prefix" parameter is handed down from Makefile.perf to Makefile.build
because it's invoked with $(MAKE), so the command line parameters are
inherited to Makefile.build:

	$(Q)$(MAKE) $(build)=perf

Source: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/tools/perf/Makefile.perf#n282


That "prefix" feature is broken in all 4.1 release candidates because a
variable definition specified on the make command line is an "overriding
variable", so this definition in line 97 of tools/build/Makefile.build
has no effect:

	prefix       := $(subst ./,,$(OUTPUT)$(dir)/)

Source: https://www.gnu.org/software/make/manual/html_node/Overriding.html


So $(prefix) contains the wrong value, yet is subsequently used in
Makefile.build, causing the build to break.


Best regards,

Lukas

> 
> jirka
> 
> > 
> > Fixes: c819e2cf2eb6f65d3208d195d7a0edef6108d5
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  tools/build/Makefile.build | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/build/Makefile.build b/tools/build/Makefile.build
> > index 10df572..98cfc38 100644
> > --- a/tools/build/Makefile.build
> > +++ b/tools/build/Makefile.build
> > @@ -94,12 +94,12 @@ obj-y        := $(patsubst %/, %/$(obj)-in.o, $(obj-y))
> >  subdir-obj-y := $(filter %/$(obj)-in.o, $(obj-y))
> >  
> >  # '$(OUTPUT)/dir' prefix to all objects
> > -prefix       := $(subst ./,,$(OUTPUT)$(dir)/)
> > -obj-y        := $(addprefix $(prefix),$(obj-y))
> > -subdir-obj-y := $(addprefix $(prefix),$(subdir-obj-y))
> > +objprefix    := $(subst ./,,$(OUTPUT)$(dir)/)
> > +obj-y        := $(addprefix $(objprefix),$(obj-y))
> > +subdir-obj-y := $(addprefix $(objprefix),$(subdir-obj-y))
> >  
> >  # Final '$(obj)-in.o' object
> > -in-target := $(prefix)$(obj)-in.o
> > +in-target := $(objprefix)$(obj)-in.o
> >  
> >  PHONY += $(subdir-y)
> >  
> > -- 
> > 1.8.5.2 (Apple Git-48)
> > 

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[David Ahern]

On 6/18/15 1:59 PM, Lukas Wunner wrote:
> E.g. the Debian "linux-tools" package makes use of that feature:
>
> 	MAKE_PERF := $(MAKE) prefix=/usr V=1 ARCH=$(KERNEL_ARCH_PERF) EXTRA_WARNINGS=-Wno-error
>
> Source: http://anonscm.debian.org/viewvc/kernel/dists/trunk/linux-tools/debian/build/tools/perf/Makefile?view=markup (line 29)
>
>
> The "prefix" parameter is handed down from Makefile.perf to Makefile.build
> because it's invoked with $(MAKE), so the command line parameters are
> inherited to Makefile.build:
>
> 	$(Q)$(MAKE) $(build)=perf
>
> Source: https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/tools/perf/Makefile.perf#n282
>
>
> That "prefix" feature is broken in all 4.1 release candidates because a

It worked for me last week with OL6.

I created a standalone perf rpm with 4.1-rc6; it builds just fine with 
_prefix (rpm variable) set to /usr:

%build
# prepare directories
rm -rf $RPM_BUILD_ROOT

cd perf-%{kversion}

%global perf_make \
   make -s -C tools/perf V=1 WERROR=0 NO_LIBUNWIND=1 
HAVE_CPLUS_DEMANGLE=1 NO_GTK2=1 NO_STRLCPY=1 NO_BIONIC=1 prefix=%{_prefix}

%{perf_make} DESTDIR=$RPM_BUILD_ROOT all

###
### install
###

%install
[ "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
cd perf-%{kversion}
%{perf_make} DESTDIR=$RPM_BUILD_ROOT lib=%{_lib} install man

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Lukas Wunner]

Hi David,

On Thu, Jun 18, 2015 at 02:26:25PM -0600, David Ahern wrote:
> It worked for me last week with OL6.
> I created a standalone perf rpm with 4.1-rc6; it builds just fine with
> _prefix (rpm variable) set to /usr:
[...]
> %global perf_make \
>   make -s -C tools/perf V=1 WERROR=0 NO_LIBUNWIND=1 HAVE_CPLUS_DEMANGLE=1
> NO_GTK2=1 NO_STRLCPY=1 NO_BIONIC=1 prefix=%{_prefix}
> %{perf_make} DESTDIR=$RPM_BUILD_ROOT all

You're not invoking tools/perf/Makefile.perf but tools/perf/Makefile
and I would say this in line 18 avoids that prefix= is passed down
to Makefile.perf:

	# We don't want to pass along options like -j:
	unexport MAKEFLAGS

Sources:
https://www.gnu.org/software/make/manual/html_node/Options_002fRecursion.html
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/tools/perf/Makefile#n18

So the prefix parameter should have no effect at all in your case,
no matter to what you set it.

Best regards,

Lukas

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Jiri Olsa]

On Thu, Jun 18, 2015 at 10:39:35PM +0200, Lukas Wunner wrote:
> Hi David,
> 
> On Thu, Jun 18, 2015 at 02:26:25PM -0600, David Ahern wrote:
> > It worked for me last week with OL6.
> > I created a standalone perf rpm with 4.1-rc6; it builds just fine with
> > _prefix (rpm variable) set to /usr:
> [...]
> > %global perf_make \
> >   make -s -C tools/perf V=1 WERROR=0 NO_LIBUNWIND=1 HAVE_CPLUS_DEMANGLE=1
> > NO_GTK2=1 NO_STRLCPY=1 NO_BIONIC=1 prefix=%{_prefix}
> > %{perf_make} DESTDIR=$RPM_BUILD_ROOT all
> 
> You're not invoking tools/perf/Makefile.perf but tools/perf/Makefile
> and I would say this in line 18 avoids that prefix= is passed down
> to Makefile.perf:
> 
> 	# We don't want to pass along options like -j:
> 	unexport MAKEFLAGS
> 
> Sources:
> https://www.gnu.org/software/make/manual/html_node/Options_002fRecursion.html
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/tools/perf/Makefile#n18
> 
> So the prefix parameter should have no effect at all in your case,
> no matter to what you set it.

ouch, right:

"make automatically passes down variable values that were defined on the command line, by putting them in the MAKEFLAGS variable. See the next section. "

haven't realized Makefile.perf is used as primary makefile :-\

I think the patch is ok, I will try to come up with some tests
for this Makefile.perf usage.

thanks,
jirka

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[David Ahern]

On 6/18/15 2:39 PM, Lukas Wunner wrote:
> So the prefix parameter should have no effect at all in your case,
> no matter to what you set it.

It does:

rpmbuild -bb --define="_prefix /tmp/junk" SPECS/perf.spec

...
+ make -s -C tools/perf V=1 WERROR=0 NO_LIBUNWIND=1 
HAVE_CPLUS_DEMANGLE=1 NO_GTK2=1 NO_STRLCPY=1 NO_BIONIC=1 
prefix=/tmp/junk DESTDIR=/root/rpmbuild/BUILDROOT/perf-4.1.rc6-1.x86_64 all

...

Note that prefix=/tmp/junk when tools/perf/Makefile gets invoked.

[root@f21-vbox rpmbuild]# rpm -qvlp 
/root/rpmbuild/RPMS/x86_64/perf-4.1.rc6-1.x86_64.rpm | grep bin/perf
-rwxr-xr-x    2 root    root                  2382176 Jun 18 13:10 
/tmp/junk/bin/perf

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Jiri Olsa]

On Thu, Jun 18, 2015 at 10:39:35PM +0200, Lukas Wunner wrote:
> Hi David,
> 
> On Thu, Jun 18, 2015 at 02:26:25PM -0600, David Ahern wrote:
> > It worked for me last week with OL6.
> > I created a standalone perf rpm with 4.1-rc6; it builds just fine with
> > _prefix (rpm variable) set to /usr:
> [...]
> > %global perf_make \
> >   make -s -C tools/perf V=1 WERROR=0 NO_LIBUNWIND=1 HAVE_CPLUS_DEMANGLE=1
> > NO_GTK2=1 NO_STRLCPY=1 NO_BIONIC=1 prefix=%{_prefix}
> > %{perf_make} DESTDIR=$RPM_BUILD_ROOT all
> 
> You're not invoking tools/perf/Makefile.perf but tools/perf/Makefile
> and I would say this in line 18 avoids that prefix= is passed down
> to Makefile.perf:
> 
> 	# We don't want to pass along options like -j:
> 	unexport MAKEFLAGS
> 
> Sources:
> https://www.gnu.org/software/make/manual/html_node/Options_002fRecursion.html
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/tools/perf/Makefile#n18
> 
> So the prefix parameter should have no effect at all in your case,
> no matter to what you set it.

it had effect.. prefix is passed to build framework throught .config-detected
file as prefix_SQ variable, which is then referenced in perf Build makefiles

the issue is that having 'unexport MAKEFLAGS' in perf's Makefile causes
'prefix' variable to change from 'command line' origin to environment

so the tools/build/Makefile.build can set prefix if called from perf's
'Makefile', but will not override it if called from perf's Makefile.perf,
because prefix will have 'command line' origin and we would need to use
'override'

jirka

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Lukas Wunner]

Hi Jiri, Hi David,

On Fri, Jun 19, 2015 at 12:17:06AM +0200, Jiri Olsa wrote:
> it had effect.. prefix is passed to build framework throught .config-detected
> file as prefix_SQ variable, which is then referenced in perf Build makefiles

You're right, thanks for the clarification.

The underlying issue is basically that the same variable name is used for
two different things, to calculate the paths of the build targets in
Makefile.build and to pass the install directory around between Makefiles.
And which one "wins" depends on invocation. If we just rename the variable
in Makefile.build, the issue is gone. That's what the patch does.

Thanks again,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Arnaldo Carvalho de Melo]

Em Fri, Jun 19, 2015 at 01:54:17PM +0200, Lukas Wunner escreveu:
> Hi Jiri, Hi David,
> 
> On Fri, Jun 19, 2015 at 12:17:06AM +0200, Jiri Olsa wrote:
> > it had effect.. prefix is passed to build framework throught .config-detected
> > file as prefix_SQ variable, which is then referenced in perf Build makefiles
> 
> You're right, thanks for the clarification.

Yes, this is all very involved, lots of details, that would be great to
have in the changeset log, so that we could read it when scratching our
heads trying to understand all this :-\

Could someone, please, send a v2 with these explanations?

- Arnaldo
 
> The underlying issue is basically that the same variable name is used for
> two different things, to calculate the paths of the build targets in
> Makefile.build and to pass the install directory around between Makefiles.
> And which one "wins" depends on invocation. If we just rename the variable
> in Makefile.build, the issue is gone. That's what the patch does.
> 
> Thanks again,
> 
> Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH regression 4.0 -> 4.1] tools perf: Fix build breakage if prefix= is specified

[Jiri Olsa]

On Thu, Jun 18, 2015 at 01:00:32PM +0200, Lukas Wunner wrote:
> Invoking Makefile.perf with prefix= breaks the build since Makefile.perf
> hands that variable down to Makefile.build where it overrides
>     prefix       := $(subst ./,,$(OUTPUT)$(dir)/)
> 
> leading to errors like this:
>     No rule to make target '/usrabspath.o', needed by '/usrlibperf-in.o'
> 
> Fixes: c819e2cf2eb6f65d3208d195d7a0edef6108d5
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka