[Buildroot] [1/1] civetweb: new package

[Buildroot] [1/1] civetweb: new package

[Thomas Davis]

Sorry if this a repeat.  I tried submitting this via git like specified in
the documentation but I think it was not working.  I checked the archive
and there was no posting for it and not on pachwork either.

Thomas

[Buildroot] [1/1] civetweb: new package

[Thomas De Schampheleire]

Hi Thomas,

On Tue, Aug 27, 2013 at 3:32 PM, Thomas Davis <sunsetbrew@sunsetbrew.com> wrote:
> Sorry if this a repeat.  I tried submitting this via git like specified in
> the documentation but I think it was not working.  I checked the archive and
> there was no posting for it and not on pachwork either.
>
> Thomas
>
> From 2dbba731f6b647ee0943c4fa46756f1f8c4a76b5 Mon Sep 17 00:00:00 2001
> From: Thomas Davis <sunsetbrew@sunsetbrew.com>
> Date: Sat, 24 Aug 2013 06:43:28 -0400
> Subject: [PATCH 1/1] Added civetweb package
>
>
> Signed-off-by: Thomas Davis <sunsetbrew@sunsetbrew.com>
> ---
>  package/Config.in            |    1 +
>  package/civetweb/Config.in   |   34 +++++++++++++++++++++++++++++
>  package/civetweb/civetweb.mk |   48
> ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 83 insertions(+), 0 deletions(-)
>  create mode 100644 package/civetweb/Config.in
>  create mode 100644 package/civetweb/civetweb.mk
>
> diff --git a/package/Config.in b/package/Config.in
> index 7069d77..25433b6 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -719,6 +719,7 @@ source "package/bridge-utils/Config.in"
>  source "package/bwm-ng/Config.in"
>  source "package/can-utils/Config.in"
>  source "package/chrony/Config.in"
> +source "package/civetweb/Config.in"
>  source "package/connman/Config.in"
>  source "package/crda/Config.in"
>  source "package/ctorrent/Config.in"
> diff --git a/package/civetweb/Config.in b/package/civetweb/Config.in
> new file mode 100644
> index 0000000..5285b09
> --- /dev/null
> +++ b/package/civetweb/Config.in
> @@ -0,0 +1,34 @@
> +config BR2_PACKAGE_CIVETWEB
> + bool "civetweb"
> + depends on BR2_TOOLCHAIN_BUILDROOT_UCLIBC && !BR2_PTHREADS_NONE

What is the reason for this uclibc check?

> + help
> +  Full featured embedded web server with LUA support.
> +
> +  https://sourceforge.net/projects/civetweb
> +
> +if BR2_PACKAGE_CIVETWEB
> +
> + config BR2_CIVETWEB_WITH_IPV6
> + bool "enable IPV6 support"
> + depends on BR2_INET_IPV6
> +
> + config BR2_CIVETWEB_WITH_LUA
> + bool "enable LUA support"
> + depends on BR2_LARGEFILE # util-linux
> +
> + config BR2_CIVETWEB_WITH_SSL
> + bool "enable SSL support"
> + depends on BR2_PACKAGE_OPENSSL
> +
> + comment "civetweb IPV6 support requires IPV6"
> + depends on !BR2_INET_IPV6
> +
> + comment "civetweb LUA support requires large file support"
> + depends on !BR2_LARGEFILE
> +
> + comment "civetweb SSL support requires OpenSSL"
> + depends on !BR2_PACKAGE_OPENSSL
> +endif
> +
> +comment "civetweb requires a toolchain with PTHREAD support"
> + depends on !BR2_TOOLCHAIN_BUILDROOT_UCLIBC || BR2_PTHREADS_NONE
> diff --git a/package/civetweb/civetweb.mk b/package/civetweb/civetweb.mk
> new file mode 100644
> index 0000000..3f81aeb
> --- /dev/null
> +++ b/package/civetweb/civetweb.mk
> @@ -0,0 +1,48 @@
> +#
> +# Copyright (c) 2013 No Face Press, LLC
> +# License http://opensource.org/licenses/mit-license.php MIT License

Could you explain why you added this comment?
Do you want to add your copyright statement to this .mk file?

And what about the MIT license? Buildroot is GPL, so any .mk file in
it will also be GPL. Do you want to dual-license this file?

