Re: [RFC PATCH v3 4/9] trace-cmd: Extract part of Makefile in utils.mk

Re: [RFC PATCH v3 4/9] trace-cmd: Extract part of Makefile in utils.mk

[Steven Rostedt]

On Wed, 13 Dec 2017 15:32:35 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> On Tue, 2017-12-12 at 19:00 -0500, Steven Rostedt wrote:
> > 						\  
> > > diff --git a/utils.mk b/utils.mk  
> > 
> > I wonder if we should add a scripts/ directory here, to place the built
> > utilities in.
> >   
> 
> As you wish. Just 'scripts' seems to me more like a directory containing
> bash/perl/python scripts. Maybe something like make_utils/make_helpers?
> I don't know. I'm fine either way. Maybe we should create the directory
> in case we have more files to put in?
> 

I'm just following what we do in the Linux kernel. It has a directory
called "scripts" to help with the build process. As trace-cmd is very
tied to the Linux kernel, I want to keep as close to it as possible.
Hence, why we are doing all this crazy stuff with make and not any
other tool.

-- Steve

Re: [RFC PATCH v3 4/9] trace-cmd: Extract part of Makefile in utils.mk

[Vladislav Valtchev]

On Wed, 2017-12-13 at 10:47 -0500, Steven Rostedt wrote:
> 
> I'm just following what we do in the Linux kernel. It has a directory
> called "scripts" to help with the build process. As trace-cmd is very
> tied to the Linux kernel, I want to keep as close to it as possible.
> Hence, why we are doing all this crazy stuff with make and not any
> other tool.
> 

Alright. I've already done the change utils.mk -> scripts/utils.mk in my repo.

Re: [RFC PATCH v3 5/9] trace-cmd: Move libparsevent *.c files in lib/pevent

[Steven Rostedt]

On Wed, 13 Dec 2017 14:04:19 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> > > +
> > > +DEFAULT_TARGET = libparsevent.a
> > > +
> > > +OBJS =  
> > 
> > Is the above really necessary?  
> 
> DEFAULT_TARGET is just a label, I can remove it.

I didn't mean the DEFAULT_TARGET when I said "the above", I was just
referring to OBJS, which I now see that could be somewhat ambiguous in
what I meant.

> While OBJS = is a "coding style" thing in order to have all the
> objects added with +=, one per line.

I like the "+=" but do we really need to initialize it to nothing
first? Doesn't make work if you just simply remove the "OBJS =" blank
line, and a "OBJS += blah" will make OBJS equal to "blah" if OBJS
doesn't exist yet.

-- Steve

> 
> How would you like the objects to be listed?

Re: [RFC PATCH v3 5/9] trace-cmd: Move libparsevent *.c files in lib/pevent

[Vladislav Valtchev]

On Wed, 2017-12-13 at 10:49 -0500, Steven Rostedt wrote:
> I didn't mean the DEFAULT_TARGET when I said "the above", I was just
> referring to OBJS, which I now see that could be somewhat ambiguous in
> what I meant.
> 
> > While OBJS = is a "coding style" thing in order to have all the
> > objects added with +=, one per line.
> 
> I like the "+=" but do we really need to initialize it to nothing
> first? Doesn't make work if you just simply remove the "OBJS =" blank
> line, and a "OBJS += blah" will make OBJS equal to "blah" if OBJS
> doesn't exist yet.
> 

OK, by just using OBJS += ... works.
But: it is a dangerous in case the parent file had an OBJS variable exported:
in that case, the variable won't be cleared first and we'll end up with the wrong
objects.

I copied the practice of using OBJS = and later OBJS += ... from the original Makefile:

PLUGIN_OBJS =
PLUGIN_OBJS += plugin_jbd2.o
PLUGIN_OBJS += plugin_hrtimer.o
PLUGIN_OBJS += plugin_kmem.o
PLUGIN_OBJS += plugin_kvm.o
PLUGIN_OBJS += plugin_mac80211.o
PLUGIN_OBJS += plugin_sched_switch.o
PLUGIN_OBJS += plugin_function.o
PLUGIN_OBJS += plugin_xen.o
PLUGIN_OBJS += plugin_scsi.o
PLUGIN_OBJS += plugin_cfg80211.o
PLUGIN_OBJS += plugin_blk.o
PLUGIN_OBJS += plugin_tlb.o 

