* [Buildroot] Move openldap package and enable LDAP server?
@ 2015-12-04 8:13 Andreas Ehmanns
2015-12-04 8:19 ` Thomas Petazzoni
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Ehmanns @ 2015-12-04 8:13 UTC (permalink / raw)
To: buildroot
Dear all,
I was going to use openLDAP on my embedded device and found the
configuration under Target Packages -> Libraries -> Networking.
You can enable openldap which only generates the ldap libraries but have
an option to let the client applications being built.
Since I need the LDAP server I added an option to compile the LDAP
server too. Everything works fine and I thought about submitting the
patch but then I asked me if the configuration location is still the
right place.
Wouldn't it be better to place the openldap configuration under
Target Packages -> Networking Applications
instead of
Target Packages -> Libraries -> Networking
What is your opinion?
Regards,
Andreas Ehmanns
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] Move openldap package and enable LDAP server?
2015-12-04 8:13 [Buildroot] Move openldap package and enable LDAP server? Andreas Ehmanns
@ 2015-12-04 8:19 ` Thomas Petazzoni
2015-12-04 8:21 ` Andreas Ehmanns
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Petazzoni @ 2015-12-04 8:19 UTC (permalink / raw)
To: buildroot
Hello,
On Fri, 4 Dec 2015 09:13:12 +0100, Andreas Ehmanns wrote:
> I was going to use openLDAP on my embedded device and found the
> configuration under Target Packages -> Libraries -> Networking.
> You can enable openldap which only generates the ldap libraries but have
> an option to let the client applications being built.
> Since I need the LDAP server I added an option to compile the LDAP
> server too. Everything works fine and I thought about submitting the
> patch but then I asked me if the configuration location is still the
> right place.
> Wouldn't it be better to place the openldap configuration under
> Target Packages -> Networking Applications
> instead of
> Target Packages -> Libraries -> Networking
>
> What is your opinion?
Yes, I agree that it probably makes more sense to have OpenLDAP in
Networking Applications, even if it indeed also provides a client
library.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] Move openldap package and enable LDAP server?
2015-12-04 8:19 ` Thomas Petazzoni
@ 2015-12-04 8:21 ` Andreas Ehmanns
2015-12-10 19:03 ` Andreas Ehmanns
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Ehmanns @ 2015-12-04 8:21 UTC (permalink / raw)
To: buildroot
Am 04.12.2015 um 09:19 schrieb Thomas Petazzoni:
> Hello,
>
> On Fri, 4 Dec 2015 09:13:12 +0100, Andreas Ehmanns wrote:
>
>> I was going to use openLDAP on my embedded device and found the
>> configuration under Target Packages -> Libraries -> Networking.
>> You can enable openldap which only generates the ldap libraries but have
>> an option to let the client applications being built.
>> Since I need the LDAP server I added an option to compile the LDAP
>> server too. Everything works fine and I thought about submitting the
>> patch but then I asked me if the configuration location is still the
>> right place.
>> Wouldn't it be better to place the openldap configuration under
>> Target Packages -> Networking Applications
>> instead of
>> Target Packages -> Libraries -> Networking
>>
>> What is your opinion?
> Yes, I agree that it probably makes more sense to have OpenLDAP in
> Networking Applications, even if it indeed also provides a client
> library.
>
> Thomas
O.k., I will prepare/propose a patch for adding the ldap server option
and moving the configuration to Networking Applications.
Regards,
Andreas
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] Move openldap package and enable LDAP server?
2015-12-04 8:21 ` Andreas Ehmanns
@ 2015-12-10 19:03 ` Andreas Ehmanns
2015-12-12 13:01 ` Thomas Petazzoni
0 siblings, 1 reply; 7+ messages in thread
From: Andreas Ehmanns @ 2015-12-10 19:03 UTC (permalink / raw)
To: buildroot
Am 04.12.2015 um 09:21 schrieb Andreas Ehmanns:
> Am 04.12.2015 um 09:19 schrieb Thomas Petazzoni:
>> Hello,
>>
>> On Fri, 4 Dec 2015 09:13:12 +0100, Andreas Ehmanns wrote:
>>
>>> I was going to use openLDAP on my embedded device and found the
>>> configuration under Target Packages -> Libraries -> Networking.
>>> You can enable openldap which only generates the ldap libraries but
>>> have
>>> an option to let the client applications being built.
>>> Since I need the LDAP server I added an option to compile the LDAP
>>> server too. Everything works fine and I thought about submitting the
>>> patch but then I asked me if the configuration location is still the
>>> right place.
>>> Wouldn't it be better to place the openldap configuration under
>>> Target Packages -> Networking Applications
>>> instead of
>>> Target Packages -> Libraries -> Networking
>>>
>>> What is your opinion?
>> Yes, I agree that it probably makes more sense to have OpenLDAP in
>> Networking Applications, even if it indeed also provides a client
>> library.
>>
>> Thomas
> O.k., I will prepare/propose a patch for adding the ldap server option
> and moving the configuration to Networking Applications.
>
> Regards,
> Andreas
Thomas,
please find attached the patch for adding the openldap server. It
contains the server option, moves the configuration to "Networking
Applications" and adds an init script for the ldap server.
Please let me know what you think about the patch and if there are
things that I should change.
Regards,
Andreas
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Added-server-to-openldap-package-and-changed-package.patch
Type: text/x-patch
Size: 4722 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20151210/939c16b9/attachment.bin>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] Move openldap package and enable LDAP server?
2015-12-10 19:03 ` Andreas Ehmanns
@ 2015-12-12 13:01 ` Thomas Petazzoni
2015-12-12 17:20 ` Andreas Ehmanns
2015-12-17 20:43 ` Andreas Ehmanns
0 siblings, 2 replies; 7+ messages in thread
From: Thomas Petazzoni @ 2015-12-12 13:01 UTC (permalink / raw)
To: buildroot
Hello Andreas,
On Thu, 10 Dec 2015 20:03:46 +0100, Andreas Ehmanns wrote:
> please find attached the patch for adding the openldap server. It
> contains the server option, moves the configuration to "Networking
> Applications" and adds an init script for the ldap server.
>
> Please let me know what you think about the patch and if there are
> things that I should change.
Thanks. But could you please send the patch with 'git send-email'
rather than as an attachment? It allows the patch to be reviewed
properly, and also easily applied.
A few comments though:
* The commit title is too long. Something like:
openldap: add support to build the server
would be preferable.
* I think you should keep the "default y" on
BR2_PACKAGE_OPENLDAP_CLIENTS in order to preserve the existing
behavior, and not breaking things for current users.
* In the S75sldapd script, I believe checking for the presence of the
daemon and config file is not really needed.
If the -4 in ARGS is to force IPv4 only, I believe it's not really
great. Why not also support IPv6 ?
* There is a fix of tabs and spaces for the indentation in the shell
script, please try to be consistent.
* Please use "printf" rather than "echo -n" in the shell script, since
printf is POSIX, while "echo -n" is not.
* We normally use a PID file, see S50dropbear for example. Is slapd
already managing its own PID file ?
* In the .mk file, the OPENLDAP_USERS definition is inside a condition
that is commented out, it doesn't look good.
* Don't do ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n), but just ifeq
($(BR2_PACKAGE_OPENLDAP_SERVER),). An option will never have the
value 'n': when an option is disabled, its value is the empty string.
Could you take those comments into account and send an updated version?
Thanks a lot!
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] Move openldap package and enable LDAP server?
2015-12-12 13:01 ` Thomas Petazzoni
@ 2015-12-12 17:20 ` Andreas Ehmanns
2015-12-17 20:43 ` Andreas Ehmanns
1 sibling, 0 replies; 7+ messages in thread
From: Andreas Ehmanns @ 2015-12-12 17:20 UTC (permalink / raw)
To: buildroot
Hi Thomas,
Am 12.12.2015 um 14:01 schrieb Thomas Petazzoni:
> Hello Andreas,
>
> On Thu, 10 Dec 2015 20:03:46 +0100, Andreas Ehmanns wrote:
>
>> please find attached the patch for adding the openldap server. It
>> contains the server option, moves the configuration to "Networking
>> Applications" and adds an init script for the ldap server.
>>
>> Please let me know what you think about the patch and if there are
>> things that I should change.
> Thanks. But could you please send the patch with 'git send-email'
> rather than as an attachment? It allows the patch to be reviewed
> properly, and also easily applied.
>
> A few comments though:
>
> * The commit title is too long. Something like:
>
> openldap: add support to build the server
>
> would be preferable.
>
> * I think you should keep the "default y" on
> BR2_PACKAGE_OPENLDAP_CLIENTS in order to preserve the existing
> behavior, and not breaking things for current users.
>
> * In the S75sldapd script, I believe checking for the presence of the
> daemon and config file is not really needed.
>
> If the -4 in ARGS is to force IPv4 only, I believe it's not really
> great. Why not also support IPv6 ?
>
> * There is a fix of tabs and spaces for the indentation in the shell
> script, please try to be consistent.
>
> * Please use "printf" rather than "echo -n" in the shell script, since
> printf is POSIX, while "echo -n" is not.
>
> * We normally use a PID file, see S50dropbear for example. Is slapd
> already managing its own PID file ?
>
> * In the .mk file, the OPENLDAP_USERS definition is inside a condition
> that is commented out, it doesn't look good.
>
> * Don't do ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n), but just ifeq
> ($(BR2_PACKAGE_OPENLDAP_SERVER),). An option will never have the
> value 'n': when an option is disabled, its value is the empty string.
>
> Could you take those comments into account and send an updated version?
>
> Thanks a lot!
>
> Thomas
thanks for your comments. I will incorporate the changes next week and
send a patch using git.
Regards,
Andreas
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Buildroot] Move openldap package and enable LDAP server?
2015-12-12 13:01 ` Thomas Petazzoni
2015-12-12 17:20 ` Andreas Ehmanns
@ 2015-12-17 20:43 ` Andreas Ehmanns
1 sibling, 0 replies; 7+ messages in thread
From: Andreas Ehmanns @ 2015-12-17 20:43 UTC (permalink / raw)
To: buildroot
Thomas, All,
Am 12.12.2015 um 14:01 schrieb Thomas Petazzoni:
> Hello Andreas,
>
> On Thu, 10 Dec 2015 20:03:46 +0100, Andreas Ehmanns wrote:
>
>> please find attached the patch for adding the openldap server. It
>> contains the server option, moves the configuration to "Networking
>> Applications" and adds an init script for the ldap server.
>>
>> Please let me know what you think about the patch and if there are
>> things that I should change.
> Thanks. But could you please send the patch with 'git send-email'
> rather than as an attachment? It allows the patch to be reviewed
> properly, and also easily applied.
>
> A few comments though:
>
> * The commit title is too long. Something like:
>
> openldap: add support to build the server
>
> would be preferable.
>
> * I think you should keep the "default y" on
> BR2_PACKAGE_OPENLDAP_CLIENTS in order to preserve the existing
> behavior, and not breaking things for current users.
>
> * In the S75sldapd script, I believe checking for the presence of the
> daemon and config file is not really needed.
>
> If the -4 in ARGS is to force IPv4 only, I believe it's not really
> great. Why not also support IPv6 ?
>
> * There is a fix of tabs and spaces for the indentation in the shell
> script, please try to be consistent.
>
> * Please use "printf" rather than "echo -n" in the shell script, since
> printf is POSIX, while "echo -n" is not.
>
> * We normally use a PID file, see S50dropbear for example. Is slapd
> already managing its own PID file ?
>
> * In the .mk file, the OPENLDAP_USERS definition is inside a condition
> that is commented out, it doesn't look good.
>
> * Don't do ifeq ($(BR2_PACKAGE_OPENLDAP_SERVER),n), but just ifeq
> ($(BR2_PACKAGE_OPENLDAP_SERVER),). An option will never have the
> value 'n': when an option is disabled, its value is the empty string.
>
> Could you take those comments into account and send an updated version?
>
> Thanks a lot!
>
> Thomas
I incorporated your proposed changes and re-sent the patch using git.
Note: slapd is managing it's own PID file. So there was no need to add
this to the init script.
Regards,
Andreas
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-12-17 20:43 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-04 8:13 [Buildroot] Move openldap package and enable LDAP server? Andreas Ehmanns
2015-12-04 8:19 ` Thomas Petazzoni
2015-12-04 8:21 ` Andreas Ehmanns
2015-12-10 19:03 ` Andreas Ehmanns
2015-12-12 13:01 ` Thomas Petazzoni
2015-12-12 17:20 ` Andreas Ehmanns
2015-12-17 20:43 ` Andreas Ehmanns
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.