[PATCH 0/2] trace-cmd: Improves python plugin support

[PATCH 0/2] trace-cmd: Improves python plugin support

[sztsian]

From: Zamir SUN (Red Hat) <sztsian@gmail.com>

This is a set of patch to improve trace-cmd python plugin support.

Patch 1 fixes the detection for swig. With this patch, python-plugin can
be built successfully.

Patch 2 improves the detection for python ldflags. With this patch,
ldflags can be detected with python3 in the future.

Zamir SUN (2):
  trace-cmd: Fix the detection for swig
  trace-cmd: Change the way of getting python ldflags.

 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.14.3

[PATCH 1/2] trace-cmd: Fix the detection for swig

[sztsian]

From: Zamir SUN <sztsian@gmail.com>

The current detection for swig will cause output to be
/usr/bin/swig
y
So this will never be equal to y. With this patch, the swig path is
removed from output, so the detection can work as expected.

Fixes 3bf187a43b7e6302592552ecbc294e5820249687

Signed-off-by: Zamir SUN (Red Hat) <sztsian@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index a5d2c38..7c0d1a6 100644
--- a/Makefile
+++ b/Makefile
@@ -105,7 +105,7 @@ PYTHON_GUI	:= ctracecmd.so ctracecmdgui.so
 PYTHON_VERS ?= python
 
 # Can build python?
-ifeq ($(shell sh -c "pkg-config --cflags $(PYTHON_VERS) > /dev/null 2>&1 && which swig && echo y"), y)
+ifeq ($(shell sh -c "pkg-config --cflags $(PYTHON_VERS) > /dev/null 2>&1 && which swig > /dev/null && echo y"), y)
 	PYTHON_PLUGINS := plugin_python.so
 	BUILD_PYTHON := $(PYTHON) $(PYTHON_PLUGINS)
 	PYTHON_SO_INSTALL := ctracecmd.install
-- 
2.14.3

[PATCH 2/2] trace-cmd: Change the way of getting python ldflags.

[sztsian]

From: Zamir SUN <sztsian@gmail.com>

Prior than this patch, Makefile detects python ldflags using a hardcoded
python command. It will cause problems if we are building against
python3 in the future when ldflags for python2 and python3 are
different. With this patch, python ldflags are detected by
corresponding python{,3}-config which will detect the right config for
python plugins.

Signed-off-by: Zamir SUN (Red Hat) <sztsian@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7c0d1a6..f41e399 100644
--- a/Makefile
+++ b/Makefile
@@ -636,7 +636,7 @@ report_noswig: force
 
 PYTHON_INCLUDES = `pkg-config --cflags $(PYTHON_VERS)`
 PYTHON_LDFLAGS = `pkg-config --libs $(PYTHON_VERS)` \
-		$(shell python2 -c "import distutils.sysconfig; print distutils.sysconfig.get_config_var('LINKFORSHARED')")
+		$(shell $(PYTHON_VERS)-config --ldflags)
 PYGTK_CFLAGS = `pkg-config --cflags pygtk-2.0`
 
 ctracecmd.so: $(TCMD_LIB_OBJS) ctracecmd.i
-- 
2.14.3

Re: [PATCH 0/2] trace-cmd: Improves python plugin support

[Zamir SUN]

Hi Steve,

Do you have any thoughts for this?

Thanks.

On 02/04/2018 11:20 AM, sztsian@gmail.com wrote:
> From: Zamir SUN (Red Hat) <sztsian@gmail.com>
> 
> This is a set of patch to improve trace-cmd python plugin support.
> 
> Patch 1 fixes the detection for swig. With this patch, python-plugin can
> be built successfully.
> 
> Patch 2 improves the detection for python ldflags. With this patch,
> ldflags can be detected with python3 in the future.
> 
> Zamir SUN (2):
>   trace-cmd: Fix the detection for swig
>   trace-cmd: Change the way of getting python ldflags.
> 
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E

Re: [PATCH 0/2] trace-cmd: Improves python plugin support

[Steven Rostedt]

On Tue, 6 Feb 2018 20:31:54 +0800
Zamir SUN <sztsian@gmail.com> wrote:

> Hi Steve,
> 
> Do you have any thoughts for this?
> 

Hi Zamir,

I just got back from traveling and I'm digging through email. I saw
your patches and do plan on looking at them this week.