Would you like the empty assignment to be removed also from there?

Vlad

Re: [RFC PATCH v3 5/9] trace-cmd: Move libparsevent *.c files in lib/pevent

[Steven Rostedt]

On Mon, 18 Dec 2017 17:34:10 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> I copied the practice of using OBJS = and later OBJS += ... from the original Makefile:
> 
> PLUGIN_OBJS =
> PLUGIN_OBJS += plugin_jbd2.o
> PLUGIN_OBJS += plugin_hrtimer.o
> PLUGIN_OBJS += plugin_kmem.o
> PLUGIN_OBJS += plugin_kvm.o
> PLUGIN_OBJS += plugin_mac80211.o
> PLUGIN_OBJS += plugin_sched_switch.o
> PLUGIN_OBJS += plugin_function.o
> PLUGIN_OBJS += plugin_xen.o
> PLUGIN_OBJS += plugin_scsi.o
> PLUGIN_OBJS += plugin_cfg80211.o
> PLUGIN_OBJS += plugin_blk.o
> PLUGIN_OBJS += plugin_tlb.o 
> 
> Would you like the empty assignment to be removed also from there?

Heh, I didn't remember doing that. But I did :-p

This is too minor to debate. Just use the "OBJS =".

Thanks,

-- Steve

Re: [RFC PATCH v3 9/9] trace-cmd: Move GUI files in kernel-shark/

[Steven Rostedt]

On Wed, 13 Dec 2017 15:50:46 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> I meant "keeping this might be a problem".
> What I had in mind was that in the best case, with the hack in case
> the parent Makefile changes (adding config_includes and config flags),
> we'll link useless libraries. In the worst case, different versions of the same
> library might be used and that will cause and kind of strange build failures.
> 
> But, I think keeping the hack in that patch makes sense, since the concept of
> config_includes and config_flags is fading away in the parent Makefile.
> In the next patches of the series also the trace-cmd application will have a separate
> directory and Makefile, so I could remove those variables from the parent Makefile.
> It seems fine to me to assume that only the Makefiles of the specific applications can
> really add config_flags (system libraries). The top-level Makefile should only orchestrate
> the others.
> 
> 
> Makes sense?

Sure.

-- Steve

Re: [RFC PATCH v3 0/9] trace-cmd: restructure of the source tree [incomplete]

[Steven Rostedt]

On Wed, 13 Dec 2017 15:56:08 +0200
Vladislav Valtchev <vladislav.valtchev@gmail.com> wrote:

> On Tue, 2017-12-12 at 19:30 -0500, Steven Rostedt wrote:
> > Hi Vlad,
> > 
> > The patches look decent. I made a few comments, but I think you can
> > send then non RFC next time after incorporating what I suggested.
> >   
> 
> I'm so happy that you liked them!
> Sure, I can make the next iteration non-RFC. Just, I was planning to add
> also the last 3 patches for:
> 
> src/plugins
> src/trace-cmd/include
> src/trace-cmd
> 
> As part of the same series. The alternative is calling this series:
> 
> "trace-cmd: restructure of the source tree [part 1]".
> 
> and the next one "... [part 2]".
> 
> 
> Which do you think is better?

Do it in the next non-RFC patch set. I think you are starting to get
the idea of what I want. And there's no reason to have it RFC. The next
series may still not be ready to go in, but that's why we have v2, v3,
v4... notations ;-)

-- Steve

Re: [RFC PATCH v3 0/9] trace-cmd: restructure of the source tree [incomplete]

[Vladislav Valtchev]

On Wed, 2017-12-13 at 10:53 -0500, Steven Rostedt wrote:
> Do it in the next non-RFC patch set. I think you are starting to get
> the idea of what I want. And there's no reason to have it RFC. The next
> series may still not be ready to go in, but that's why we have v2, v3,
> v4... notations ;-)
> 

Sure, no problem. I'm working on it.