> +#
> +################################################################################
> +#
> +# civetweb
> +#
> +################################################################################
> +
> +CIVETWEB_VERSION = 1.2
> +CIVETWEB_SOURCE = civetweb-$(CIVETWEB_VERSION).tar.gz
> +CIVETWEB_SITE =
> http://github.com/sunsetbrew/civetweb/tarball/v$(CIVETWEB_VERSION)
> +CIVETWEB_LICENSE = MIT
> +CIVETWEB_LICENSE_FILES = LICENSE.md
> +
> +CIVETWEB_COPT = $(TARGET_CFLAGS)
> +CIVETWEB_MOPT = TARGET_OS=LINUX
> +CIVETWEB_LDFLAGS = $(TARGET_LDFLAGS)
> +
> +ifndef BR2_PACKAGE_UTIL_LINUX_FALLOCATE
> + CIVETWEB_COPT += -DHAVE_POSIX_FALLOCATE=0
> +endif
> +
> +ifdef BR2_CIVETWEB_WITH_IPV6
> + CIVETWEB_MOPT += WITH_IPV6=1
> +endif
> +
> +ifdef BR2_CIVETWEB_WITH_LUA
> + CIVETWEB_MOPT += WITH_LUA=1
> +endif
> +
> +ifdef BR2_CIVETWEB_WITH_SSL
> + CIVETWEB_COPT += -DNO_SSL_DL -lcrypt -lssl
> + CIVETWEB_DEPENDENCIES += openssl
> +else
> + CIVETWEB_COPT += -DNO_SSL
> +endif

In general we use the format
ifeq ($(BR2_FOO),y)
...
endif

instead of
ifdef BR2_FOO
...
endif

I don't know if there is a particular reason for it, but I would
advocate to use that for consistency reasons.

> +
> +define CIVETWEB_BUILD_CMDS
> + $(MAKE) CC="$(TARGET_CC)" -C $(@D) all $(CIVETWEB_MOPT)
> COPT="$(CIVETWEB_COPT)"
> +endef
> +
> +define CIVETWEB_INSTALL_TARGET_CMDS
> + $(MAKE) CC="$(TARGET_CC)" -C $(@D) install
> DOCUMENT_ROOT=/usr/local/share/doc/civetweb PREFIX="$(TARGET_DIR)/usr/local"
> $(CIVETWEB_MOPT) COPT='$(CIVETWEB_COPT)'
> +endef
> +
> +$(eval $(generic-package))
> \ No newline at end of file

There is no newline at the end of your .mk file, this should be fixed too.

Best regards,
Thomas

[Buildroot] [1/1] civetweb: new package

[Thomas Petazzoni]

Thomas, Thomas,

On Tue, 27 Aug 2013 16:27:39 +0200, Thomas De Schampheleire wrote:

> > +$(eval $(generic-package))
> > \ No newline at end of file
> 
> There is no newline at the end of your .mk file, this should be fixed too.

And also generally speaking, the patch should be sent using 'git
send-email', because HTML formatted patches that are line wrapped are
not really practical to be applied on Buildroot.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas Davis]

Thank you so much for the quick review!

>What is the reason for this uclibc check?

I noticed that there were several alternatives for pthreads in uclibc and
the only one that not compatible is BR2_PTHREADS_NONE.  I was afraid of
additional variants may occur in the future and I am basically only
excluding the one option.  Further, if it is possible to exclude uclibc, it
would not be compatible either.  If you feel it is overkill I can simply it.

> Could you explain why you added this comment?
Civetweb is MIT and the MIT license does not prevent it from being included
in GPL code while the inverse is not true. If I put GPL code in Civetweb,
it compromises the MIT license and currently a copy of these files are in
the repository, at least until included with buildroot.  If dual licensing
the file is the best way, I can do that, or if preferred I can remove the
statement altogether.

> In general we use the format
> ifeq ($(BR2_FOO),y)

No problem, i can fix that

>  \ No newline at end of file

will fix that too

Please advise on the uclib check and the license header and I will submit a
new patch.

Thank you,
Thomas Davis
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20130827/66790200/attachment-0001.html>

[Buildroot] [1/1] civetweb: new package

[Thomas Petazzoni]

Dear Thomas Davis,

Please don't reply to me directly, always keep the list in Cc for
Buildroot related discussions that started on the list.

On Tue, 27 Aug 2013 11:39:17 -0400, Thomas Davis wrote:

> I tried git send-mail twice before and it did not make it to the list.  I
> am guessing there is some spam guard in place that is preventing it because
> I received the cc'ed copies just fine.  Have you seen this before?

Are you sure the From: address that was shown when using 'git
send-email' is really the address with which you've subscribed to the
list? I strongly suspect something like that, many other contributors
are using git send-email with the Buildroot mailing list without any
problem.

Can you try to 'git send-email' me your patch, so that I can see if
something obvious prevents your e-mail from reaching the list?

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas Davis]

Submission attempt #2 with recommended changes.

1. Change make file ifdef's to ifeq.
2. Copyright lines removed to prevent confusion.
3. Put end of line in make file.
4. Submitted with git mail if it works, otherwise in plain text
5. Rebased, rebuilt and retested with buildroot.

