[dpdk-dev] [RFC PATCH] mk: symlink every headers first

[dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Thomas Monjalon]

If a library or a build tool uses a definition from a driver,
there is a build ordering issue, like seen when moving PCI code
into a bus driver.

One option is to keep PCI helpers and some common definitions in EAL.
The other option is to symlink every headers at the beginning of
the build so they can be included by any other component.

This patch shows how to achieve the second option.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 mk/internal/rte.install-post.mk | 2 ++
 mk/rte.sdkbuild.mk              | 9 ++++++++-
 mk/rte.subdir.mk                | 7 +++++++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/mk/internal/rte.install-post.mk b/mk/internal/rte.install-post.mk
index b99e2b2f7..61804561f 100644
--- a/mk/internal/rte.install-post.mk
+++ b/mk/internal/rte.install-post.mk
@@ -55,7 +55,9 @@ $(foreach dir,$(INSTALL-DIRS-y),\
 # arg1: relative install dir in RTE_OUTPUT
 # arg2: relative file name in a source dir (VPATH)
 #
+.PHONY: headers
 define symlink_rule
+headers: $(addprefix $(RTE_OUTPUT)/$(1)/,$(notdir $(2)))
 $(addprefix $(RTE_OUTPUT)/$(1)/,$(notdir $(2))): $(2)
 	@echo "  SYMLINK-FILE $(addprefix $(1)/,$(notdir $(2)))"
 	@[ -d $(RTE_OUTPUT)/$(1) ] || mkdir -p $(RTE_OUTPUT)/$(1)
diff --git a/mk/rte.sdkbuild.mk b/mk/rte.sdkbuild.mk
index 0bf909e9e..ec04ccb2b 100644
--- a/mk/rte.sdkbuild.mk
+++ b/mk/rte.sdkbuild.mk
@@ -67,9 +67,16 @@ clean: $(CLEANDIRS)
 .PHONY: test-build
 test-build: test
 
+.PHONY: headers
+headers: $(addprefix headers-, $(ROOTDIRS-y))
+headers-%:
+	@[ -d $(BUILDDIR)/$* ] || mkdir -p $(BUILDDIR)/$*
+	$(Q)$(MAKE) S=$* -f $(RTE_SRCDIR)/$*/Makefile -C $(BUILDDIR)/$* \
+		-srR headers
+
 .SECONDEXPANSION:
 .PHONY: $(ROOTDIRS-y) $(ROOTDIRS-)
-$(ROOTDIRS-y) $(ROOTDIRS-):
+$(ROOTDIRS-y) $(ROOTDIRS-): headers
 	@[ -d $(BUILDDIR)/$@ ] || mkdir -p $(BUILDDIR)/$@
 	@echo "== Build $@"
 	$(Q)$(MAKE) S=$@ -f $(RTE_SRCDIR)/$@/Makefile -C $(BUILDDIR)/$@ all
diff --git a/mk/rte.subdir.mk b/mk/rte.subdir.mk
index 92f5de4c8..3ae4ce525 100644
--- a/mk/rte.subdir.mk
+++ b/mk/rte.subdir.mk
@@ -57,6 +57,13 @@ _postinstall: build
 .PHONY: build
 build: _postbuild
 
+.PHONY: headers
+headers: $(addprefix headers-, $(DIRS-y))
+headers-%:
+	@[ -d $(CURDIR)/$* ] || mkdir -p $(CURDIR)/$*
+	@$(MAKE) S=$S/$* -f $(SRCDIR)/$*/Makefile -C $(CURDIR)/$* \
+		-srR headers
+
 .SECONDEXPANSION:
 .PHONY: $(DIRS-y)
 $(DIRS-y):
-- 
2.13.1

Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Bruce Richardson]

On Tue, Jun 20, 2017 at 11:21:39PM +0200, Thomas Monjalon wrote:
> If a library or a build tool uses a definition from a driver,
> there is a build ordering issue, like seen when moving PCI code
> into a bus driver.
> 
> One option is to keep PCI helpers and some common definitions in EAL.
> The other option is to symlink every headers at the beginning of
> the build so they can be included by any other component.
> 
> This patch shows how to achieve the second option.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---

My 2c.

This may be worth doing, however, two points to consider.

1. If we look to change build system this may not be worth doing unless
a compelling need becomes obvious in the short term. In the meantime,
for cases where it is needed...
2. libraries can already access the includes from drivers or other
places fairly easily, just by adding the relevant "-I" flag to the
CFLAGS for that lib.

That said, since the work is already done developing this, and if it
doesn't hurt in terms of build time, I suppose we might as well merge
it in.

So tentative ack from me, subject to testing and feedback from others.

/Bruce

Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Thomas Monjalon]

