Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-16 Thread Joe Perches 
On Tue, 2016-08-16 at 12:53 +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 15, 2016 at 02:14:18PM -0700, Joe Perches wrote:
> > On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > > > Start to rationalize include paths in source code files.
[]
> Nah, I think it's fine the way it is.  What's wrong with the "../" type
> things in the include directives?

.. path specific includes are unnecessarily file layout sensitive.

There are identically named .h files in separate directories.

The idea was to isolate the uapi type .h files from local ones.

I'll just give this up and let the lustre folks handle it.

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-16 Thread Joe Perches 
On Tue, 2016-08-16 at 12:53 +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 15, 2016 at 02:14:18PM -0700, Joe Perches wrote:
> > On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > > > Start to rationalize include paths in source code files.
[]
> Nah, I think it's fine the way it is.  What's wrong with the "../" type
> things in the include directives?

.. path specific includes are unnecessarily file layout sensitive.

There are identically named .h files in separate directories.

The idea was to isolate the uapi type .h files from local ones.

I'll just give this up and let the lustre folks handle it.

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-16 Thread Greg Kroah-Hartman 
On Mon, Aug 15, 2016 at 02:14:18PM -0700, Joe Perches wrote:
> On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > > Start to rationalize include paths in source code files.
> []
> > > diff --git a/drivers/staging/lustre/Makefile 
> > > b/drivers/staging/lustre/Makefile
> []
> > > @@ -1,2 +1,5 @@
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > > +
> > >  obj-$(CONFIG_LNET)   += lnet/
> > >  obj-$(CONFIG_LUSTRE_FS)  += lustre/
> > This is good, but does this break the subdir make command:
> > make M=drivers/staging/lustre/foo_dir/
> > ?
> 
> hmm, yeah, it does.  Oh well, nevermind for awhile.
> 
> > I remember the last time I tried to clean this up, it took a while...
> 
> It seems like something the build tools should
> handle correctly now, but I'll look at it.

Nah, I think it's fine the way it is.  What's wrong with the "../" type
things in the include directives?  The build system works well with it,
you can build any single module or file correctly, and it's obvious what
the code is including from.

Now you might argue that the lustre code shouldn't have such a mess of
include files like this, and I would totally agree.  I suggest working
on reducing the number of .h files they have, and centralizing them such
that all of these cross-directory includes aren't needed anymore either
in the build directives, or in the .c files.

thanks,

greg k-h

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-16 Thread Greg Kroah-Hartman 
On Mon, Aug 15, 2016 at 02:14:18PM -0700, Joe Perches wrote:
> On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > > Start to rationalize include paths in source code files.
> []
> > > diff --git a/drivers/staging/lustre/Makefile 
> > > b/drivers/staging/lustre/Makefile
> []
> > > @@ -1,2 +1,5 @@
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > > +
> > >  obj-$(CONFIG_LNET)   += lnet/
> > >  obj-$(CONFIG_LUSTRE_FS)  += lustre/
> > This is good, but does this break the subdir make command:
> > make M=drivers/staging/lustre/foo_dir/
> > ?
> 
> hmm, yeah, it does.  Oh well, nevermind for awhile.
> 
> > I remember the last time I tried to clean this up, it took a while...
> 
> It seems like something the build tools should
> handle correctly now, but I'll look at it.

Nah, I think it's fine the way it is.  What's wrong with the "../" type
things in the include directives?  The build system works well with it,
you can build any single module or file correctly, and it's obvious what
the code is including from.

Now you might argue that the lustre code shouldn't have such a mess of
include files like this, and I would totally agree.  I suggest working
on reducing the number of .h files they have, and centralizing them such
that all of these cross-directory includes aren't needed anymore either
in the build directives, or in the .c files.

thanks,

greg k-h

Building a subdirectory ignores parent subdir-ccflags (was: Re: [PATCH 1/2] staging: lustre: Add include path to Makefile)