The explaination of BR2_TOOLCHAIN_BUILDROOT_UCLIBC check in
Config.in was answered in a previous post.  If that will
always be included, I can remove the check.

BTW:  git --send-mail is not working for me.  But I made sure this
mail is in plain-text mode.

Thanks,
Thomas

Signed-off-by: Thomas Davis <sunsetbrew@sunsetbrew.com>
---
 package/Config.in            |    1 +
 package/civetweb/Config.in   |   34 +++++++++++++++++++++++++++++++
 package/civetweb/civetweb.mk |   45 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 80 insertions(+), 0 deletions(-)
 create mode 100644 package/civetweb/Config.in
 create mode 100644 package/civetweb/civetweb.mk

diff --git a/package/Config.in b/package/Config.in
index 7069d77..25433b6 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -719,6 +719,7 @@ source "package/bridge-utils/Config.in"
 source "package/bwm-ng/Config.in"
 source "package/can-utils/Config.in"
 source "package/chrony/Config.in"
+source "package/civetweb/Config.in"
 source "package/connman/Config.in"
 source "package/crda/Config.in"
 source "package/ctorrent/Config.in"
diff --git a/package/civetweb/Config.in b/package/civetweb/Config.in
new file mode 100644
index 0000000..5285b09
--- /dev/null
+++ b/package/civetweb/Config.in
@@ -0,0 +1,34 @@
+config BR2_PACKAGE_CIVETWEB
+       bool "civetweb"
+       depends on BR2_TOOLCHAIN_BUILDROOT_UCLIBC && !BR2_PTHREADS_NONE
+       help
+         Full featured embedded web server with LUA support.
+
+         https://sourceforge.net/projects/civetweb
+
+if BR2_PACKAGE_CIVETWEB
+
+       config BR2_CIVETWEB_WITH_IPV6
+               bool "enable IPV6 support"
+               depends on BR2_INET_IPV6
+
+       config BR2_CIVETWEB_WITH_LUA
+               bool "enable LUA support"
+               depends on BR2_LARGEFILE # util-linux
+
+       config BR2_CIVETWEB_WITH_SSL
+               bool "enable SSL support"
+               depends on BR2_PACKAGE_OPENSSL
+
+       comment "civetweb IPV6 support requires IPV6"
+               depends on !BR2_INET_IPV6
+
+       comment "civetweb LUA support requires large file support"
+               depends on !BR2_LARGEFILE
+
+       comment "civetweb SSL support requires OpenSSL"
+               depends on !BR2_PACKAGE_OPENSSL
+endif
+
+comment "civetweb requires a toolchain with PTHREAD support"
+       depends on !BR2_TOOLCHAIN_BUILDROOT_UCLIBC || BR2_PTHREADS_NONE
diff --git a/package/civetweb/civetweb.mk b/package/civetweb/civetweb.mk
new file mode 100644
index 0000000..db14b2b
--- /dev/null
+++ b/package/civetweb/civetweb.mk
@@ -0,0 +1,45 @@
+################################################################################
+#
+# civetweb
+#
+################################################################################
+
+CIVETWEB_VERSION = 1.2
+CIVETWEB_SOURCE = civetweb-$(CIVETWEB_VERSION).tar.gz
+CIVETWEB_SITE =
http://github.com/sunsetbrew/civetweb/tarball/v$(CIVETWEB_VERSION)
+CIVETWEB_LICENSE = MIT
+CIVETWEB_LICENSE_FILES = LICENSE.md
+
+CIVETWEB_COPT = $(TARGET_CFLAGS)
+CIVETWEB_MOPT = TARGET_OS=LINUX
+CIVETWEB_LDFLAGS = $(TARGET_LDFLAGS)
+
+ifeq (BR2_PACKAGE_UTIL_LINUX_FALLOCATE,y)
+       CIVETWEB_COPT += -DHAVE_POSIX_FALLOCATE=0
+endif
+
+ifeq (BR2_CIVETWEB_WITH_IPV6,y)
+       CIVETWEB_MOPT += WITH_IPV6=1
+endif
+
+ifeq (BR2_CIVETWEB_WITH_LUA,y)
+       CIVETWEB_MOPT += WITH_LUA=1
+endif
+
+ifeq (BR2_CIVETWEB_WITH_SSL,y)
+       CIVETWEB_COPT += -DNO_SSL_DL -lcrypt -lssl
+       CIVETWEB_DEPENDENCIES += openssl
+else
+       CIVETWEB_COPT += -DNO_SSL
+endif
+
+define CIVETWEB_BUILD_CMDS
+       $(MAKE) CC="$(TARGET_CC)" -C $(@D) all $(CIVETWEB_MOPT)
COPT="$(CIVETWEB_COPT)"
+endef
+
+define CIVETWEB_INSTALL_TARGET_CMDS
+       $(MAKE) CC="$(TARGET_CC)" -C $(@D) install
DOCUMENT_ROOT=/usr/local/share/doc/civetweb
PREFIX="$(TARGET_DIR)/usr/local" $(CIVETWEB_MOPT)
COPT='$(CIVETWEB_COPT)'
+endef
+
+$(eval $(generic-package))
+