21/06/2017 12:27, Bruce Richardson:
> On Tue, Jun 20, 2017 at 11:21:39PM +0200, Thomas Monjalon wrote:
> > If a library or a build tool uses a definition from a driver,
> > there is a build ordering issue, like seen when moving PCI code
> > into a bus driver.
> > 
> > One option is to keep PCI helpers and some common definitions in EAL.
> > The other option is to symlink every headers at the beginning of
> > the build so they can be included by any other component.
> > 
> > This patch shows how to achieve the second option.
> > 
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > ---
> 
> My 2c.
> 
> This may be worth doing, however, two points to consider.
> 
> 1. If we look to change build system this may not be worth doing unless
> a compelling need becomes obvious in the short term. In the meantime,
> for cases where it is needed...
> 2. libraries can already access the includes from drivers or other
> places fairly easily, just by adding the relevant "-I" flag to the
> CFLAGS for that lib.

You mean adding a -I pointing to source location instead of
the build directory?

> That said, since the work is already done developing this, and if it
> doesn't hurt in terms of build time, I suppose we might as well merge
> it in.

It hurts a bit the build time.

> So tentative ack from me, subject to testing and feedback from others.

I would prefer not applying this patch if Gaetan can organize EAL and
bus drivers in a way that everything compile fine.
Libraries should not look into drivers.

Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Richardson, Bruce]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, June 21, 2017 11:42 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: gaetan.rivet@6wind.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first
> 
> 21/06/2017 12:27, Bruce Richardson:
> > On Tue, Jun 20, 2017 at 11:21:39PM +0200, Thomas Monjalon wrote:
> > > If a library or a build tool uses a definition from a driver, there
> > > is a build ordering issue, like seen when moving PCI code into a bus
> > > driver.
> > >
> > > One option is to keep PCI helpers and some common definitions in EAL.
> > > The other option is to symlink every headers at the beginning of the
> > > build so they can be included by any other component.
> > >
> > > This patch shows how to achieve the second option.
> > >
> > > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> > > ---
> >
> > My 2c.
> >
> > This may be worth doing, however, two points to consider.
> >
> > 1. If we look to change build system this may not be worth doing
> > unless a compelling need becomes obvious in the short term. In the
> > meantime, for cases where it is needed...
> > 2. libraries can already access the includes from drivers or other
> > places fairly easily, just by adding the relevant "-I" flag to the
> > CFLAGS for that lib.
> 
> You mean adding a -I pointing to source location instead of the build
> directory?
> 

Yep. Easy workaround for limited cases where it is needed. It's been done before in our code, not sure there are any instances left after the rework of the last couple of releases, but we certainly used to have that scheme in the past and it didn't cause us a single problem as I recall.

/Bruce

Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Wiles, Keith]

> On Jun 21, 2017, at 5:27 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> On Tue, Jun 20, 2017 at 11:21:39PM +0200, Thomas Monjalon wrote:
>> If a library or a build tool uses a definition from a driver,
>> there is a build ordering issue, like seen when moving PCI code
>> into a bus driver.
>> 
>> One option is to keep PCI helpers and some common definitions in EAL.
>> The other option is to symlink every headers at the beginning of
>> the build so they can be included by any other component.
>> 
>> This patch shows how to achieve the second option.
>> 
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>> ---
> 
> My 2c.
> 
> This may be worth doing, however, two points to consider.
> 
> 1. If we look to change build system this may not be worth doing unless
> a compelling need becomes obvious in the short term. In the meantime,
> for cases where it is needed...
> 2. libraries can already access the includes from drivers or other
> places fairly easily, just by adding the relevant "-I" flag to the
> CFLAGS for that lib.
> 
> That said, since the work is already done developing this, and if it
> doesn't hurt in terms of build time, I suppose we might as well merge
> it in.
> 
> So tentative ack from me, subject to testing and feedback from others.

+1, I already have to make sure everything is symlinked first in my private DPDK work for other reasons. This patch would allow me to remove that special code.

> 
> /Bruce

Regards,
Keith

Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Gaëtan Rivet]

Hi,

On Wed, Jun 21, 2017 at 02:27:49PM +0000, Wiles, Keith wrote:
> 
> > On Jun 21, 2017, at 5:27 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > On Tue, Jun 20, 2017 at 11:21:39PM +0200, Thomas Monjalon wrote:
> >> If a library or a build tool uses a definition from a driver,
> >> there is a build ordering issue, like seen when moving PCI code
> >> into a bus driver.
> >> 
> >> One option is to keep PCI helpers and some common definitions in EAL.
> >> The other option is to symlink every headers at the beginning of
> >> the build so they can be included by any other component.
> >> 
> >> This patch shows how to achieve the second option.
> >> 
> >> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> >> ---
> > 
> > My 2c.
> > 
> > This may be worth doing, however, two points to consider.
> > 
> > 1. If we look to change build system this may not be worth doing unless
> > a compelling need becomes obvious in the short term. In the meantime,
> > for cases where it is needed...
> > 2. libraries can already access the includes from drivers or other
> > places fairly easily, just by adding the relevant "-I" flag to the
> > CFLAGS for that lib.
> > 
> > That said, since the work is already done developing this, and if it
> > doesn't hurt in terms of build time, I suppose we might as well merge
> > it in.
> > 
> > So tentative ack from me, subject to testing and feedback from others.
> 
> +1, I already have to make sure everything is symlinked first in my private DPDK work for other reasons. This patch would allow me to remove that special code.
> 
> > 
> > /Bruce
> 
> Regards,
> Keith
> 

