[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Matt Weber]

Signed-off-by: Matt Weber <mlweber1@rockwellcollins.com>
---
 package/omniorb/Config.in  |    9 +++++++++
 package/omniorb/omniorb.mk |    8 ++++++++
 2 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/package/omniorb/Config.in b/package/omniorb/Config.in
index 6326688..22527fc 100644
--- a/package/omniorb/Config.in
+++ b/package/omniorb/Config.in
@@ -10,5 +10,14 @@ config BR2_PACKAGE_OMNIORB
 
 	  http://omniorb.sourceforge.net/
 
+if BR2_PACKAGE_OMNIORB
+
+config BR2_PACKAGE_OMNIORB_WITH_SERVICES
+        bool "COS Naming Service"
+        default y
+        help
+          omniORB COS Naming Service
+endif
+
 comment "omniORB needs a toolchain w/ C++"
 	depends on !BR2_INSTALL_LIBSTDCPP
diff --git a/package/omniorb/omniorb.mk b/package/omniorb/omniorb.mk
index 490ff93..cad50f8 100644
--- a/package/omniorb/omniorb.mk
+++ b/package/omniorb/omniorb.mk
@@ -24,6 +24,14 @@ OMNIORB_INSTALL_TARGET = YES
 OMNIORB_CONF_OPT += --disable-longdouble
 HOST_OMNIORB_CONF_OPT += --disable-longdouble
 
+define OMNIORB_ENABLE_SERVICES
+	$(SED) 's:SUBDIRS += lib:SUBDIRS += lib services:g' $(@D)/src/dir.mk
+endef
+
+ifeq ($(BR2_PACKAGE_OMNIORB_WITH_SERVICES),y)
+	OMNIORB_POST_CONFIGURE_HOOKS += OMNIORB_ENABLE_SERVICES
+endif
+
 # omniORB is not completely cross-compile friendly and has some
 # assumptions where a couple host tools must be built and then
 # used by the target build.  The host tools generate code from
-- 
1.7.1

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Thomas De Schampheleire]

Hi Matt,

Matt Weber <mlweber1@rockwellcollins.com> wrote:
>
>Signed-off-by: Matt Weber <mlweber1@rockwellcollins.com>
>---
> package/omniorb/Config.in  |    9 +++++++++
> package/omniorb/omniorb.mk |    8 ++++++++
> 2 files changed, 17 insertions(+), 0 deletions(-)
>
>diff --git a/package/omniorb/Config.in b/package/omniorb/Config.in
>index 6326688..22527fc 100644
>--- a/package/omniorb/Config.in
>+++ b/package/omniorb/Config.in
>@@ -10,5 +10,14 @@ config BR2_PACKAGE_OMNIORB
> 
> 	  http://omniorb.sourceforge.net/
> 
>+if BR2_PACKAGE_OMNIORB
>+
>+config BR2_PACKAGE_OMNIORB_WITH_SERVICES
>+        bool "COS Naming Service"
>+        default y
>+        help
>+          omniORB COS Naming Service
>+endif
>+
> comment "omniORB needs a toolchain w/ C++"
> 	depends on !BR2_INSTALL_LIBSTDCPP

I think it makes more sense to keep this comment close to the config option it applies to, thus moving the new cos option below it.
I know that many packages do not follow this, but I'm planning on fixing that...

Best regards,
Thomas

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Arnout Vandecappelle]

