Message ID | 1357948846-3710-1-git-send-email-gilles.talis@gmail.com |
---|---|
State | Superseded |
Headers | show |
Dear gilles.talis@gmail.com, Thanks for your contribution! Your package mostly looks good, but I (of course!) have a few comments. See below. On Fri, 11 Jan 2013 16:00:46 -0800, gilles.talis@gmail.com wrote: > diff --git a/package/httping/Config.in b/package/httping/Config.in > new file mode 100644 > index 0000000..578028f > --- /dev/null > +++ b/package/httping/Config.in > @@ -0,0 +1,24 @@ > +config BR2_PACKAGE_HTTPING > + bool "httping" > + help > + Httping is like 'ping' but for http-requests. > + Give it an url, and it'll show you how long it takes to connect, > + send a request and retrieve the reply (only the headers). > + Be aware that the transmission across the network also takes time! > + So it measures the latency of the webserver + network. Indentation of this line is not correct: it is composed of spaces only, while it should be one tab + 2 spaces like the other lines. > +if BR2_PACKAGE_HTTPING > + > +config BR2_PACKAGE_HTTPING_OPENSSL > + bool "OpenSSL support" > + default y > + select BR2_PACKAGE_OPENSSL Here there is a point that is rather unclear in Buildroot: *) Should each package offer a configuration sub-option to enable features that depend on other packages (like you did on OpenSSL) *) Or should a package automatically enable features if it finds that the dependencies are available? This is generally what we do for packages having an optional dependency on OpenSSL. This is to be discussed with other Buildroot developers. Probably we need to clarify what the rule is, and then document it. > +config BR2_PACKAGE_HTTPING_TFO > + bool "TCP Fast Open (TFO) support" > + default n 'default n' is the default, so it's not needed. > --- /dev/null > +++ b/package/httping/httping.mk > @@ -0,0 +1,44 @@ > +############################################################# > +# > +# httping > +# > +############################################################# > +HTTPING_VERSION = 1.5.6 > +HTTPING_SOURCE = httping-$(HTTPING_VERSION).tgz > +HTTPING_SITE = http://www.vanheusden.com/httping > +HTTPING_LICENSE = GPL You should be more specific about the version, and whether the "or later" specifier is here or not. Good choices are: GPLv2, GPLv2+, GPLv3, GPLv3+. > +HTTPING_LICENSE_FILES = license.txt > + > +ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > +HTTPING_DEPENDENCIES = openssl > +endif > + > + > +ifneq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > + HTTPING_SSL = no > +endif This should be slightly changed to something like: ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) HTTPING_DEPENDENCIES += openssl HTTPING_SSL = yes else HTTPING_SSL = no endif (assuming SSL=yes is correct to enable SSL support) > +ifeq ($(BR2_PACKAGE_HTTPING_TFO),y) > + HTTPING_TFO = yes > +endif > + > +define HTTPING_BUILD_CMDS > + $(MAKE) CC="$(TARGET_CC)" \ > + SSL=$(HTTPING_SSL) \ > + DEBUG=no \ > + TFO=$(HTTPING_TFO) \ > + LD="$(TARGET_LD)" \ > + STRIP="$(TARGET_STRIP)" -C $(@D) > +endef Here, I would recommend using $(TARGET_CONFIGURE_OPTS) : $(MAKE) $(TARGET_CONFIGURE_OPTS) \ SSL=$(HTTPING_SSL) \ DEBUG=no \ TFO=$(HTTPING_TFO) \ -C $(@D) > +define HTTPING_INSTALL_TARGET_CMDS > + $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin > +endef With -D, you should give a complete path to the target binary: $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin/httping > +define HTTPING_CLEAN_CMDS > + $(MAKE) -C $(@D) clean > +endef > + > +$(eval $(generic-package)) > + > +# http://www.vanheusden.com/httping/httping-1.5.6.tgz This comment is not needed. Best regards, Thomas
Dear Thomas, Thanks a lot for your comments, I am going to fix these issues right away! It is amazing how much you can learn from code reviews :-). Best regards, Gilles On Jan 12, 2013 3:07 AM, "Thomas Petazzoni" < thomas.petazzoni@free-electrons.com> wrote: > Dear gilles.talis@gmail.com, > > Thanks for your contribution! Your package mostly looks good, but I (of > course!) have a few comments. See below. > > On Fri, 11 Jan 2013 16:00:46 -0800, gilles.talis@gmail.com wrote: > > diff --git a/package/httping/Config.in b/package/httping/Config.in > > new file mode 100644 > > index 0000000..578028f > > --- /dev/null > > +++ b/package/httping/Config.in > > @@ -0,0 +1,24 @@ > > +config BR2_PACKAGE_HTTPING > > + bool "httping" > > + help > > + Httping is like 'ping' but for http-requests. > > + Give it an url, and it'll show you how long it takes to connect, > > + send a request and retrieve the reply (only the headers). > > + Be aware that the transmission across the network also takes > time! > > + So it measures the latency of the webserver + network. > > Indentation of this line is not correct: it is composed of spaces only, > while it should be one tab + 2 spaces like the other lines. > > > +if BR2_PACKAGE_HTTPING > > + > > +config BR2_PACKAGE_HTTPING_OPENSSL > > + bool "OpenSSL support" > > + default y > > + select BR2_PACKAGE_OPENSSL > > Here there is a point that is rather unclear in Buildroot: > > *) Should each package offer a configuration sub-option to enable > features that depend on other packages (like you did on OpenSSL) > > *) Or should a package automatically enable features if it finds that > the dependencies are available? This is generally what we do for > packages having an optional dependency on OpenSSL. > > This is to be discussed with other Buildroot developers. Probably we > need to clarify what the rule is, and then document it. > > > +config BR2_PACKAGE_HTTPING_TFO > > + bool "TCP Fast Open (TFO) support" > > + default n > > 'default n' is the default, so it's not needed. > > > --- /dev/null > > +++ b/package/httping/httping.mk > > @@ -0,0 +1,44 @@ > > +############################################################# > > +# > > +# httping > > +# > > +############################################################# > > +HTTPING_VERSION = 1.5.6 > > +HTTPING_SOURCE = httping-$(HTTPING_VERSION).tgz > > +HTTPING_SITE = http://www.vanheusden.com/httping > > +HTTPING_LICENSE = GPL > > You should be more specific about the version, and whether the "or > later" specifier is here or not. Good choices are: GPLv2, GPLv2+, > GPLv3, GPLv3+. > > > +HTTPING_LICENSE_FILES = license.txt > > + > > +ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > > +HTTPING_DEPENDENCIES = openssl > > +endif > > + > > + > > +ifneq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > > + HTTPING_SSL = no > > +endif > > This should be slightly changed to something like: > > ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > HTTPING_DEPENDENCIES += openssl > HTTPING_SSL = yes > else > HTTPING_SSL = no > endif > > (assuming SSL=yes is correct to enable SSL support) > > > +ifeq ($(BR2_PACKAGE_HTTPING_TFO),y) > > + HTTPING_TFO = yes > > +endif > > + > > +define HTTPING_BUILD_CMDS > > + $(MAKE) CC="$(TARGET_CC)" \ > > + SSL=$(HTTPING_SSL) \ > > + DEBUG=no \ > > + TFO=$(HTTPING_TFO) \ > > + LD="$(TARGET_LD)" \ > > + STRIP="$(TARGET_STRIP)" -C $(@D) > > +endef > > Here, I would recommend using $(TARGET_CONFIGURE_OPTS) : > > $(MAKE) $(TARGET_CONFIGURE_OPTS) \ > SSL=$(HTTPING_SSL) \ > DEBUG=no \ > TFO=$(HTTPING_TFO) \ > -C $(@D) > > > +define HTTPING_INSTALL_TARGET_CMDS > > + $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin > > +endef > > With -D, you should give a complete path to the target binary: > > $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin/httping > > > +define HTTPING_CLEAN_CMDS > > + $(MAKE) -C $(@D) clean > > +endef > > + > > +$(eval $(generic-package)) > > + > > +# http://www.vanheusden.com/httping/httping-1.5.6.tgz > > This comment is not needed. > > Best regards, > > Thomas > -- > Thomas Petazzoni, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com >
Dear Thomas, Thanks again for your comments. There's one recommendation I could not follow though. Using $(TARGET_CONFIGURE_OPTS) instead of "CC=..." and "LD..." seems to dismiss the "SSL=" and "TFO=" options. I will have to spend more time to figure out why this happens. I have kept the build command that way for the time being. best regards Gilles. On Jan 12, 2013 3:07 AM, "Thomas Petazzoni" < thomas.petazzoni@free-electrons.com> wrote: > Dear gilles.talis@gmail.com, > > Thanks for your contribution! Your package mostly looks good, but I (of > course!) have a few comments. See below. > > On Fri, 11 Jan 2013 16:00:46 -0800, gilles.talis@gmail.com wrote: > > diff --git a/package/httping/Config.in b/package/httping/Config.in > > new file mode 100644 > > index 0000000..578028f > > --- /dev/null > > +++ b/package/httping/Config.in > > @@ -0,0 +1,24 @@ > > +config BR2_PACKAGE_HTTPING > > + bool "httping" > > + help > > + Httping is like 'ping' but for http-requests. > > + Give it an url, and it'll show you how long it takes to connect, > > + send a request and retrieve the reply (only the headers). > > + Be aware that the transmission across the network also takes > time! > > + So it measures the latency of the webserver + network. > > Indentation of this line is not correct: it is composed of spaces only, > while it should be one tab + 2 spaces like the other lines. > > > +if BR2_PACKAGE_HTTPING > > + > > +config BR2_PACKAGE_HTTPING_OPENSSL > > + bool "OpenSSL support" > > + default y > > + select BR2_PACKAGE_OPENSSL > > Here there is a point that is rather unclear in Buildroot: > > *) Should each package offer a configuration sub-option to enable > features that depend on other packages (like you did on OpenSSL) > > *) Or should a package automatically enable features if it finds that > the dependencies are available? This is generally what we do for > packages having an optional dependency on OpenSSL. > > This is to be discussed with other Buildroot developers. Probably we > need to clarify what the rule is, and then document it. > > > +config BR2_PACKAGE_HTTPING_TFO > > + bool "TCP Fast Open (TFO) support" > > + default n > > 'default n' is the default, so it's not needed. > > > --- /dev/null > > +++ b/package/httping/httping.mk > > @@ -0,0 +1,44 @@ > > +############################################################# > > +# > > +# httping > > +# > > +############################################################# > > +HTTPING_VERSION = 1.5.6 > > +HTTPING_SOURCE = httping-$(HTTPING_VERSION).tgz > > +HTTPING_SITE = http://www.vanheusden.com/httping > > +HTTPING_LICENSE = GPL > > You should be more specific about the version, and whether the "or > later" specifier is here or not. Good choices are: GPLv2, GPLv2+, > GPLv3, GPLv3+. > > > +HTTPING_LICENSE_FILES = license.txt > > + > > +ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > > +HTTPING_DEPENDENCIES = openssl > > +endif > > + > > + > > +ifneq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > > + HTTPING_SSL = no > > +endif > > This should be slightly changed to something like: > > ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) > HTTPING_DEPENDENCIES += openssl > HTTPING_SSL = yes > else > HTTPING_SSL = no > endif > > (assuming SSL=yes is correct to enable SSL support) > > > +ifeq ($(BR2_PACKAGE_HTTPING_TFO),y) > > + HTTPING_TFO = yes > > +endif > > + > > +define HTTPING_BUILD_CMDS > > + $(MAKE) CC="$(TARGET_CC)" \ > > + SSL=$(HTTPING_SSL) \ > > + DEBUG=no \ > > + TFO=$(HTTPING_TFO) \ > > + LD="$(TARGET_LD)" \ > > + STRIP="$(TARGET_STRIP)" -C $(@D) > > +endef > > Here, I would recommend using $(TARGET_CONFIGURE_OPTS) : > > $(MAKE) $(TARGET_CONFIGURE_OPTS) \ > SSL=$(HTTPING_SSL) \ > DEBUG=no \ > TFO=$(HTTPING_TFO) \ > -C $(@D) > > > +define HTTPING_INSTALL_TARGET_CMDS > > + $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin > > +endef > > With -D, you should give a complete path to the target binary: > > $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin/httping > > > +define HTTPING_CLEAN_CMDS > > + $(MAKE) -C $(@D) clean > > +endef > > + > > +$(eval $(generic-package)) > > + > > +# http://www.vanheusden.com/httping/httping-1.5.6.tgz > > This comment is not needed. > > Best regards, > > Thomas > -- > Thomas Petazzoni, Free Electrons > Kernel, drivers, real-time and embedded Linux > development, consulting, training and support. > http://free-electrons.com >
On 12/01/13 12:07, Thomas Petazzoni wrote: >> > +config BR2_PACKAGE_HTTPING_OPENSSL >> > + bool "OpenSSL support" >> > + default y >> > + select BR2_PACKAGE_OPENSSL > Here there is a point that is rather unclear in Buildroot: > > *) Should each package offer a configuration sub-option to enable > features that depend on other packages (like you did on OpenSSL) > > *) Or should a package automatically enable features if it finds that > the dependencies are available? This is generally what we do for > packages having an optional dependency on OpenSSL. > > This is to be discussed with other Buildroot developers. Probably we > need to clarify what the rule is, and then document it. +1 on the "needs to be documented". But for me the rule is fairly clear: if it doesn't significantly increase binary size, then it should be selected automatically if available. It would be good to add to the package's help text that "SSL support is available if the openssl package (in the Libraries -> Crypto menu) is selected." Regards, Arnout
diff --git a/package/Config.in b/package/Config.in index bd1db6b..2ce7de8 100644 --- a/package/Config.in +++ b/package/Config.in @@ -617,6 +617,7 @@ source "package/ethtool/Config.in" source "package/heirloom-mailx/Config.in" source "package/hiawatha/Config.in" source "package/hostapd/Config.in" +source "package/httping/Config.in" if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS source "package/ifplugd/Config.in" endif diff --git a/package/httping/Config.in b/package/httping/Config.in new file mode 100644 index 0000000..578028f --- /dev/null +++ b/package/httping/Config.in @@ -0,0 +1,24 @@ +config BR2_PACKAGE_HTTPING + bool "httping" + help + Httping is like 'ping' but for http-requests. + Give it an url, and it'll show you how long it takes to connect, + send a request and retrieve the reply (only the headers). + Be aware that the transmission across the network also takes time! + So it measures the latency of the webserver + network. + + http://www.vanheusden.com/httping/ + +if BR2_PACKAGE_HTTPING + +config BR2_PACKAGE_HTTPING_OPENSSL + bool "OpenSSL support" + default y + select BR2_PACKAGE_OPENSSL + +config BR2_PACKAGE_HTTPING_TFO + bool "TCP Fast Open (TFO) support" + default n + +endif + diff --git a/package/httping/httping.mk b/package/httping/httping.mk new file mode 100644 index 0000000..7076440 --- /dev/null +++ b/package/httping/httping.mk @@ -0,0 +1,44 @@ +############################################################# +# +# httping +# +############################################################# +HTTPING_VERSION = 1.5.6 +HTTPING_SOURCE = httping-$(HTTPING_VERSION).tgz +HTTPING_SITE = http://www.vanheusden.com/httping +HTTPING_LICENSE = GPL +HTTPING_LICENSE_FILES = license.txt + +ifeq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) +HTTPING_DEPENDENCIES = openssl +endif + + +ifneq ($(BR2_PACKAGE_HTTPING_OPENSSL),y) + HTTPING_SSL = no +endif + +ifeq ($(BR2_PACKAGE_HTTPING_TFO),y) + HTTPING_TFO = yes +endif + +define HTTPING_BUILD_CMDS + $(MAKE) CC="$(TARGET_CC)" \ + SSL=$(HTTPING_SSL) \ + DEBUG=no \ + TFO=$(HTTPING_TFO) \ + LD="$(TARGET_LD)" \ + STRIP="$(TARGET_STRIP)" -C $(@D) +endef + +define HTTPING_INSTALL_TARGET_CMDS + $(INSTALL) -D -m 0755 $(@D)/httping $(TARGET_DIR)/usr/bin +endef + +define HTTPING_CLEAN_CMDS + $(MAKE) -C $(@D) clean +endef + +$(eval $(generic-package)) + +# http://www.vanheusden.com/httping/httping-1.5.6.tgz