--
1.7.8.3

[Buildroot] [1/1] civetweb: new package

[Thomas Petazzoni]

Dear Thomas Davis,

Thanks, this patch looks much better. Some comments below.

On Tue, 27 Aug 2013 13:57:01 -0400, Thomas Davis wrote:
> Submission attempt #2 with recommended changes.
> 
> 1. Change make file ifdef's to ifeq.
> 2. Copyright lines removed to prevent confusion.
> 3. Put end of line in make file.
> 4. Submitted with git mail if it works, otherwise in plain text
> 5. Rebased, rebuilt and retested with buildroot.
> 
> The explaination of BR2_TOOLCHAIN_BUILDROOT_UCLIBC check in
> Config.in was answered in a previous post.  If that will
> always be included, I can remove the check.
> 
> BTW:  git --send-mail is not working for me.  But I made sure this
> mail is in plain-text mode.

See
http://buildroot.org/downloads/manual/manual.html#_patch_revision_changelog
for details on how the commit log should be formatted. Essentially,
what you're missing is that the details of what changed between the
previous version should go below the "---" sign (or be part of a
separate cover letter). Everything that is before the "---" will be
part of the commit log for the eternity, and we don't want that. See
the documentation for details.

Also, you should use --subject-prefix="PATCHv2" when generating your
patches, so that the title of your patch makes it clear which version
we're looking at.

> diff --git a/package/civetweb/Config.in b/package/civetweb/Config.in
> new file mode 100644
> index 0000000..5285b09
> --- /dev/null
> +++ b/package/civetweb/Config.in
> @@ -0,0 +1,34 @@
> +config BR2_PACKAGE_CIVETWEB
> +       bool "civetweb"
> +       depends on BR2_TOOLCHAIN_BUILDROOT_UCLIBC && !BR2_PTHREADS_NONE

As Thomas said, this is not correct, as we don't want to prevent this
package from being built with an external toolchain, or a glibc
toolchain.

If what you want to express is that your package needs thread support,
then you should add a:

	depends on BR2_TOOLCHAIN_HAS_THREADS

and that's it.

> +       help
> +         Full featured embedded web server with LUA support.
> +
> +         https://sourceforge.net/projects/civetweb
> +
> +if BR2_PACKAGE_CIVETWEB
> +
> +       config BR2_CIVETWEB_WITH_IPV6
> +               bool "enable IPV6 support"
> +               depends on BR2_INET_IPV6

We generally do not indent sub-configuration options.

I think this IPv6 option is not needed, the package should
automatically enable IPv6 support when BR2_INET_IPV6 is y.

> +       config BR2_CIVETWEB_WITH_LUA
> +               bool "enable LUA support"
> +               depends on BR2_LARGEFILE # util-linux

Why are you adding this largefile dependency here? From where does the
reference to util-linux comes?

If this option needs the "lua" package to be built, then you need to
"select BR2_PACKAGE_LUA" in the Config.in file, and add "lua" to
CIVETWEB_DEPENDENCIES in your .mk file.

> +       config BR2_CIVETWEB_WITH_SSL
> +               bool "enable SSL support"
> +               depends on BR2_PACKAGE_OPENSSL

We generally enable SSL support automatically when BR2_PACKAGE_OPENSSL
is enabled, so this option would not be necessary.

> +
> +       comment "civetweb IPV6 support requires IPV6"
> +               depends on !BR2_INET_IPV6

Not needed, as per the above.

> +       comment "civetweb LUA support requires large file support"
> +               depends on !BR2_LARGEFILE

Please answer the question above to decide what to do about this.

> +       comment "civetweb SSL support requires OpenSSL"
> +               depends on !BR2_PACKAGE_OPENSSL

Not needed, as per the above.

> +comment "civetweb requires a toolchain with PTHREAD support"
> +       depends on !BR2_TOOLCHAIN_BUILDROOT_UCLIBC || BR2_PTHREADS_NONE

This should be:

comment "civetweb requires a toolchain with threads support"
	depends on !BR2_TOOLCHAIN_HAS_THREADS