-- Steve

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Steven Rostedt]

On Sun,  4 Feb 2018 11:20:13 +0800
sztsian@gmail.com wrote:

> From: Zamir SUN <sztsian@gmail.com>
> 
> The current detection for swig will cause output to be
> /usr/bin/swig
> y
> So this will never be equal to y. With this patch, the swig path is
> removed from output, so the detection can work as expected.
> 
> Fixes 3bf187a43b7e6302592552ecbc294e5820249687

Hi Zamir,

Actually, a fix was already sent to me:

 http://lkml.kernel.org/r/20171220180748.17273-18-vladislav.valtchev@gmail.com

I'm working on add that one.

Thanks!

-- Steve

 
> 
> Signed-off-by: Zamir SUN (Red Hat) <sztsian@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index a5d2c38..7c0d1a6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -105,7 +105,7 @@ PYTHON_GUI	:= ctracecmd.so ctracecmdgui.so
>  PYTHON_VERS ?= python
>  
>  # Can build python?
> -ifeq ($(shell sh -c "pkg-config --cflags $(PYTHON_VERS) > /dev/null 2>&1 && which swig && echo y"), y)
> +ifeq ($(shell sh -c "pkg-config --cflags $(PYTHON_VERS) > /dev/null 2>&1 && which swig > /dev/null && echo y"), y)
>  	PYTHON_PLUGINS := plugin_python.so
>  	BUILD_PYTHON := $(PYTHON) $(PYTHON_PLUGINS)
>  	PYTHON_SO_INSTALL := ctracecmd.install

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Zamir SUN]

On 02/07/2018 07:18 AM, Steven Rostedt wrote:
> On Sun,  4 Feb 2018 11:20:13 +0800
> sztsian@gmail.com wrote:
> 
>> From: Zamir SUN <sztsian@gmail.com>
>>
>> The current detection for swig will cause output to be
>> /usr/bin/swig
>> y
>> So this will never be equal to y. With this patch, the swig path is
>> removed from output, so the detection can work as expected.
>>
>> Fixes 3bf187a43b7e6302592552ecbc294e5820249687
> 
> Hi Zamir,
> 
> Actually, a fix was already sent to me:
> 
>  http://lkml.kernel.org/r/20171220180748.17273-18-vladislav.valtchev@gmail.com
> 
> I'm working on add that one.
> 

Thanks for the heads-up! I did not see that before.

While just a note for this, in Fedora 27:

$ if command -v swig; then echo 1; else echo 0; fi
/usr/bin/swig
1

Actually this test has the same problem as of `which swig` - When swig
exists, the var SWIG_DEFINED will not be 1 (as shown above).

I see Vladislav is already on this thread. I hope this can be fixed
before merging the patch set.


> Thanks!
> 
> -- Steve
> 
>  
>>
>> Signed-off-by: Zamir SUN (Red Hat) <sztsian@gmail.com>
>> ---
>>  Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index a5d2c38..7c0d1a6 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -105,7 +105,7 @@ PYTHON_GUI	:= ctracecmd.so ctracecmdgui.so
>>  PYTHON_VERS ?= python
>>  
>>  # Can build python?
>> -ifeq ($(shell sh -c "pkg-config --cflags $(PYTHON_VERS) > /dev/null 2>&1 && which swig && echo y"), y)
>> +ifeq ($(shell sh -c "pkg-config --cflags $(PYTHON_VERS) > /dev/null 2>&1 && which swig > /dev/null && echo y"), y)
>>  	PYTHON_PLUGINS := plugin_python.so
>>  	BUILD_PYTHON := $(PYTHON) $(PYTHON_PLUGINS)
>>  	PYTHON_SO_INSTALL := ctracecmd.install
> 

-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Steven Rostedt]

On Wed, 7 Feb 2018 22:01:46 +0800
Zamir SUN <sztsian@gmail.com> wrote:


> Thanks for the heads-up! I did not see that before.
> 
> While just a note for this, in Fedora 27:
> 
> $ if command -v swig; then echo 1; else echo 0; fi
> /usr/bin/swig
> 1
> 
> Actually this test has the same problem as of `which swig` - When swig
> exists, the var SWIG_DEFINED will not be 1 (as shown above).
> 
> I see Vladislav is already on this thread. I hope this can be fixed
> before merging the patch set.
>