On 12/11/13 21:31, Matt Weber wrote:
>
> Signed-off-by: Matt Weber <mlweber1@rockwellcollins.com>
> ---
>   package/omniorb/Config.in  |    9 +++++++++
>   package/omniorb/omniorb.mk |    8 ++++++++
>   2 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/package/omniorb/Config.in b/package/omniorb/Config.in
> index 6326688..22527fc 100644
> --- a/package/omniorb/Config.in
> +++ b/package/omniorb/Config.in
> @@ -10,5 +10,14 @@ config BR2_PACKAGE_OMNIORB
>
>   	  http://omniorb.sourceforge.net/
>
> +if BR2_PACKAGE_OMNIORB
> +
> +config BR2_PACKAGE_OMNIORB_WITH_SERVICES
> +        bool "COS Naming Service"
> +        default y
> +        help
> +          omniORB COS Naming Service
> +endif
> +
>   comment "omniORB needs a toolchain w/ C++"
>   	depends on !BR2_INSTALL_LIBSTDCPP
> diff --git a/package/omniorb/omniorb.mk b/package/omniorb/omniorb.mk
> index 490ff93..cad50f8 100644
> --- a/package/omniorb/omniorb.mk
> +++ b/package/omniorb/omniorb.mk
> @@ -24,6 +24,14 @@ OMNIORB_INSTALL_TARGET = YES
>   OMNIORB_CONF_OPT += --disable-longdouble
>   HOST_OMNIORB_CONF_OPT += --disable-longdouble
>
> +define OMNIORB_ENABLE_SERVICES
> +	$(SED) 's:SUBDIRS += lib:SUBDIRS += lib services:g' $(@D)/src/dir.mk
> +endef
> +
> +ifeq ($(BR2_PACKAGE_OMNIORB_WITH_SERVICES),y)
> +	OMNIORB_POST_CONFIGURE_HOOKS += OMNIORB_ENABLE_SERVICES

  We normally don't indent variable assignments within conditions (the 
indentation of the sed statement is OK, because that's a shell command).

  Also, wouldn't it be better to do this as post-patch hook?

  Regards,
  Arnout

> +endif
> +
>   # omniORB is not completely cross-compile friendly and has some
>   # assumptions where a couple host tools must be built and then
>   # used by the target build.  The host tools generate code from
>


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Thomas Petazzoni]

Dear Matt Weber,

On Tue, 12 Nov 2013 14:31:45 -0600, Matt Weber wrote:
> 
> Signed-off-by: Matt Weber <mlweber1@rockwellcollins.com>
> ---
>  package/omniorb/Config.in  |    9 +++++++++
>  package/omniorb/omniorb.mk |    8 ++++++++
>  2 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/package/omniorb/Config.in b/package/omniorb/Config.in
> index 6326688..22527fc 100644
> --- a/package/omniorb/Config.in
> +++ b/package/omniorb/Config.in
> @@ -10,5 +10,14 @@ config BR2_PACKAGE_OMNIORB
>  
>  	  http://omniorb.sourceforge.net/
>  
> +if BR2_PACKAGE_OMNIORB
> +
> +config BR2_PACKAGE_OMNIORB_WITH_SERVICES
> +        bool "COS Naming Service"
> +        default y
> +        help
> +          omniORB COS Naming Service

Use tab for indentation (and one tab + two spaces for the help text).

> +define OMNIORB_ENABLE_SERVICES
> +	$(SED) 's:SUBDIRS += lib:SUBDIRS += lib services:g' $(@D)/src/dir.mk
> +endef
> +
> +ifeq ($(BR2_PACKAGE_OMNIORB_WITH_SERVICES),y)
> +	OMNIORB_POST_CONFIGURE_HOOKS += OMNIORB_ENABLE_SERVICES
> +endif

This is kind of weird, because the "services" directory is built in
some condition:

ifndef EmbeddedSystem
SUBDIRS += appl services
endif

but you define EmbeddedSystem=1 in Buildroot:

echo "EmbeddedSystem=1" >> $(@D)/mk/beforeauto.mk

Is there a reason for defining EmbeddedSystem=1 ? The only difference
will be that it's going to build the stuff in src/appl/, but looking at
this directory, it doesn't seem like a huge amount of code compared to
the overall size of omniORB.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Matthew Weber]

Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote on 11/12/2013 
02:54:50 PM:

> From: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> To: Matt Weber <mlweber1@rockwellcollins.com>, buildroot at busybox.net
> Date: 11/12/2013 02:55 PM
> Subject: Re: [Buildroot] [PATCH 1/1] omniorb: add COS Naming Service
> 
> Hi Matt,
> 
> Matt Weber <mlweber1@rockwellcollins.com> wrote:
> >
> >Signed-off-by: Matt Weber <mlweber1@rockwellcollins.com>
> >---
> > package/omniorb/Config.in  |    9 +++++++++
> > package/omniorb/omniorb.mk |    8 ++++++++
> > 2 files changed, 17 insertions(+), 0 deletions(-)
> >
> >diff --git a/package/omniorb/Config.in b/package/omniorb/Config.in
> >index 6326688..22527fc 100644
> >--- a/package/omniorb/Config.in
> >+++ b/package/omniorb/Config.in
> >@@ -10,5 +10,14 @@ config BR2_PACKAGE_OMNIORB
> > 
> >      http://omniorb.sourceforge.net/
> > 
> >+if BR2_PACKAGE_OMNIORB
> >+
> >+config BR2_PACKAGE_OMNIORB_WITH_SERVICES
> >+        bool "COS Naming Service"
> >+        default y
> >+        help
> >+          omniORB COS Naming Service
> >+endif
> >+
> > comment "omniORB needs a toolchain w/ C++"
> >    depends on !BR2_INSTALL_LIBSTDCPP
> 
> I think it makes more sense to keep this comment close to the config
> option it applies to, thus moving the new cos option below it.
> I know that many packages do not follow this, but I'm planning on 
> fixing that...
> 
> Best regards,
> Thomas
> 
Sure that works.  I noticed the indenting of the new cfg option doesn't 
seem 
to happen in the menuconfig if I move that comment below the original 
package.
I'll have to dig into why that happens.

Thanks,
Matt 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20131112/c1fd93e8/attachment.html>

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Peter Korsgaard]

>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

Hi,

>> +if BR2_PACKAGE_OMNIORB
>> +
>> +config BR2_PACKAGE_OMNIORB_WITH_SERVICES
>> +        bool "COS Naming Service"
>> +        default y
>> +        help
>> +          omniORB COS Naming Service
>> +endif
>> +
>> comment "omniORB needs a toolchain w/ C++"
>> depends on !BR2_INSTALL_LIBSTDCPP

> I think it makes more sense to keep this comment close to the config
> option it applies to, thus moving the new cos option below it.  I know
> that many packages do not follow this, but I'm planning on fixing
> that...

Careful, kconfig needs sub options to be directly under the main option,
otherwise they don't get indented correctly in menuconfig, so your
options are either:

- Comment before main option
- Comment after all the sub options

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Matthew Weber]

Arnout Vandecappelle <arnout@mind.be> wrote on 11/12/2013 03:25:40 PM:

> From: Arnout Vandecappelle <arnout@mind.be>
> To: Matt Weber <mlweber1@rockwellcollins.com>
> Cc: buildroot at busybox.net
> Date: 11/12/2013 03:25 PM
> Subject: Re: [Buildroot] [PATCH 1/1] omniorb: add COS Naming Service
> 
> On 12/11/13 21:31, Matt Weber wrote:
> >
<snip>
> >
> > +define OMNIORB_ENABLE_SERVICES
> > +   $(SED) 's:SUBDIRS += lib:SUBDIRS += lib services:g' 
$(@D)/src/dir.mk
> > +endef
> > +
> > +ifeq ($(BR2_PACKAGE_OMNIORB_WITH_SERVICES),y)
> > +   OMNIORB_POST_CONFIGURE_HOOKS += OMNIORB_ENABLE_SERVICES
> 
>   We normally don't indent variable assignments within conditions (the 
> indentation of the sed statement is OK, because that's a shell command).
> 

Sure, I'll clean those up.

>   Also, wouldn't it be better to do this as post-patch hook?

In this case, yes.  The file is not modified during configure.  I'll 
update.

Thanks!
Matt Weber
mlweber1 at rockwellcollins.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20131112/6e73bca7/attachment.html>

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Matthew Weber]

Peter Korsgaard <jacmet@gmail.com> wrote on 11/12/2013 04:24:10 PM:

> From: Peter Korsgaard <jacmet@uclibc.org>
> To: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> Cc: Matt Weber <mlweber1@rockwellcollins.com>, buildroot at busybox.net
> Date: 11/12/2013 04:24 PM
> Subject: Re: [PATCH 1/1] omniorb: add COS Naming Service
> Sent by: Peter Korsgaard <jacmet@gmail.com>
> 
> >>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> 
writes:
> 
> Hi,
> 
> >> +if BR2_PACKAGE_OMNIORB
> >> +
> >> +config BR2_PACKAGE_OMNIORB_WITH_SERVICES
> >> +        bool "COS Naming Service"
> >> +        default y
> >> +        help
> >> +          omniORB COS Naming Service
> >> +endif
> >> +
> >> comment "omniORB needs a toolchain w/ C++"
> >> depends on !BR2_INSTALL_LIBSTDCPP
> 
> > I think it makes more sense to keep this comment close to the config
> > option it applies to, thus moving the new cos option below it.  I know
> > that many packages do not follow this, but I'm planning on fixing
> > that...
> 
> Careful, kconfig needs sub options to be directly under the main option,
> otherwise they don't get indented correctly in menuconfig, so your
> options are either:

Thanks for the clarification, I was wondering if that was needed.
I just tested moving it above and that seems to be more readable then 
below.

Thanks,
Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20131112/bcb5fdd1/attachment.html>

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Matthew Weber]

Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote on 11/12/2013 
03:54:56 PM:

> From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> To: Matt Weber <mlweber1@rockwellcollins.com>
> Cc: buildroot at busybox.net
> Date: 11/12/2013 03:55 PM
> Subject: Re: [Buildroot] [PATCH 1/1] omniorb: add COS Naming Service
> 
> Dear Matt Weber,
> 
> On Tue, 12 Nov 2013 14:31:45 -0600, Matt Weber wrote:
> > 
> > Signed-off-by: Matt Weber <mlweber1@rockwellcollins.com>
> > ---
> >  package/omniorb/Config.in  |    9 +++++++++
> >  package/omniorb/omniorb.mk |    8 ++++++++
> >  2 files changed, 17 insertions(+), 0 deletions(-)
> > 
> > diff --git a/package/omniorb/Config.in b/package/omniorb/Config.in
> > index 6326688..22527fc 100644
> > --- a/package/omniorb/Config.in
> > +++ b/package/omniorb/Config.in
> > @@ -10,5 +10,14 @@ config BR2_PACKAGE_OMNIORB
> > 
> >       http://omniorb.sourceforge.net/
> > 
> > +if BR2_PACKAGE_OMNIORB
> > +
> > +config BR2_PACKAGE_OMNIORB_WITH_SERVICES
> > +        bool "COS Naming Service"
> > +        default y
> > +        help
> > +          omniORB COS Naming Service
> 
> Use tab for indentation (and one tab + two spaces for the help text).

Ack

> 
> > +define OMNIORB_ENABLE_SERVICES
> > +   $(SED) 's:SUBDIRS += lib:SUBDIRS += lib services:g' 
$(@D)/src/dir.mk
> > +endef
> > +
> > +ifeq ($(BR2_PACKAGE_OMNIORB_WITH_SERVICES),y)
> > +   OMNIORB_POST_CONFIGURE_HOOKS += OMNIORB_ENABLE_SERVICES
> > +endif
> 
> This is kind of weird, because the "services" directory is built in
> some condition:
> 
> ifndef EmbeddedSystem
> SUBDIRS += appl services
> endif
> 
> but you define EmbeddedSystem=1 in Buildroot:
> 
> echo "EmbeddedSystem=1" >> $(@D)/mk/beforeauto.mk
> 
> Is there a reason for defining EmbeddedSystem=1 ? The only difference
> will be that it's going to build the stuff in src/appl/, but looking at
> this directory, it doesn't seem like a huge amount of code compared to
> the overall size of omniORB.

Yeah, the EmbeddedSystems option trims down the build and doesn't build 
a bunch of host tools, extra libs, and apps that aren't required on the 
target. 
So for majority of the configuration it seems like the best option (used 
to trim
down src/, src/lib, src/lib/omniORB).  It also removed the example apps 
that 
don't cross-compile... 
So to add this option we just added back in the COS Naming service 
as an option instead of doing a complete build to get the app/lib.

Thanks,
Matt 
mlweber1 at rockwellcollins.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20131112/8f17f651/attachment.html>

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Thomas De Schampheleire]