> diff --git a/package/civetweb/civetweb.mk b/package/civetweb/civetweb.mk
> new file mode 100644
> index 0000000..db14b2b
> --- /dev/null
> +++ b/package/civetweb/civetweb.mk
> @@ -0,0 +1,45 @@
> +################################################################################
> +#
> +# civetweb
> +#
> +################################################################################
> +
> +CIVETWEB_VERSION = 1.2
> +CIVETWEB_SOURCE = civetweb-$(CIVETWEB_VERSION).tar.gz

This line is not needed since this is the default.

> +CIVETWEB_SITE =
> http://github.com/sunsetbrew/civetweb/tarball/v$(CIVETWEB_VERSION)

You're not using git send-email: your patch has been line-wrapped by
your e-mail client, so it still cannot be applied as is.

> +CIVETWEB_LICENSE = MIT
> +CIVETWEB_LICENSE_FILES = LICENSE.md
> +
> +CIVETWEB_COPT = $(TARGET_CFLAGS)
> +CIVETWEB_MOPT = TARGET_OS=LINUX

We generally use something like CIVETWEB_CONF_OPT for the configuration
options.

> +CIVETWEB_LDFLAGS = $(TARGET_LDFLAGS)
> +
> +ifeq (BR2_PACKAGE_UTIL_LINUX_FALLOCATE,y)

Did you really test your package? This test is testing if the string
"BR2_PACKAGE_UTIL_LINUX_FALLOCATE" is equal to the string "y", so it
will always be false. Your tests should look like:

ifeq ($(BR2_....),y)

endif

see the thousands of examples available throughout the Buildroot source
code.

> +       CIVETWEB_COPT += -DHAVE_POSIX_FALLOCATE=0
> +endif

However, this test is generally wrong. You're confusing the
availability of the 'fallocate' command line tool from the util-linux
package with the availability of the fallocate() system call.

I think you can pass -DHAVE_POSIX_FALLOCATE=1 in all cases: it's always
available in glibc/eglibc toolchain, and our default configuration for
uClibc enables fallocate() as well.

> +
> +ifeq (BR2_CIVETWEB_WITH_IPV6,y)
> +       CIVETWEB_MOPT += WITH_IPV6=1
> +endif

This should be:

ifeq ($(BR2_INET_IPV6),y)
CIVETWEB_CONF_OPT += WITH_IPV6=1
endif

> +
> +ifeq (BR2_CIVETWEB_WITH_LUA,y)
> +       CIVETWEB_MOPT += WITH_LUA=1
> +endif

Ditto. And it doesn't need the 'lua' package as a dependency?

> +ifeq (BR2_CIVETWEB_WITH_SSL,y)
> +       CIVETWEB_COPT += -DNO_SSL_DL -lcrypt -lssl
> +       CIVETWEB_DEPENDENCIES += openssl
> +else
> +       CIVETWEB_COPT += -DNO_SSL
> +endif

Make this use BR2_PACKAGE_OPENSSL instead, as explained above.

> +
> +define CIVETWEB_BUILD_CMDS
> +       $(MAKE) CC="$(TARGET_CC)" -C $(@D) all $(CIVETWEB_MOPT)
> COPT="$(CIVETWEB_COPT)"
> +endef

This is badly line-wrapped.

> +
> +define CIVETWEB_INSTALL_TARGET_CMDS
> +       $(MAKE) CC="$(TARGET_CC)" -C $(@D) install
> DOCUMENT_ROOT=/usr/local/share/doc/civetweb
> PREFIX="$(TARGET_DIR)/usr/local" $(CIVETWEB_MOPT)
> COPT='$(CIVETWEB_COPT)'
> +endef

This is also badly line-wrapped. You should use $(TARGET_DIR)/usr as
the prefix, ditto for the documentation (and shouldn't DOCUMENT_ROOT
refer to $(TARGET_DIR) as well ?). Also, this line should be split:

define CIVETWEB_INSTALL_TARGET_CMDS
	$(MAKE) CC="$(TARGET_CC)" -C $(@D) install \
		DOCUMENT_ROOT=$(TARGET_DIR)/usr/share/doc/civetweb \
		PREFIX=$(TARGET_DIR)/usr \
		$(CIVETWEB_CONF_OPT) \
		COPT="$(CIVETWEB_COPT)"
endef

> +
> +$(eval $(generic-package))
> +

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas Davis]

I will make the suggested changes and retest and re-submit.  To answer
outstanding questions.

1. BR2_LARGEFILE is required by SQLITE3 which is a dependency in the
LUA support.

2. DOCUMENT_ROOT is the path from with-in the chroot to the documents
folder.  This is a value written into a configuration file, not an
actual directory something gets placed in.  Normally it would have
been automatically determined by PREFIX, but in the fake root
situation it has to overridden so it works correctly inside the
chroot.  It is not an install path itself.

