| Message ID | 20160904210846.1332-1-theo.debrouwere@skynet.be |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Theo, all, Thanks for this contribution. Hereafter few comments inlined. The subject should be: "libpugixml: new package" On Sun, Sep 4, 2016 at 11:08 PM, Theo Debrouwere <theo.debrouwere@skynet.be> wrote: > From: Theo Debrouwere <t.debrouwere@televic.com> > > pugixml is a light-weight C++ XML processing library. It features: > * DOM-like interface with rich traversal/modification capabilities > * Extremely fast non-validating XML parser which constructs the DOM tree from an XML file/buffer > * XPath 1.0 implementation for complex data-driven tree queries > * Full Unicode support with Unicode interface variants and automatic encoding conversions > > Homepage: http://pugixml.org/ > Repository: https://github.com/zeux/pugixml > > Signed-off-by: Theo Debrouwere <t.debrouwere@televic.com> > --- > package/Config.in | 1 + > package/libpugixml/Config.in | 15 +++++++++++++++ > package/libpugixml/libpugixml.hash | 2 ++ > package/libpugixml/libpugixml.mk | 19 +++++++++++++++++++ > 4 files changed, 37 insertions(+) > create mode 100644 package/libpugixml/Config.in > create mode 100644 package/libpugixml/libpugixml.hash > create mode 100644 package/libpugixml/libpugixml.mk > > diff --git a/package/Config.in b/package/Config.in > index 1e51a45..d0198ce 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -1092,6 +1092,7 @@ menu "JSON/XML" > source "package/libxslt/Config.in" > source "package/libyaml/Config.in" > source "package/mxml/Config.in" > + source "package/libpugixml/Config.in" Keep package list alphabetically sorted please. > source "package/rapidjson/Config.in" > source "package/rapidxml/Config.in" > source "package/raptor/Config.in" > diff --git a/package/libpugixml/Config.in b/package/libpugixml/Config.in > new file mode 100644 > index 0000000..17c6eec > --- /dev/null > +++ b/package/libpugixml/Config.in > @@ -0,0 +1,15 @@ > +config BR2_PACKAGE_LIBPUGIXML > + bool "libpugixml" Since it is a C++ project, it should depend on BR2_INSTALL_LIBSTDCPP, see [1]. > + help > + Light-weight, simple and fast XML parser for C++ with XPath support > + > + Features: > + * DOM-like interface with rich traversal/modification capabilities > + * Extremely fast non-validating XML parser which constructs the DOM tree > + from an XML file/buffer > + * XPath 1.0 implementation for complex data-driven tree queries > + * Full Unicode support with Unicode interface variants and automatic > + encoding conversions > + > + http://pugixml.org/ > + https://github.com/zeux/pugixml > diff --git a/package/libpugixml/libpugixml.hash b/package/libpugixml/libpugixml.hash > new file mode 100644 > index 0000000..9007f7c > --- /dev/null > +++ b/package/libpugixml/libpugixml.hash > @@ -0,0 +1,2 @@ > +# Locally computed: > +sha256 fbe10d46f61d769f7d92a296102e4e2bd3ee16130f11c5b10a1aae590ea1f5ca pugixml-1.7.tar.gz > diff --git a/package/libpugixml/libpugixml.mk b/package/libpugixml/libpugixml.mk > new file mode 100644 > index 0000000..3c9987e > --- /dev/null > +++ b/package/libpugixml/libpugixml.mk > @@ -0,0 +1,19 @@ > +################################################################################ > +# > +# libpugixml > +# > +################################################################################ > + > +LIBPUGIXML_VERSION = 1.7 > +LIBPUGIXML_SOURCE = pugixml-$(LIBPUGIXML_VERSION).tar.gz > +LIBPUGIXML_SITE = http://github.com/zeux/pugixml/releases/download/v$(LIBPUGIXML_VERSION) > +LIBPUGIXML_LICENSE = MIT > +LIBPUGIXML_LICENSE_FILES = docs/manual.html > +LIBPUGIXML_INSTALL_STAGING = YES > + > +LIBPUGIXML_SUBDIR = scripts > + > +# build libpugixml as a shared library > +LIBPUGIXML_CONF_OPTS = -DBUILD_SHARED_LIBS=ON Please do not do that, this will break static only build. This flag is automatically driven by the cmake infrastructure depending on kind of build (shared lib only, shared + static lib or static lib only), check the cmake infrastructure reference section in the manual [2]. Note that, in case of shared + static lib build, if the build-system supports building only one kind of libs, shared libs is prefered (see [3]). If you really want to forbid the static build of the library, then consider using "depends on !BR2_STATIC_LIBS", see the package section in the manual [1]. > + > +$(eval $(cmake-package)) > -- > 2.9.3 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot [1] http://nightly.buildroot.org/manual.html#dependencies-target-toolchain-options [2] http://nightly.buildroot.org/manual.html#cmake-package-reference [3] https://github.com/buildroot/buildroot/blob/master/package/pkg-cmake.mk#L100 Regards,
Hello, Thanks for your contribution! This review is in addition to what Samuel Martin already said on the same patch. On Sun, 4 Sep 2016 23:08:46 +0200, Theo Debrouwere wrote: > +LIBPUGIXML_VERSION = 1.7 > +LIBPUGIXML_SOURCE = pugixml-$(LIBPUGIXML_VERSION).tar.gz Please use pugixml for the package name, since it's the upstream name. Then you can drop this line as the value will be the default. > +LIBPUGIXML_SITE = http://github.com/zeux/pugixml/releases/download/v$(LIBPUGIXML_VERSION) > +LIBPUGIXML_LICENSE = MIT > +LIBPUGIXML_LICENSE_FILES = docs/manual.html Using an HTML file here is not really appropriate, as we concatenate all license files into a big text file, so it's not really going to look nice :/ The readme.txt is good enough as a license file. > +LIBPUGIXML_INSTALL_STAGING = YES > + > +LIBPUGIXML_SUBDIR = scripts Hum, why? > + > +# build libpugixml as a shared library > +LIBPUGIXML_CONF_OPTS = -DBUILD_SHARED_LIBS=ON > + > +$(eval $(cmake-package)) Thomas
Hi Theo, In addition to what the other two have commented... On 04-09-16 23:08, Theo Debrouwere wrote: > From: Theo Debrouwere <t.debrouwere@televic.com> > > pugixml is a light-weight C++ XML processing library. It features: Since upstream calls it pugixml (without lib- prefix), the buildroot package should be named the same. > * DOM-like interface with rich traversal/modification capabilities > * Extremely fast non-validating XML parser which constructs the DOM tree from an XML file/buffer > * XPath 1.0 implementation for complex data-driven tree queries > * Full Unicode support with Unicode interface variants and automatic encoding conversions > > Homepage: http://pugixml.org/ > Repository: https://github.com/zeux/pugixml > > Signed-off-by: Theo Debrouwere <t.debrouwere@televic.com> > --- > package/Config.in | 1 + > package/libpugixml/Config.in | 15 +++++++++++++++ > package/libpugixml/libpugixml.hash | 2 ++ > package/libpugixml/libpugixml.mk | 19 +++++++++++++++++++ > 4 files changed, 37 insertions(+) > create mode 100644 package/libpugixml/Config.in > create mode 100644 package/libpugixml/libpugixml.hash > create mode 100644 package/libpugixml/libpugixml.mk > > diff --git a/package/Config.in b/package/Config.in > index 1e51a45..d0198ce 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -1092,6 +1092,7 @@ menu "JSON/XML" > source "package/libxslt/Config.in" > source "package/libyaml/Config.in" > source "package/mxml/Config.in" > + source "package/libpugixml/Config.in" ... and then it's in the right place here :-) > source "package/rapidjson/Config.in" > source "package/rapidxml/Config.in" > source "package/raptor/Config.in" > diff --git a/package/libpugixml/Config.in b/package/libpugixml/Config.in > new file mode 100644 > index 0000000..17c6eec > --- /dev/null > +++ b/package/libpugixml/Config.in > @@ -0,0 +1,15 @@ > +config BR2_PACKAGE_LIBPUGIXML > + bool "libpugixml" > + help > + Light-weight, simple and fast XML parser for C++ with XPath support > + > + Features: > + * DOM-like interface with rich traversal/modification capabilities > + * Extremely fast non-validating XML parser which constructs the DOM tree > + from an XML file/buffer > + * XPath 1.0 implementation for complex data-driven tree queries > + * Full Unicode support with Unicode interface variants and automatic > + encoding conversions > + > + http://pugixml.org/ > + https://github.com/zeux/pugixml > diff --git a/package/libpugixml/libpugixml.hash b/package/libpugixml/libpugixml.hash > new file mode 100644 > index 0000000..9007f7c > --- /dev/null > +++ b/package/libpugixml/libpugixml.hash > @@ -0,0 +1,2 @@ > +# Locally computed: > +sha256 fbe10d46f61d769f7d92a296102e4e2bd3ee16130f11c5b10a1aae590ea1f5ca pugixml-1.7.tar.gz > diff --git a/package/libpugixml/libpugixml.mk b/package/libpugixml/libpugixml.mk > new file mode 100644 > index 0000000..3c9987e > --- /dev/null > +++ b/package/libpugixml/libpugixml.mk > @@ -0,0 +1,19 @@ > +################################################################################ > +# > +# libpugixml > +# > +################################################################################ > + > +LIBPUGIXML_VERSION = 1.7 > +LIBPUGIXML_SOURCE = pugixml-$(LIBPUGIXML_VERSION).tar.gz If the package is called pugixml, this line isn't needed. Regards, Arnout > +LIBPUGIXML_SITE = http://github.com/zeux/pugixml/releases/download/v$(LIBPUGIXML_VERSION) > +LIBPUGIXML_LICENSE = MIT > +LIBPUGIXML_LICENSE_FILES = docs/manual.html > +LIBPUGIXML_INSTALL_STAGING = YES > + > +LIBPUGIXML_SUBDIR = scripts > + > +# build libpugixml as a shared library > +LIBPUGIXML_CONF_OPTS = -DBUILD_SHARED_LIBS=ON > + > +$(eval $(cmake-package)) >
Hi, Thanks for the pointers. I'm cleaning up the patches & I'll resubmit them. One question though: > The readme.txt is good enough as a license file. > > > +LIBPUGIXML_INSTALL_STAGING = YES > > + > > +LIBPUGIXML_SUBDIR = scripts > > Hum, why? Is this about the SUBDIR option? The CMakeLists.txt file is in a subfolder called scripts, so, it seems that I need this? Is there another option/setting? Or should I add a comment about it? I'm not sure what you are asking? Theo
I fixed all the mentioned issues, except:
> +LIBPUGIXML_SUBDIR = scripts
In this release, CMakelists.txt is located in a subfolder.
This has been fixed in the git tree, but that hasn't been released yet.
I've fixed the remarks, except: LIBPUGIXML_SUBDIR = scripts The location of the current release is inside a subfolder. This has been fixed in the git tree, but isn't present in the release itself. Theo
Hello, On Tue, 6 Sep 2016 14:07:39 +0200, Theo Debrouwere wrote: > > The readme.txt is good enough as a license file. > > > > > +LIBPUGIXML_INSTALL_STAGING = YES > > > + > > > +LIBPUGIXML_SUBDIR = scripts > > > > Hum, why? > > Is this about the SUBDIR option? > > The CMakeLists.txt file is in a subfolder called scripts, > so, it seems that I need this? Is there another option/setting? > > Or should I add a comment about it? > > I'm not sure what you are asking? What confused me is that in the latest version of pugixml (i.e master branch), the CMakeLists.txt is directly at the root of the project: https://github.com/zeux/pugixml However, since you are packaging the 1.7 version, which is a bit older than the master branch, the CMakeLists.txt is indeed inside the scripts/ subdirectory: https://github.com/zeux/pugixml/tree/v1.7 So, it makes complete sense to use the SUBDIR option in this case. It's just that I was confused when looking at the Github repository, as I was looking at the master branch. Thanks for your contribution! Thomas
diff --git a/package/Config.in b/package/Config.in index 1e51a45..d0198ce 100644 --- a/package/Config.in +++ b/package/Config.in @@ -1092,6 +1092,7 @@ menu "JSON/XML" source "package/libxslt/Config.in" source "package/libyaml/Config.in" source "package/mxml/Config.in" + source "package/libpugixml/Config.in" source "package/rapidjson/Config.in" source "package/rapidxml/Config.in" source "package/raptor/Config.in" diff --git a/package/libpugixml/Config.in b/package/libpugixml/Config.in new file mode 100644 index 0000000..17c6eec --- /dev/null +++ b/package/libpugixml/Config.in @@ -0,0 +1,15 @@ +config BR2_PACKAGE_LIBPUGIXML + bool "libpugixml" + help + Light-weight, simple and fast XML parser for C++ with XPath support + + Features: + * DOM-like interface with rich traversal/modification capabilities + * Extremely fast non-validating XML parser which constructs the DOM tree + from an XML file/buffer + * XPath 1.0 implementation for complex data-driven tree queries + * Full Unicode support with Unicode interface variants and automatic + encoding conversions + + http://pugixml.org/ + https://github.com/zeux/pugixml diff --git a/package/libpugixml/libpugixml.hash b/package/libpugixml/libpugixml.hash new file mode 100644 index 0000000..9007f7c --- /dev/null +++ b/package/libpugixml/libpugixml.hash @@ -0,0 +1,2 @@ +# Locally computed: +sha256 fbe10d46f61d769f7d92a296102e4e2bd3ee16130f11c5b10a1aae590ea1f5ca pugixml-1.7.tar.gz diff --git a/package/libpugixml/libpugixml.mk b/package/libpugixml/libpugixml.mk new file mode 100644 index 0000000..3c9987e --- /dev/null +++ b/package/libpugixml/libpugixml.mk @@ -0,0 +1,19 @@ +################################################################################ +# +# libpugixml +# +################################################################################ + +LIBPUGIXML_VERSION = 1.7 +LIBPUGIXML_SOURCE = pugixml-$(LIBPUGIXML_VERSION).tar.gz +LIBPUGIXML_SITE = http://github.com/zeux/pugixml/releases/download/v$(LIBPUGIXML_VERSION) +LIBPUGIXML_LICENSE = MIT +LIBPUGIXML_LICENSE_FILES = docs/manual.html +LIBPUGIXML_INSTALL_STAGING = YES + +LIBPUGIXML_SUBDIR = scripts + +# build libpugixml as a shared library +LIBPUGIXML_CONF_OPTS = -DBUILD_SHARED_LIBS=ON + +$(eval $(cmake-package))