After doing some more testing, I'm going to push out the changes today.
Would you be able to add patches on top of the changes? We are doing a
restructuring of the directory such that it can grow more.

I'll be backporting these fixes to 2.6 and 2.7 as well.

Thanks!

-- Steve

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Zamir SUN]

On 02/07/2018 10:40 PM, Steven Rostedt wrote:
> On Wed, 7 Feb 2018 22:01:46 +0800
> Zamir SUN <sztsian@gmail.com> wrote:
> 
> 
>> Thanks for the heads-up! I did not see that before.
>>
>> While just a note for this, in Fedora 27:
>>
>> $ if command -v swig; then echo 1; else echo 0; fi
>> /usr/bin/swig
>> 1
>>
>> Actually this test has the same problem as of `which swig` - When swig
>> exists, the var SWIG_DEFINED will not be 1 (as shown above).
>>
>> I see Vladislav is already on this thread. I hope this can be fixed
>> before merging the patch set.
>>
> 
> After doing some more testing, I'm going to push out the changes today.
> Would you be able to add patches on top of the changes? We are doing a
> restructuring of the directory such that it can grow more.
> 
> I'll be backporting these fixes to 2.6 and 2.7 as well.
> 

Sure, I will rebase after Vladislav's patch set is merged.

Thanks!


-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Vladislav Valtchev]

On Wed, 2018-02-07 at 22:01 +0800, Zamir SUN wrote:
> 
> While just a note for this, in Fedora 27:
> 
> $ if command -v swig; then echo 1; else echo 0; fi
> /usr/bin/swig
> 1
> 
> Actually this test has the same problem as of `which swig` - When swig
> exists, the var SWIG_DEFINED will not be 1 (as shown above).
> 
> I see Vladislav is already on this thread. I hope this can be fixed
> before merging the patch set.
> 

Hi Zamir,
I'm sorry but I'm unable to reproduce the issue on Fedora 27,
after applying my series of patches.

In my case (fresh install of Fedora27, no customizations):
	if command -v swig; then echo 1; else echo 0; fi
works as expected.

SWIG_DEFINED does not have to be '1' in order the Makefile to work.
Because the check in my branch is:

ifeq ($(SWIG_DEFINED), 0)
BUILD_PYTHON := report_noswig
NO_PYTHON = 1
endif

So there should be no problem. Just to be sure, I've tried to compile trace-cmd
with my patches first without 'swig' and later with it (on Fedora).
The result was that, in the first case the message 'no swig installed'
was displayed, while in the other case it was not. Or better, a message
saying that 'python-dev' is missing was shown.
[Yes, both swig and python-dev are necessary].

After installing also 'python-devel' (in Ubuntu is just '-dev'), the
build system was able to build the rest of the python targets as well.


Could you please try by using my fork on GitHub?
https://github.com/vvaltchev/trace-cmd-fork
BRANCH NAME: make8.

Let me know if you still experience the issue.

Thanks,
Vlad



-- 
Vladislav Valtchev
VMware Open Source Technology Center

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Zamir SUN]

On 02/08/2018 01:25 AM, Vladislav Valtchev wrote:
> On Wed, 2018-02-07 at 22:01 +0800, Zamir SUN wrote:
>>
>> While just a note for this, in Fedora 27:
>>
>> $ if command -v swig; then echo 1; else echo 0; fi
>> /usr/bin/swig
>> 1
>>
>> Actually this test has the same problem as of `which swig` - When swig
>> exists, the var SWIG_DEFINED will not be 1 (as shown above).
>>
>> I see Vladislav is already on this thread. I hope this can be fixed
>> before merging the patch set.
>>
> 
> Hi Zamir,
> I'm sorry but I'm unable to reproduce the issue on Fedora 27,
> after applying my series of patches.
> 
> In my case (fresh install of Fedora27, no customizations):
> 	if command -v swig; then echo 1; else echo 0; fi
> works as expected.
> 
> SWIG_DEFINED does not have to be '1' in order the Makefile to work.
> Because the check in my branch is:
> 
> ifeq ($(SWIG_DEFINED), 0)
> BUILD_PYTHON := report_noswig
> NO_PYTHON = 1
> endif
> 
> So there should be no problem. Just to be sure, I've tried to compile trace-cmd
> with my patches first without 'swig' and later with it (on Fedora).
> The result was that, in the first case the message 'no swig installed'
> was displayed, while in the other case it was not. Or better, a message
> saying that 'python-dev' is missing was shown.
> [Yes, both swig and python-dev are necessary].
> 
> After installing also 'python-devel' (in Ubuntu is just '-dev'), the
> build system was able to build the rest of the python targets as well.
> 
> 
> Could you please try by using my fork on GitHub?
> https://github.com/vvaltchev/trace-cmd-fork
> BRANCH NAME: make8.
> 
> Let me know if you still experience the issue.
> 