Thank you very much for the help!

Thomas


On Tue, Aug 27, 2013 at 2:39 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas Davis,
>
> Thanks, this patch looks much better. Some comments below.
>
> On Tue, 27 Aug 2013 13:57:01 -0400, Thomas Davis wrote:
>> Submission attempt #2 with recommended changes.
>>
>> 1. Change make file ifdef's to ifeq.
>> 2. Copyright lines removed to prevent confusion.
>> 3. Put end of line in make file.
>> 4. Submitted with git mail if it works, otherwise in plain text
>> 5. Rebased, rebuilt and retested with buildroot.
>>
>> The explaination of BR2_TOOLCHAIN_BUILDROOT_UCLIBC check in
>> Config.in was answered in a previous post.  If that will
>> always be included, I can remove the check.
>>
>> BTW:  git --send-mail is not working for me.  But I made sure this
>> mail is in plain-text mode.
>
> See
> http://buildroot.org/downloads/manual/manual.html#_patch_revision_changelog
> for details on how the commit log should be formatted. Essentially,
> what you're missing is that the details of what changed between the
> previous version should go below the "---" sign (or be part of a
> separate cover letter). Everything that is before the "---" will be
> part of the commit log for the eternity, and we don't want that. See
> the documentation for details.
>
> Also, you should use --subject-prefix="PATCHv2" when generating your
> patches, so that the title of your patch makes it clear which version
> we're looking at.
>
>> diff --git a/package/civetweb/Config.in b/package/civetweb/Config.in
>> new file mode 100644
>> index 0000000..5285b09
>> --- /dev/null
>> +++ b/package/civetweb/Config.in
>> @@ -0,0 +1,34 @@
>> +config BR2_PACKAGE_CIVETWEB
>> +       bool "civetweb"
>> +       depends on BR2_TOOLCHAIN_BUILDROOT_UCLIBC && !BR2_PTHREADS_NONE
>
> As Thomas said, this is not correct, as we don't want to prevent this
> package from being built with an external toolchain, or a glibc
> toolchain.
>
> If what you want to express is that your package needs thread support,
> then you should add a:
>
>         depends on BR2_TOOLCHAIN_HAS_THREADS
>
> and that's it.
>
>> +       help
>> +         Full featured embedded web server with LUA support.
>> +
>> +         https://sourceforge.net/projects/civetweb
>> +
>> +if BR2_PACKAGE_CIVETWEB
>> +
>> +       config BR2_CIVETWEB_WITH_IPV6
>> +               bool "enable IPV6 support"
>> +               depends on BR2_INET_IPV6
>
> We generally do not indent sub-configuration options.
>
> I think this IPv6 option is not needed, the package should
> automatically enable IPv6 support when BR2_INET_IPV6 is y.
>
>> +       config BR2_CIVETWEB_WITH_LUA
>> +               bool "enable LUA support"
>> +               depends on BR2_LARGEFILE # util-linux
>
> Why are you adding this largefile dependency here? From where does the
> reference to util-linux comes?
>
> If this option needs the "lua" package to be built, then you need to
> "select BR2_PACKAGE_LUA" in the Config.in file, and add "lua" to
> CIVETWEB_DEPENDENCIES in your .mk file.
>
>> +       config BR2_CIVETWEB_WITH_SSL
>> +               bool "enable SSL support"
>> +               depends on BR2_PACKAGE_OPENSSL
>
> We generally enable SSL support automatically when BR2_PACKAGE_OPENSSL
> is enabled, so this option would not be necessary.
>
>> +
>> +       comment "civetweb IPV6 support requires IPV6"
>> +               depends on !BR2_INET_IPV6
>
> Not needed, as per the above.
>
>> +       comment "civetweb LUA support requires large file support"
>> +               depends on !BR2_LARGEFILE
>
> Please answer the question above to decide what to do about this.
>
>> +       comment "civetweb SSL support requires OpenSSL"
>> +               depends on !BR2_PACKAGE_OPENSSL
>
> Not needed, as per the above.
>
>> +comment "civetweb requires a toolchain with PTHREAD support"
>> +       depends on !BR2_TOOLCHAIN_BUILDROOT_UCLIBC || BR2_PTHREADS_NONE
>
> This should be:
>
> comment "civetweb requires a toolchain with threads support"
>         depends on !BR2_TOOLCHAIN_HAS_THREADS
>
>> diff --git a/package/civetweb/civetweb.mk b/package/civetweb/civetweb.mk
>> new file mode 100644
>> index 0000000..db14b2b
>> --- /dev/null
>> +++ b/package/civetweb/civetweb.mk
>> @@ -0,0 +1,45 @@
>> +################################################################################
>> +#
>> +# civetweb
>> +#
>> +################################################################################
>> +
>> +CIVETWEB_VERSION = 1.2
>> +CIVETWEB_SOURCE = civetweb-$(CIVETWEB_VERSION).tar.gz
>
> This line is not needed since this is the default.
>
>> +CIVETWEB_SITE =
>> http://github.com/sunsetbrew/civetweb/tarball/v$(CIVETWEB_VERSION)
>
> You're not using git send-email: your patch has been line-wrapped by
> your e-mail client, so it still cannot be applied as is.
>
>> +CIVETWEB_LICENSE = MIT
>> +CIVETWEB_LICENSE_FILES = LICENSE.md
>> +
>> +CIVETWEB_COPT = $(TARGET_CFLAGS)
>> +CIVETWEB_MOPT = TARGET_OS=LINUX
>
> We generally use something like CIVETWEB_CONF_OPT for the configuration
> options.
>
>> +CIVETWEB_LDFLAGS = $(TARGET_LDFLAGS)
>> +
>> +ifeq (BR2_PACKAGE_UTIL_LINUX_FALLOCATE,y)
>
> Did you really test your package? This test is testing if the string
> "BR2_PACKAGE_UTIL_LINUX_FALLOCATE" is equal to the string "y", so it
> will always be false. Your tests should look like:
>
> ifeq ($(BR2_....),y)
>
> endif
>
> see the thousands of examples available throughout the Buildroot source
> code.
>
>> +       CIVETWEB_COPT += -DHAVE_POSIX_FALLOCATE=0
>> +endif
>
> However, this test is generally wrong. You're confusing the
> availability of the 'fallocate' command line tool from the util-linux
> package with the availability of the fallocate() system call.
>
> I think you can pass -DHAVE_POSIX_FALLOCATE=1 in all cases: it's always
> available in glibc/eglibc toolchain, and our default configuration for
> uClibc enables fallocate() as well.
>
>> +
>> +ifeq (BR2_CIVETWEB_WITH_IPV6,y)
>> +       CIVETWEB_MOPT += WITH_IPV6=1
>> +endif
>
> This should be:
>
> ifeq ($(BR2_INET_IPV6),y)
> CIVETWEB_CONF_OPT += WITH_IPV6=1
> endif
>
>> +
>> +ifeq (BR2_CIVETWEB_WITH_LUA,y)
>> +       CIVETWEB_MOPT += WITH_LUA=1
>> +endif
>
> Ditto. And it doesn't need the 'lua' package as a dependency?
>
>> +ifeq (BR2_CIVETWEB_WITH_SSL,y)
>> +       CIVETWEB_COPT += -DNO_SSL_DL -lcrypt -lssl
>> +       CIVETWEB_DEPENDENCIES += openssl
>> +else
>> +       CIVETWEB_COPT += -DNO_SSL
>> +endif
>
> Make this use BR2_PACKAGE_OPENSSL instead, as explained above.
>
>> +
>> +define CIVETWEB_BUILD_CMDS
>> +       $(MAKE) CC="$(TARGET_CC)" -C $(@D) all $(CIVETWEB_MOPT)
>> COPT="$(CIVETWEB_COPT)"
>> +endef
>
> This is badly line-wrapped.
>
>> +
>> +define CIVETWEB_INSTALL_TARGET_CMDS
>> +       $(MAKE) CC="$(TARGET_CC)" -C $(@D) install
>> DOCUMENT_ROOT=/usr/local/share/doc/civetweb
>> PREFIX="$(TARGET_DIR)/usr/local" $(CIVETWEB_MOPT)
>> COPT='$(CIVETWEB_COPT)'
>> +endef
>
> This is also badly line-wrapped. You should use $(TARGET_DIR)/usr as
> the prefix, ditto for the documentation (and shouldn't DOCUMENT_ROOT
> refer to $(TARGET_DIR) as well ?). Also, this line should be split:
>
> define CIVETWEB_INSTALL_TARGET_CMDS
>         $(MAKE) CC="$(TARGET_CC)" -C $(@D) install \
>                 DOCUMENT_ROOT=$(TARGET_DIR)/usr/share/doc/civetweb \
>                 PREFIX=$(TARGET_DIR)/usr \
>                 $(CIVETWEB_CONF_OPT) \
>                 COPT="$(CIVETWEB_COPT)"
> endef
>
>> +
>> +$(eval $(generic-package))
>> +
>
> Thanks,
>
> Thomas
> --
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas Petazzoni]