On Tue, Nov 12, 2013 at 11:24 PM, Peter Korsgaard <jacmet@uclibc.org> wrote:
>>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:
>
> Hi,
>
>>> +if BR2_PACKAGE_OMNIORB
>>> +
>>> +config BR2_PACKAGE_OMNIORB_WITH_SERVICES
>>> +        bool "COS Naming Service"
>>> +        default y
>>> +        help
>>> +          omniORB COS Naming Service
>>> +endif
>>> +
>>> comment "omniORB needs a toolchain w/ C++"
>>> depends on !BR2_INSTALL_LIBSTDCPP
>
>> I think it makes more sense to keep this comment close to the config
>> option it applies to, thus moving the new cos option below it.  I know
>> that many packages do not follow this, but I'm planning on fixing
>> that...
>
> Careful, kconfig needs sub options to be directly under the main option,
> otherwise they don't get indented correctly in menuconfig, so your
> options are either:
>
> - Comment before main option
> - Comment after all the sub options
>

Yuk, that's a pity...
In this case my preference is to have the comment before the main
option (so as close as possible), rather than after all sub options.

What is your opinion?

Thanks,
Thomas

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Peter Korsgaard]

>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

Hi,

> Yuk, that's a pity...
> In this case my preference is to have the comment before the main
> option (so as close as possible), rather than after all sub options.

> What is your opinion?

I don't have a stong opion about it, but that's fine by me as well. For
simple packages (most), it imho is a bit more logical to have it after
the option though (and it's what we are currently doing most places).

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Thomas De Schampheleire]

On Wed, Nov 13, 2013 at 10:50 AM, Peter Korsgaard <jacmet@uclibc.org> wrote:
>>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:
>
> Hi,
>
>> Yuk, that's a pity...
>> In this case my preference is to have the comment before the main
>> option (so as close as possible), rather than after all sub options.
>
>> What is your opinion?
>
> I don't have a stong opion about it, but that's fine by me as well. For
> simple packages (most), it imho is a bit more logical to have it after
> the option though (and it's what we are currently doing most places).
>

As long as there are no suboptions that is indeed more logical.
However, when suboptions are added the comment moves down, and
depending on where new suboptions are added this may not be apparent
from the diff output alone. So one way is to use one 'rule' which
would be the comment above, or we keep simple packages without
suboptions with the comment below, and make sure the comment is moved
to the top when suboptions are added. The first alternative is easier
to maintain, but less beautiful for the simple packages without
suboptions.

If I had to pick, I'd pick the first alternative, for easier
maintainability and uniformity.

Best regards,
Thomas

[Buildroot] [PATCH 1/1] omniorb: add COS Naming Service

[Matthew Weber]

Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote on 11/13/2013 
03:59:43 AM:

> From: Thomas De Schampheleire <patrickdepinguin@gmail.com>
> To: Peter Korsgaard <jacmet@uclibc.org>
> Cc: Matt Weber <mlweber1@rockwellcollins.com>, buildroot 
> <buildroot@busybox.net>
> Date: 11/13/2013 03:59 AM
> Subject: Re: [PATCH 1/1] omniorb: add COS Naming Service
> 
> On Wed, Nov 13, 2013 at 10:50 AM, Peter Korsgaard <jacmet@uclibc.org> 
wrote:
> >>>>>> "Thomas" == Thomas De Schampheleire 
> <patrickdepinguin@gmail.com> writes:
> >
> > Hi,
> >
> >> Yuk, that's a pity...
> >> In this case my preference is to have the comment before the main
> >> option (so as close as possible), rather than after all sub options.
> >
> >> What is your opinion?
> >
> > I don't have a stong opion about it, but that's fine by me as well. 
For
> > simple packages (most), it imho is a bit more logical to have it after
> > the option though (and it's what we are currently doing most places).
> >
> 
> As long as there are no suboptions that is indeed more logical.
> However, when suboptions are added the comment moves down, and
> depending on where new suboptions are added this may not be apparent
> from the diff output alone. So one way is to use one 'rule' which
> would be the comment above, or we keep simple packages without
> suboptions with the comment below, and make sure the comment is moved
> to the top when suboptions are added. The first alternative is easier
> to maintain, but less beautiful for the simple packages without
> suboptions.
> 
> If I had to pick, I'd pick the first alternative, for easier
> maintainability and uniformity.
I agree, I would have liked to keep it right below the main option,
but above is a reasonable location, since if it was placed below all 
suboptions I could see it get confusing.

Thanks,
Matt

> 
> Best regards,
> Thomas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20131113/fa8f7234/attachment.html>