Hi Vladislav,

Thanks for the URL. I tried to build from your make8 branch, it works
fine. I must made some copy-paste mistake yesterday.

Sorry for the noise.

Thanks.

> Thanks,
> Vlad
> 
> 
> 

-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E

Re: [PATCH 1/2] trace-cmd: Fix the detection for swig

[Vladislav Valtchev]

On Thu, 2018-02-08 at 20:05 +0800, Zamir SUN wrote:
> 
> Hi Vladislav,
> 
> Thanks for the URL. I tried to build from your make8 branch, it works
> fine. I must made some copy-paste mistake yesterday.
> 
> Sorry for the noise.
> 
> Thanks.

No problem, Zamir.
I'm happy that it works.

The series has been merged in the master branch by Steven now.

Anyway, I think your patch 2/2 is still necessary: thanks for it!

Vlad

-- 
Vladislav Valtchev
VMware Open Source Technology Center

Re: [PATCH 2/2] trace-cmd: Change the way of getting python ldflags.

[Steven Rostedt]

On Sun,  4 Feb 2018 11:20:14 +0800
sztsian@gmail.com wrote:

> From: Zamir SUN <sztsian@gmail.com>
> 
> Prior than this patch, Makefile detects python ldflags using a hardcoded
> python command. It will cause problems if we are building against
> python3 in the future when ldflags for python2 and python3 are
> different. With this patch, python ldflags are detected by
> corresponding python{,3}-config which will detect the right config for
> python plugins.

Thanks, applied.

-- Steve

Re: [PATCH 2/2] trace-cmd: Change the way of getting python ldflags.

[Steven Rostedt]

On Sun,  4 Feb 2018 11:20:14 +0800
sztsian@gmail.com wrote:

> From: Zamir SUN <sztsian@gmail.com>
> 
> Prior than this patch, Makefile detects python ldflags using a hardcoded
> python command. It will cause problems if we are building against
> python3 in the future when ldflags for python2 and python3 are
> different. With this patch, python ldflags are detected by
> corresponding python{,3}-config which will detect the right config for
> python plugins.
> 
> Signed-off-by: Zamir SUN (Red Hat) <sztsian@gmail.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 7c0d1a6..f41e399 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -636,7 +636,7 @@ report_noswig: force
>  
>  PYTHON_INCLUDES = `pkg-config --cflags $(PYTHON_VERS)`
>  PYTHON_LDFLAGS = `pkg-config --libs $(PYTHON_VERS)` \
> -		$(shell python2 -c "import distutils.sysconfig; print distutils.sysconfig.get_config_var('LINKFORSHARED')")
> +		$(shell $(PYTHON_VERS)-config --ldflags)
>  PYGTK_CFLAGS = `pkg-config --cflags pygtk-2.0`
>  
>  ctracecmd.so: $(TCMD_LIB_OBJS) ctracecmd.i

BTW, I did have this applied, but when testing it caused warnings. I
had to apply this patch to fix it:

-- Steve

>From d5579b729114135da20bcee6896a0683ff54f33a Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Date: Thu, 22 Mar 2018 18:38:22 -0400
Subject: [PATCH] trace-cmd build: Do not execute python scripts if there is no
 python

I was triggering the following build messages when not having swig installed:

make: -config: Command not found

    NO_PYTHON forced: swig not installed, not compiling python plugins

make: -config: Command not found
  UPDATE                 trace_python_dir
make: -config: Command not found

This is due to the executing of $(PYTHON_VERS)-config, which would just turn
into "-config". Do not assign the PYTHON_* variables if NO_PYTHON is
defined.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile
index 2c82301..26fd42b 100644
--- a/Makefile
+++ b/Makefile
@@ -373,10 +373,16 @@ report_nopythondev: force
 	$(Q)echo "    python-dev is not installed, not compiling python plugins"
 	$(Q)echo
 