Personally I do not like this approach.

A well designed architecture introduces constraints that developers
ought to follow. These constraints, when well defined, foster a cleaner
growth on top of it with better practices.

This solution is a crutch meant to alleviate other deep-rooted issues that
should be fixed anyway. It makes them disappear right now, only for
them to reappear when people will write libs and drivers with blurred
hierarchies and hard-to-determine dependencies.

These constraints should be a tool for developers to be critical of their work
and help them see whether they are making a mistake, for example by
putting implementation specific data in generic structures (as seen by
the problems at the root of this RFC: KNI, pmdinfogen dependencies).

This would have led for example eventdev, cryptodev, ethdev to leave PCI
specific data within their structures. Yes, it works. But it is not
clean, it leads to ABI instability, unsafe design practices.

This RFC is the easy way out, making it work, at the price of technical
debt.

I understand that it is a lot of work to properly clean up the
structures and that ressource is scarce. Having a clear architecture and
proper structures however helps external eyes to read, understand, use,
and ultimately contribute.

This kind of move goes against the previous work that was done to
make devices, drivers and buses generic, which I think was right.

I do not feel that it is my place to nack, nor that it is constructive
to block this if others are thinking that it is useful; however I hope
to sway others to my position.

Cheers,
-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [RFC PATCH] mk: symlink every headers first

[Wiles, Keith]

> On Jun 21, 2017, at 10:14 AM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> 
> Hi,
> 
> On Wed, Jun 21, 2017 at 02:27:49PM +0000, Wiles, Keith wrote:
>> 
>>> On Jun 21, 2017, at 5:27 AM, Bruce Richardson <bruce.richardson@intel.com> wrote:
>>> 
>>> On Tue, Jun 20, 2017 at 11:21:39PM +0200, Thomas Monjalon wrote:
>>>> If a library or a build tool uses a definition from a driver,
>>>> there is a build ordering issue, like seen when moving PCI code
>>>> into a bus driver.
>>>> 
>>>> One option is to keep PCI helpers and some common definitions in EAL.
>>>> The other option is to symlink every headers at the beginning of
>>>> the build so they can be included by any other component.
>>>> 
>>>> This patch shows how to achieve the second option.
>>>> 
>>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>>> ---
>>> 
>>> My 2c.
>>> 
>>> This may be worth doing, however, two points to consider.
>>> 
>>> 1. If we look to change build system this may not be worth doing unless
>>> a compelling need becomes obvious in the short term. In the meantime,
>>> for cases where it is needed...
>>> 2. libraries can already access the includes from drivers or other
>>> places fairly easily, just by adding the relevant "-I" flag to the
>>> CFLAGS for that lib.
>>> 
>>> That said, since the work is already done developing this, and if it
>>> doesn't hurt in terms of build time, I suppose we might as well merge
>>> it in.
>>> 
>>> So tentative ack from me, subject to testing and feedback from others.
>> 
>> +1, I already have to make sure everything is symlinked first in my private DPDK work for other reasons. This patch would allow me to remove that special code.
>> 
>>> 
>>> /Bruce
>> 
>> Regards,
>> Keith
>> 
> 
> Personally I do not like this approach.
> 
> A well designed architecture introduces constraints that developers
> ought to follow. These constraints, when well defined, foster a cleaner
> growth on top of it with better practices.
> 
> This solution is a crutch meant to alleviate other deep-rooted issues that
> should be fixed anyway. It makes them disappear right now, only for
> them to reappear when people will write libs and drivers with blurred
> hierarchies and hard-to-determine dependencies.
> 
> These constraints should be a tool for developers to be critical of their work
> and help them see whether they are making a mistake, for example by
> putting implementation specific data in generic structures (as seen by
> the problems at the root of this RFC: KNI, pmdinfogen dependencies).
> 
> This would have led for example eventdev, cryptodev, ethdev to leave PCI
> specific data within their structures. Yes, it works. But it is not
> clean, it leads to ABI instability, unsafe design practices.
> 
> This RFC is the easy way out, making it work, at the price of technical
> debt.
> 
> I understand that it is a lot of work to properly clean up the
> structures and that ressource is scarce. Having a clear architecture and
> proper structures however helps external eyes to read, understand, use,
> and ultimately contribute.
> 
> This kind of move goes against the previous work that was done to
> make devices, drivers and buses generic, which I think was right.
> 
> I do not feel that it is my place to nack, nor that it is constructive
> to block this if others are thinking that it is useful; however I hope
> to sway others to my position.

I do not think this is a crutch as it allows the headers to be included from a single location, instead of relative includes, which can be the worst method.

The real problem is we did not police or restrict usage of structures/includes in the current work or patches. I suggest we start the cleanup of these dependencies and be more strict on what can be included in a feature or file. This way we can have both and it makes it easier for locating headers. We also do not really restrict or create public/private headers. Private headers should never be accessed by another feature/module or exported to the global location. Most if not all of the PMD headers should be private or they need to be split into a public/private set of headers.

> 
> Cheers,
> -- 
> Gaëtan Rivet
> 6WIND

Regards,
Keith