2016-08-15 Thread Joe Perches 
On Mon, 2016-08-15 at 14:14 -0700, Joe Perches wrote:
> On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > > Start to rationalize include paths in source code files.
> []
> > > diff --git a/drivers/staging/lustre/Makefile 
> > > b/drivers/staging/lustre/Makefile
> []
> > > @@ -1,2 +1,5 @@
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > > +
> > >  obj-$(CONFIG_LNET)   += lnet/
> > >  obj-$(CONFIG_LUSTRE_FS)  += lustre/
> > This is good, but does this break the subdir make command:
> > make M=drivers/staging/lustre/foo_dir/
> > ?
> hmm, yeah, it does.  Oh well, nevermind for awhile.
> > I remember the last time I tried to clean this up, it took a while...
> It seems like something the build tools should
> handle correctly now, but I'll look at it.

Perhaps making a specific directory should also walk up
any parent directory Makefiles looking for subdir flags.

Is that unreasonable?  Any suggestions?

Building a subdirectory ignores parent subdir-ccflags (was: Re: [PATCH 1/2] staging: lustre: Add include path to Makefile)

2016-08-15 Thread Joe Perches 
On Mon, 2016-08-15 at 14:14 -0700, Joe Perches wrote:
> On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > > Start to rationalize include paths in source code files.
> []
> > > diff --git a/drivers/staging/lustre/Makefile 
> > > b/drivers/staging/lustre/Makefile
> []
> > > @@ -1,2 +1,5 @@
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > > +
> > >  obj-$(CONFIG_LNET)   += lnet/
> > >  obj-$(CONFIG_LUSTRE_FS)  += lustre/
> > This is good, but does this break the subdir make command:
> > make M=drivers/staging/lustre/foo_dir/
> > ?
> hmm, yeah, it does.  Oh well, nevermind for awhile.
> > I remember the last time I tried to clean this up, it took a while...
> It seems like something the build tools should
> handle correctly now, but I'll look at it.

Perhaps making a specific directory should also walk up
any parent directory Makefiles looking for subdir flags.

Is that unreasonable?  Any suggestions?

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread Joe Perches 
On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > Start to rationalize include paths in source code files.
[]
> > diff --git a/drivers/staging/lustre/Makefile 
> > b/drivers/staging/lustre/Makefile
[]
> > @@ -1,2 +1,5 @@
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > +
> >  obj-$(CONFIG_LNET) += lnet/
> >  obj-$(CONFIG_LUSTRE_FS)+= lustre/
> This is good, but does this break the subdir make command:
>   make M=drivers/staging/lustre/foo_dir/
> ?

hmm, yeah, it does.  Oh well, nevermind for awhile.

> I remember the last time I tried to clean this up, it took a while...

It seems like something the build tools should
handle correctly now, but I'll look at it.

No rush.

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread Joe Perches 
On Mon, 2016-08-15 at 23:04 +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > Start to rationalize include paths in source code files.
[]
> > diff --git a/drivers/staging/lustre/Makefile 
> > b/drivers/staging/lustre/Makefile
[]
> > @@ -1,2 +1,5 @@
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > +
> >  obj-$(CONFIG_LNET) += lnet/
> >  obj-$(CONFIG_LUSTRE_FS)+= lustre/
> This is good, but does this break the subdir make command:
>   make M=drivers/staging/lustre/foo_dir/
> ?

hmm, yeah, it does.  Oh well, nevermind for awhile.

> I remember the last time I tried to clean this up, it took a while...

It seems like something the build tools should
handle correctly now, but I'll look at it.

No rush.