+ifndef NO_PYTHON
 PYTHON_INCLUDES = `pkg-config --cflags $(PYTHON_VERS)`
 PYTHON_LDFLAGS = `pkg-config --libs $(PYTHON_VERS)` \
 		$(shell $(PYTHON_VERS)-config --ldflags)
 PYGTK_CFLAGS = `pkg-config --cflags pygtk-2.0`
+else
+PYTHON_INCLUDES =
+PYTHON_LDFLAGS =
+PYGTK_CFLAGS =
+endif
 
 export PYTHON_INCLUDES
 export PYTHON_LDFLAGS
-- 
2.13.6

Re: [PATCH 2/2] trace-cmd: Change the way of getting python ldflags.

[Zamir SUN]

On 03/23/2018 07:13 AM, Steven Rostedt wrote:
> On Sun,  4 Feb 2018 11:20:14 +0800
> sztsian@gmail.com wrote:
> 
>> From: Zamir SUN <sztsian@gmail.com>
>>
>> Prior than this patch, Makefile detects python ldflags using a hardcoded
>> python command. It will cause problems if we are building against
>> python3 in the future when ldflags for python2 and python3 are
>> different. With this patch, python ldflags are detected by
>> corresponding python{,3}-config which will detect the right config for
>> python plugins.
>>
>> Signed-off-by: Zamir SUN (Red Hat) <sztsian@gmail.com>
>> ---
>>  Makefile | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 7c0d1a6..f41e399 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -636,7 +636,7 @@ report_noswig: force
>>  
>>  PYTHON_INCLUDES = `pkg-config --cflags $(PYTHON_VERS)`
>>  PYTHON_LDFLAGS = `pkg-config --libs $(PYTHON_VERS)` \
>> -		$(shell python2 -c "import distutils.sysconfig; print distutils.sysconfig.get_config_var('LINKFORSHARED')")
>> +		$(shell $(PYTHON_VERS)-config --ldflags)
>>  PYGTK_CFLAGS = `pkg-config --cflags pygtk-2.0`
>>  
>>  ctracecmd.so: $(TCMD_LIB_OBJS) ctracecmd.i
> 
> BTW, I did have this applied, but when testing it caused warnings. I
> had to apply this patch to fix it:
> 
> -- Steve

Thanks for the info!

I am always using Fedora and Python is always preloaded so I did not
think of this scenario.

> 
> From d5579b729114135da20bcee6896a0683ff54f33a Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> Date: Thu, 22 Mar 2018 18:38:22 -0400
> Subject: [PATCH] trace-cmd build: Do not execute python scripts if there is no
>  python
> 
> I was triggering the following build messages when not having swig installed:
> 
> make: -config: Command not found
> 
>     NO_PYTHON forced: swig not installed, not compiling python plugins
> 
> make: -config: Command not found
>   UPDATE                 trace_python_dir
> make: -config: Command not found
> 
> This is due to the executing of $(PYTHON_VERS)-config, which would just turn
> into "-config". Do not assign the PYTHON_* variables if NO_PYTHON is
> defined.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  Makefile | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index 2c82301..26fd42b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -373,10 +373,16 @@ report_nopythondev: force
>  	$(Q)echo "    python-dev is not installed, not compiling python plugins"
>  	$(Q)echo
>  
> +ifndef NO_PYTHON
>  PYTHON_INCLUDES = `pkg-config --cflags $(PYTHON_VERS)`
>  PYTHON_LDFLAGS = `pkg-config --libs $(PYTHON_VERS)` \
>  		$(shell $(PYTHON_VERS)-config --ldflags)
>  PYGTK_CFLAGS = `pkg-config --cflags pygtk-2.0`
> +else
> +PYTHON_INCLUDES =
> +PYTHON_LDFLAGS =
> +PYGTK_CFLAGS =
> +endif
>  
>  export PYTHON_INCLUDES
>  export PYTHON_LDFLAGS
> 

-- 
Zamir SUN
Fedora user
GPG : 1D86 6D4A 49CE 4BBD 72CF FCF5 D856 6E11 F2A0 525E