Dear Thomas Davis,

On Tue, 27 Aug 2013 14:54:28 -0400, Thomas Davis wrote:
> I will make the suggested changes and retest and re-submit.  To answer
> outstanding questions.
> 
> 1. BR2_LARGEFILE is required by SQLITE3 which is a dependency in the
> LUA support.

Ok. This is something you could potentially address by passing
-DSQLITE_DISABLE_LFS to the Sqlite build when !BR2_LARGEFILE. See
package/sqlite/sqlite.mk in Buildroot. But ok, it's not mandatory to
support this use case for civetweb.

So, what I'd like to see is something like:

config BR2_PACKAGE_CIVETWEB_LUA_SUPPORT
	bool "lua support"
	# required by the bundled sqlite code
	depends on BR2_LARGEFILE
	comment
	  Enable Lua support in Civetweb. Note that this will use a
	  version of Lua and Sqlite bundled within the Civetweb
	  sources, and not the packages from Buildroot.

comment "lua support requires largefile support in toolchain"
	depends on !BR2_LARGEFILE

> 2. DOCUMENT_ROOT is the path from with-in the chroot to the documents
> folder.  This is a value written into a configuration file, not an
> actual directory something gets placed in.  Normally it would have
> been automatically determined by PREFIX, but in the fake root
> situation it has to overridden so it works correctly inside the
> chroot.  It is not an install path itself.

