| Message ID | 1384288305-4945-1-git-send-email-mlweber1@rockwellcollins.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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 >
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 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@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
>>>>> "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
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@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@rockwellcollins.com
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@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
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@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@rockwellcollins.com
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
>>>>> "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).
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
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
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
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(-)