Re: [lustre-devel] [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread James Simmons 

> On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > Start to rationalize include paths in source code files.
> > 
> > Signed-off-by: Joe Perches 
> > ---
> >  drivers/staging/lustre/Makefile | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/lustre/Makefile 
> > b/drivers/staging/lustre/Makefile
> > index 95ffe33..9d00237 100644
> > --- a/drivers/staging/lustre/Makefile
> > +++ b/drivers/staging/lustre/Makefile
> > @@ -1,2 +1,5 @@
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > +
> >  obj-$(CONFIG_LNET) += lnet/
> >  obj-$(CONFIG_LUSTRE_FS)+= lustre/
> 
> This is good, but does this break the subdir make command:
>   make M=drivers/staging/lustre/foo_dir/
> ?
> 
> I remember the last time I tried to clean this up, it took a while...

It breaks make M=drivers/staging/lustre/foo_dir

Re: [lustre-devel] [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread James Simmons 

> On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> > Start to rationalize include paths in source code files.
> > 
> > Signed-off-by: Joe Perches 
> > ---
> >  drivers/staging/lustre/Makefile | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/lustre/Makefile 
> > b/drivers/staging/lustre/Makefile
> > index 95ffe33..9d00237 100644
> > --- a/drivers/staging/lustre/Makefile
> > +++ b/drivers/staging/lustre/Makefile
> > @@ -1,2 +1,5 @@
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> > +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> > +
> >  obj-$(CONFIG_LNET) += lnet/
> >  obj-$(CONFIG_LUSTRE_FS)+= lustre/
> 
> This is good, but does this break the subdir make command:
>   make M=drivers/staging/lustre/foo_dir/
> ?
> 
> I remember the last time I tried to clean this up, it took a while...

It breaks make M=drivers/staging/lustre/foo_dir

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread Greg Kroah-Hartman 
On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> Start to rationalize include paths in source code files.
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/staging/lustre/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/lustre/Makefile b/drivers/staging/lustre/Makefile
> index 95ffe33..9d00237 100644
> --- a/drivers/staging/lustre/Makefile
> +++ b/drivers/staging/lustre/Makefile
> @@ -1,2 +1,5 @@
> +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> +
>  obj-$(CONFIG_LNET)   += lnet/
>  obj-$(CONFIG_LUSTRE_FS)  += lustre/

This is good, but does this break the subdir make command:
make M=drivers/staging/lustre/foo_dir/
?

I remember the last time I tried to clean this up, it took a while...

thanks,

greg k-h

Re: [PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread Greg Kroah-Hartman 
On Mon, Aug 15, 2016 at 12:33:23PM -0700, Joe Perches wrote:
> Start to rationalize include paths in source code files.
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/staging/lustre/Makefile | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/lustre/Makefile b/drivers/staging/lustre/Makefile
> index 95ffe33..9d00237 100644
> --- a/drivers/staging/lustre/Makefile
> +++ b/drivers/staging/lustre/Makefile
> @@ -1,2 +1,5 @@
> +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
> +subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
> +
>  obj-$(CONFIG_LNET)   += lnet/
>  obj-$(CONFIG_LUSTRE_FS)  += lustre/

This is good, but does this break the subdir make command:
make M=drivers/staging/lustre/foo_dir/
?

I remember the last time I tried to clean this up, it took a while...

thanks,

greg k-h

[PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread Joe Perches 
Start to rationalize include paths in source code files.

Signed-off-by: Joe Perches 
---
 drivers/staging/lustre/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/lustre/Makefile b/drivers/staging/lustre/Makefile
index 95ffe33..9d00237 100644
--- a/drivers/staging/lustre/Makefile
+++ b/drivers/staging/lustre/Makefile
@@ -1,2 +1,5 @@
+subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
+subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
+
 obj-$(CONFIG_LNET) += lnet/
 obj-$(CONFIG_LUSTRE_FS)+= lustre/
-- 
2.8.0.rc4.16.g56331f8

[PATCH 1/2] staging: lustre: Add include path to Makefile

2016-08-15 Thread Joe Perches 
Start to rationalize include paths in source code files.

Signed-off-by: Joe Perches 
---
 drivers/staging/lustre/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/lustre/Makefile b/drivers/staging/lustre/Makefile
index 95ffe33..9d00237 100644
--- a/drivers/staging/lustre/Makefile
+++ b/drivers/staging/lustre/Makefile
@@ -1,2 +1,5 @@
+subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/include/
+subdir-ccflags-y += -I$(srctree)/drivers/staging/lustre/lustre/include/
+
 obj-$(CONFIG_LNET) += lnet/
 obj-$(CONFIG_LUSTRE_FS)+= lustre/
-- 
2.8.0.rc4.16.g56331f8