Right, discovered this after having a deeper look at civetweb. Then
please set it to /var/www, which is what we do for other web servers in
Buildroot.

Thanks!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas Davis]

Thanks once again for the reply and the detailed examination.  I will
see about having a non large file option in the civetweb build and
test that separately before continuing.    I will also add the comment
about LUA and sqlite3 not coming from buildroot and move the document
directory as well.

Thanks,
Thomas Davis

On Tue, Aug 27, 2013 at 3:15 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Thomas Davis,
>
> On Tue, 27 Aug 2013 14:54:28 -0400, Thomas Davis wrote:
>> I will make the suggested changes and retest and re-submit.  To answer
>> outstanding questions.
>>
>> 1. BR2_LARGEFILE is required by SQLITE3 which is a dependency in the
>> LUA support.
>
> Ok. This is something you could potentially address by passing
> -DSQLITE_DISABLE_LFS to the Sqlite build when !BR2_LARGEFILE. See
> package/sqlite/sqlite.mk in Buildroot. But ok, it's not mandatory to
> support this use case for civetweb.
>
> So, what I'd like to see is something like:
>
> config BR2_PACKAGE_CIVETWEB_LUA_SUPPORT
>         bool "lua support"
>         # required by the bundled sqlite code
>         depends on BR2_LARGEFILE
>         comment
>           Enable Lua support in Civetweb. Note that this will use a
>           version of Lua and Sqlite bundled within the Civetweb
>           sources, and not the packages from Buildroot.
>
> comment "lua support requires largefile support in toolchain"
>         depends on !BR2_LARGEFILE
>
>> 2. DOCUMENT_ROOT is the path from with-in the chroot to the documents
>> folder.  This is a value written into a configuration file, not an
>> actual directory something gets placed in.  Normally it would have
>> been automatically determined by PREFIX, but in the fake root
>> situation it has to overridden so it works correctly inside the
>> chroot.  It is not an install path itself.
>
> Right, discovered this after having a deeper look at civetweb. Then
> please set it to /var/www, which is what we do for other web servers in
> Buildroot.
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas De Schampheleire]

On Tue, Aug 27, 2013 at 4:29 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Thomas, Thomas,
>

Three cheers for the concept of surnames!

Thomas

[Buildroot] [1/1] civetweb: new package

[Thomas Petazzoni]

Dear Thomas Davis,

On Tue, 27 Aug 2013 15:28:49 -0400, Thomas Davis wrote:
> Thanks once again for the reply and the detailed examination.  I will
> see about having a non large file option in the civetweb build and
> test that separately before continuing.    I will also add the comment
> about LUA and sqlite3 not coming from buildroot and move the document
> directory as well.

As I said, support non-large file builds is really not mandatory. You
can submit your package without supporting non-large file builds for
the moment, and if you're interested, you can send later a followup
patch to make this possible. But we have plenty of packages that don't
support non-large file builds in Buildroot, so it's not really a
problem to have another one.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[Buildroot] [1/1] civetweb: new package

[Thomas Petazzoni]

Thomas, Thomas,

On Tue, 27 Aug 2013 22:12:34 +0200, Thomas De Schampheleire wrote:
> On Tue, Aug 27, 2013 at 4:29 PM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Thomas, Thomas,
> >
> 
> Three cheers for the concept of surnames!
> 
> Thomas

Yeah, sounded like a fun start of e-mail for something signed:

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com