Skip to contentSkip to conversation

package/libtorrent-rasterbar: new package

PATCH New Philipp Richter 3 participants 6 messages Sep 11, 2018 – Oct 13, 2018 1 message not in the archive

lore
Share & tools
lore

Philipp Richter

author
#0 lore raw

PATCH 1/2 package/libtorrent-rasterbar: new package

libtorrent is a feature complete C++ bittorrent implementation focusing on efficiency and scalability.

https://www.libtorrent.org/

Signed-off-by Philipp Richter <[email protected]>
---
 DEVELOPERS                                    |   3 +
 package/Config.in                             |   1 +
 package/libtorrent-rasterbar/Config.in        | 133 ++++++++++++++++++
 .../libtorrent-rasterbar.hash                 |   3 +
 .../libtorrent-rasterbar.mk                   |  95 +++++++++++++
 5 files changed, 235 insertions(+)
 create mode 100644 package/libtorrent-rasterbar/Config.in
 create mode 100644 package/libtorrent-rasterbar/libtorrent-rasterbar.hash
 create mode 100644 package/libtorrent-rasterbar/libtorrent-rasterbar.mk
DEVELOPERS +3 -0
index b05564dd44..5cf182e5fe 100644
@@ -1668,6 +1668,9 @@ F:	package/psplash/
 F:	package/sispmctl/
 F:	package/zsh/
 
+N:	Philipp Richter <[email protected]>
+F:	package/libtorrent-rasterbar/
+
 N:	Philippe Proulx <[email protected]>
 F:	package/lttng-babeltrace/
 F:	package/lttng-libust/
package/Config.in +1 -0
index fb4d328846..d1b4147833 100644
@@ -1497,6 +1497,7 @@ menu "Networking"
 	source "package/libstrophe/Config.in"
 	source "package/libtirpc/Config.in"
 	source "package/libtorrent/Config.in"
+	source "package/libtorrent-rasterbar/Config.in"
 	source "package/libupnp/Config.in"
 	source "package/libupnp18/Config.in"
 	source "package/libupnpp/Config.in"
package/libtorrent-rasterbar/Config.in new file +133 -0

133 lines not on this page · raw message

package/libtorrent-rasterbar/libtorrent-rasterbar.hash new file +3 -0
new file mode 100644
index 0000000000..01ac303bae
@@ -0,0 +1,3 @@
+# Locally calculated
+sha256 d57a0f5b159f58003c3031943463503f0d05ae3e428dd7c2383d1e35fb2c4e8c libtorrent-rasterbar-1.1.9.tar.gz
+sha256 f3a5dd1558cce616b12edad521427ec8976ce2bb0af33f7f359cfa648bf55ad8 COPYING
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +95 -0

95 lines not on this page · raw message

Signature · 1 line
2.18.0
author
#1 lore raw

PATCH 2/2 package/qbittorrent: new package

The qBittorrent project aims to provide
an open-source software alternative to ?Torrent.

https://www.qbittorrent.org/

Signed-off-by Philipp Richter <[email protected]>
---
 DEVELOPERS                                    |  1 +
 package/Config.in                             |  1 +
 .../0001-fix-webui-unreachable-issue.patch    | 36 +++++++
 package/qbittorrent/Config.in                 | 97 +++++++++++++++++++
 package/qbittorrent/qbittorrent.hash          |  6 ++
 package/qbittorrent/qbittorrent.mk            | 49 ++++++++++
 6 files changed, 190 insertions(+)
 create mode 100644 package/qbittorrent/0001-fix-webui-unreachable-issue.patch
 create mode 100644 package/qbittorrent/Config.in
 create mode 100644 package/qbittorrent/qbittorrent.hash
 create mode 100644 package/qbittorrent/qbittorrent.mk
DEVELOPERS +1 -0
index 5cf182e5fe..03dfa9bf8c 100644
@@ -1670,6 +1670,7 @@ F:	package/zsh/
 
 N:	Philipp Richter <[email protected]>
 F:	package/libtorrent-rasterbar/
+F:	package/qbittorrent/
 
 N:	Philippe Proulx <[email protected]>
 F:	package/lttng-babeltrace/
package/Config.in +1 -0
index d1b4147833..cc7173cb4c 100644
@@ -1891,6 +1891,7 @@ endif
 	source "package/ptpd2/Config.in"
 	source "package/pure-ftpd/Config.in"
 	source "package/putty/Config.in"
+	source "package/qbittorrent/Config.in"
 	source "package/quagga/Config.in"
 	source "package/rabbitmq-server/Config.in"
 	source "package/radvd/Config.in"
package/qbittorrent/0001-fix-webui-unreachable-issue.patch new file +36 -0
new file mode 100644
index 0000000000..e7e955d813
@@ -0,0 +1,36 @@
+Backported from: 5f175e113ab0eaeaea560f58b6a255932b194892
+
+From 262c3a75bd3a99de16eea2327213bcd32b727d36 Mon Sep 17 00:00:00 2001
+From: Chocobo1 <[email protected]>
+Date: Sun, 19 Aug 2018 03:28:41 +0800
+Subject: [PATCH] Fix WebUI unreachable issue
+
+QVariant doesn't have constructor for plain char, by default it converts
+a plain char into an integer, hence the WebUI issue.
+Closes #9333.
+---
+ src/base/preferences.cpp | 4 ++--
+ 1 file changed, 2 insertions(+), 2 deletions(-)
+
+diff --git a/src/base/preferences.cpp b/src/base/preferences.cpp
+index 61c6a3d327..e82db5cb03 100644
+--- a/src/base/preferences.cpp
++++ b/src/base/preferences.cpp
+@@ -530,7 +530,7 @@ void Preferences::setWebUiAuthSubnetWhitelist(QStringList subnets)
+ 
+ QString Preferences::getServerDomains() const
+ {
+-    return value("Preferences/WebUI/ServerDomains", '*').toString();
++    return value("Preferences/WebUI/ServerDomains", QChar('*')).toString();
+ }
+ 
+ void Preferences::setServerDomains(const QString &str)
+@@ -540,7 +540,7 @@ void Preferences::setServerDomains(const QString &str)
+ 
+ QString Preferences::getWebUiAddress() const
+ {
+-    return value("Preferences/WebUI/Address", '*').toString().trimmed();
++    return value("Preferences/WebUI/Address", QChar('*')).toString().trimmed();
+ }
+ 
+ void Preferences::setWebUiAddress(const QString &addr)
package/qbittorrent/Config.in new file +97 -0

97 lines not on this page · raw message

package/qbittorrent/qbittorrent.hash new file +6 -0
new file mode 100644
index 0000000000..f5cf78e7fc
@@ -0,0 +1,6 @@
+# Locally checked with PGP signature from https://downloads.sourceforge.net/sourceforge/qbittorrent/qbittorrent-4.1.2.tar.xz.asc
+sha256 e0165bd427820c64bce596ef952d80058ea8cd7294d3c84bc723d79cd9e2f9c7 qbittorrent-4.1.2.tar.xz
+
+# Locally calculated
+sha256 ed266afaf97e160adc8954a2ddc6d1aeb63bc537b9b8b65348581239052bee03 0001-fix-webui-unreachable-issue.patch
+sha256 fc68233a17d308ee633aefedbd761c7582ec48557539aca310b4162e54212fe5 COPYING
package/qbittorrent/qbittorrent.mk new file +49 -0

49 lines not on this page · raw message

Signature · 1 line
2.18.0
#2 lore raw

PATCH 2/2 package/qbittorrent: new package

Hello Philipp,

Here as well, thanks for your submission. See below a number of comments.

package/qbittorrent/0001-fix-webui-unreachable-issue.patch line 1

Quoted 7 lines · Philipp Richter
package/qbittorrent/0001-fix-webui-unreachable-issue.patch new file +1 -0
new file mode 100644
index 0000000000..e7e955d813
@@ -0,0 +1,36 @@
+Backported from: 5f175e113ab0eaeaea560f58b6a255932b194892

This should go...

package/qbittorrent/0001-fix-webui-unreachable-issue.patch line 10

Quoted 9 lines
package/qbittorrent/0001-fix-webui-unreachable-issue.patch new file +9 -0
+
+From 262c3a75bd3a99de16eea2327213bcd32b727d36 Mon Sep 17 00:00:00 2001
+From: Chocobo1 <[email protected]>
+Date: Sun, 19 Aug 2018 03:28:41 +0800
+Subject: [PATCH] Fix WebUI unreachable issue
+
+QVariant doesn't have constructor for plain char, by default it converts
+a plain char into an integer, hence the WebUI issue.
+Closes #9333.

.... here.

And be followed by your Signed-off-by.

Two reasons:

- To keep the patch a valid git-formatted patch that can be applied with 'git am', the Backport from: .. should not be added at the beginning.

- We require contributors to sign-off on the patches they add to Buildroot to keep track of who added what.

package/qbittorrent/Config.in line 7

Quoted 13 lines
package/qbittorrent/Config.in new file +7 -0
new file mode 100644
index 0000000000..fb33e49d02
@@ -0,0 +1,97 @@
+comment "qbittorrent needs a toolchain w/ C++"
+	depends on !BR2_INSTALL_LIBSTDCPP
+
+config BR2_PACKAGE_QBITTORRENT
+	bool "qbittorrent"
+	depends on BR2_INSTALL_LIBSTDCPP
+	select BR2_PACKAGE_HOST_PKGCONF

As explained for libtorrent-rasterbar, this select is not needed.

package/qbittorrent/Config.in line 10

Quoted 3 lines
package/qbittorrent/Config.in new file +3 -0
+	select BR2_PACKAGE_BOOST
+	select BR2_PACKAGE_BOOST_SYSTEM
+	select BR2_PACKAGE_BOOST_THREAD

Is this package directly using Boost, or only indirectly because it's using libtorrent-rasterbar ? It's rather odd for a program to use both Boost and Qt.

package/qbittorrent/Config.in line 14

Quoted 4 lines
package/qbittorrent/Config.in new file +4 -0
+	select BR2_PACKAGE_LIBTORRENT_RASTERBAR
+	select BR2_PACKAGE_QT5
+	select BR2_PACKAGE_QT5BASE
+	select BR2_PACKAGE_ZLIB

As explained for libtorrent-rasterbar, you need to replicate all the dependencies of the packages you select. So something like this:

depends on BR2_INSTALL_LIBSTDCPP # boost, libtorrent-rasterbar, qt5 depends on BR2_TOOLCHAIN_HAS_THREADS # boost, libtorrent-rasterbar depends on BR2_USE_WCHAR # boost, libtorrent-rasterbar, qt5 depends on BR2_TOOLCHAIN_HAS_THREADS_NPTL # qt5
depends on !BR2_STATIC_LIBS # qt5
depends on !BR2_PACKAGE_QT # qt5

and of course, add the corresponding Config.in comment.

package/qbittorrent/Config.in line 26

Quoted 6 lines
package/qbittorrent/Config.in new file +6 -0
+if BR2_PACKAGE_QBITTORRENT
+
+config BR2_PACKAGE_QBITTORRENT_GUI
+	bool "GUI"
+	select BR2_PACKAGE_HICOLOR_ICON_THEME
+	select BR2_PACKAGE_QT5BASE_WIDGETS

You need to select BR2_PACKAGE_QT5BASE_GUI, otherwise BR2_PACKAGE_QT5BASE_WIDGETS can't be selected. It did work for you, because QT5SVG already selects QT5BASE_GUI, but it's better to be explicit here.

package/qbittorrent/Config.in line 35

Quoted 9 lines
package/qbittorrent/Config.in new file +9 -0
+	select BR2_PACKAGE_QT5SVG
+	help
+	  Disable for headless running.
+	  The target binary will be called qbittorrent-nox.
+
+if BR2_PACKAGE_QBITTORRENT_GUI
+
+config BR2_PACKAGE_QBITTORRENT_QTDBUS
+	bool "QtDBUS"

Perhaps the option should be named "D-Bus support" ?

package/qbittorrent/Config.in line 41

Quoted 6 lines
package/qbittorrent/Config.in new file +6 -0
+	default y
+	select BR2_PACKAGE_QT5BASE_DBUS
+	help
+	  Enable QtDBUS support.
+
+	  Default: yes

Drop those "Default: XYZ".

Quoted 5 lines
package/qbittorrent/Config.in new file +5 -0
+
+endif
+
+if !BR2_PACKAGE_QBITTORRENT_GUI
+

package/qbittorrent/Config.in line 56

Quoted 10 lines
package/qbittorrent/Config.in new file +10 -0
+comment "Systemd service needs systemd enabled"
+	depends on !BR2_PACKAGE_SYSTEMD
+
+config BR2_PACKAGE_QBITTORRENT_SYSTEMD
+	bool "Systemd service"
+	depends on BR2_PACKAGE_SYSTEMD
+	help
+	  Install systemd service file.
+
+	  Default: no

You don't need an option, just use BR2_INIT_SYSTEMD to decide whether the systemd service should be installed or not.

package/qbittorrent/Config.in line 61

Quoted 5 lines
package/qbittorrent/Config.in new file +5 -0
+
+endif
+
+comment "Stacktrace feature needs a glibc toolchain"
+	depends on !BR2_TOOLCHAIN_USES_GLIBC

Just use "auto" when glibc is available, no need to make this configurable.

package/qbittorrent/qbittorrent.hash line 5

Quoted 11 lines
package/qbittorrent/qbittorrent.hash new file +5 -0
new file mode 100644
index 0000000000..f5cf78e7fc
@@ -0,0 +1,6 @@
+# Locally checked with PGP signature from https://downloads.sourceforge.net/sourceforge/qbittorrent/qbittorrent-4.1.2.tar.xz.asc
+sha256 e0165bd427820c64bce596ef952d80058ea8cd7294d3c84bc723d79cd9e2f9c7 qbittorrent-4.1.2.tar.xz
+
+# Locally calculated
+sha256 ed266afaf97e160adc8954a2ddc6d1aeb63bc537b9b8b65348581239052bee03 0001-fix-webui-unreachable-issue.patch

No need to have hashes for patches that are part of the Buildroot tree.

Quoted 10 lines
package/qbittorrent/qbittorrent.hash new file +1 -0
+sha256 fc68233a17d308ee633aefedbd761c7582ec48557539aca310b4162e54212fe5 COPYING
package/qbittorrent/qbittorrent.mk new file +3 -0
new file mode 100644
index 0000000000..b90dd9ac84
@@ -0,0 +1,49 @@
+################################################################################
+#
+# qbittorrent

package/qbittorrent/qbittorrent.mk line 13

Quoted 10 lines
package/qbittorrent/qbittorrent.mk new file +10 -0
+#
+################################################################################
+
+QBITTORRENT_VERSION = 4.1.2
+QBITTORRENT_SOURCE = qbittorrent-$(QBITTORRENT_VERSION).tar.xz
+QBITTORRENT_SITE = https://downloads.sourceforge.net/sourceforge/qbittorrent
+QBITTORRENT_LICENSE = GPL-2.0
+QBITTORRENT_LICENSE_FILES = COPYING
+QBITTORRENT_DEPENDENCIES = host-pkgconf boost libtorrent-rasterbar qt5base zlib
+QBITTORRENT_CONF_OPTS += --with-boost-libdir="$(STAGING_DIR)/usr/lib"

You can drop the double quotes.

Could you take into account those comments, and send an updated version ?

Thanks a lot!

Thomas Petazzoni

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
#3 lore raw

PATCH 1/2 package/libtorrent-rasterbar: new package

In reply to Philipp Richter

Hello Philipp,

Thanks for your submission, and sorry for the slow response. I have to say that for a first contribution, it looks very good. Still, I have a number of comments, see below.

package/libtorrent-rasterbar/Config.in line 7

Quoted 13 lines · Philipp Richter
package/libtorrent-rasterbar/Config.in new file +7 -0
new file mode 100644
index 0000000000..0ce9496b70
@@ -0,0 +1,133 @@
+comment "libtorrent-rasterbar needs a toolchain w/ C++"
+	depends on !BR2_INSTALL_LIBSTDCPP
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR
+	bool "libtorrent-rasterbar"
+	depends on BR2_INSTALL_LIBSTDCPP
+	select BR2_PACKAGE_HOST_PKGCONF

You don't need to select this option. Currently, we don't care about selecting Config.in options for host packages, so for consistency, we also shouldn't do it here.

In the future, we might add Config.in options for all host packages, and if/when we do this, we'll add the necessary selects. But for the time being, I'd rather not have any of them.

package/libtorrent-rasterbar/Config.in line 11

Quoted 4 lines
package/libtorrent-rasterbar/Config.in new file +4 -0
+	select BR2_PACKAGE_BOOST
+	select BR2_PACKAGE_BOOST_CHRONO
+	select BR2_PACKAGE_BOOST_SYSTEM
+	select BR2_PACKAGE_BOOST_RANDOM

Since you select boost, you must inherit all its dependencies, i.e:

depends on BR2_INSTALL_LIBSTDCPP
depends on BR2_TOOLCHAIN_HAS_THREADS
depends on BR2_USE_WCHAR

package/libtorrent-rasterbar/Config.in line 27

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+if BR2_PACKAGE_LIBTORRENT_RASTERBAR
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_LOGGING
+	bool "Logging alerts"
+	default y
+	help
+	  Enable support for logging alerts,
+	  like log_alert, torrent_log_alert and peer_log_alert.
+
+	  Default: yes

Drop those "Default: XYZ" from the help text, we don't have them anywhere, and it's already pretty explicit from the Config.in text.

package/libtorrent-rasterbar/Config.in line 34

Quoted 6 lines
package/libtorrent-rasterbar/Config.in new file +6 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+	bool "Debug"
+	help
+	  Enable debug build.
+
+	  Default: no

What does --enable-debug/--disable-debug does? We have a global BR2_ENABLE_DEBUG boolean that enables building with debugging symbols.

Quoted 15 lines
package/libtorrent-rasterbar/Config.in new file +15 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DHT
+	bool "DHT"
+	default y
+	help
+	  Enable support for trackerless torrents.
+
+	  Default: yes
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_ENCRYPTION
+	bool "Encryption"
+	default y
+	select BR2_PACKAGE_OPENSSL
+	help
+	  Enable encryption support.
+	  Encryption support is the encrypted peer connection

package/libtorrent-rasterbar/Config.in line 60

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+	  supported by clients such as uTorrent, Azureus and KTorrent.
+
+	  Default: yes
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_EXPORT_ALL_SYMBOLS
+	bool "Export all symbols"
+	help
+	  Export all symbols from libtorrent, including non-public ones.
+
+	  Default: no

Do we really need an option for this ?

package/libtorrent-rasterbar/Config.in line 68

Quoted 7 lines
package/libtorrent-rasterbar/Config.in new file +7 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_POOL_ALLOCATORS
+	bool "Pool allocators"
+	default y
+	help
+	  Enable pool allocators for send buffers using boost::pool<>.
+
+	  Default: yes

And this ?

package/libtorrent-rasterbar/Config.in line 77

Quoted 8 lines
package/libtorrent-rasterbar/Config.in new file +8 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS
+	bool "Invariant checks"
+	default y
+	depends on BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+	help
+	  Enable invariant checks.
+
+	  Default: yes

This clearly seems like a developer-oriented option, I don't think we should support it in Buildroot.

Quoted 13 lines
package/libtorrent-rasterbar/Config.in new file +13 -0
+
+comment "Invariant checks need debug build"
+	depends on !BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+
+if BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS
+
+choice
+	prompt "Invariant checks type"
+	default BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS_YES
+	help
+	  Select the type of invariant checks to use.
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS_YES

package/libtorrent-rasterbar/Config.in line 100

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+	bool "yes"
+	help
+	  Standard invariant checks.
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS_FULL
+	bool "full"
+	help
+	  Turn on extra expensive invariant checks.
+
+endchoice

... and therefore we can remove all of this.

package/libtorrent-rasterbar/Config.in line 110

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+
+endif
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEPRECATED_FUNCTIONS
+	bool "Deprecated functions"
+	default y
+	help
+	  Enable deprecated functions in the API.
+
+	  Default: yes

Ditto, why would we want this option ?

In general, it feels like you have taken every --enable/--disable of the configure script, and added a Buildroot Config.in option for each of them. I don't think we necessarily want to support each and every configuration option of the upstream package.

Quoted 11 lines
package/libtorrent-rasterbar/Config.in new file +11 -0
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DISK_STATS
+	bool "Disk stats"
+	help
+	  Enable disk activity logging feature.
+
+	  Default: no
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_EXAMPLES
+	bool "Examples"
+	help

package/libtorrent-rasterbar/Config.in line 131

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+	  Build example files.
+
+	  Default: no
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_TESTS
+	bool "Tests"
+	help
+	  Build test files.
+
+	  Default: no

We generally don't build tests in the context of Buildroot, so just disable them unconditionally.

Quoted 7 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +1 -0
new file mode 100644
index 0000000000..19f0a9304d
@@ -0,0 +1,95 @@
+################################################################################

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 11

Quoted 10 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +10 -0
+#
+# libtorrent-rasterbar
+#
+################################################################################
+
+LIBTORRENT_RASTERBAR_VERSION = 1.1.9
+LIBTORRENT_RASTERBAR_SITE = https://github.com/arvidn/libtorrent/releases/download/libtorrent-$(subst .,_,$(LIBTORRENT_RASTERBAR_VERSION))
+LIBTORRENT_RASTERBAR_LICENSE = BSD-2-Clause
+LIBTORRENT_RASTERBAR_LICENSE_FILES = COPYING
+LIBTORRENT_RASTERBAR_DEPENDENCIES = host-pkgconf boost $(if $(BR2_PACKAGE_LIBICONV),libiconv)

I would move the $(if $(BR2_PACKAGE_LIBICONV),libiconv) part inside the iconv condition.

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 13

Quoted 2 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +2 -0
+LIBTORRENT_RASTERBAR_INSTALL_STAGING = YES
+LIBTORRENT_RASTERBAR_CONF_OPTS += --with-boost-libdir="$(STAGING_DIR)/usr/lib"

You can drop the double quotes here.

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 14

Quoted 1 line
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +1 -0
+LIBTORRENT_RASTERBAR_CONF_ENV += CXXFLAGS="$(TARGET_CXXFLAGS) -std=c++11"

So, you need C++11, so your package should have a dependency on gcc >= 4.8.

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 17

Quoted 2 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +2 -0
+ifeq ($(BR2_ENABLE_LOCALE)$(BR2_PACKAGE_LIBICONV),y)
+LIBTORRENT_RASTERBAR_CONF_OPTS += --with-libiconv

This is where I'd prefer to see:

LIBTORRENT_RASTERBAR_DEPENDENCIES += $(if $(BR2_PACKAGE_LIBICONV),libiconv)

Could you rework the package according to those comments?

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

A message that is not in the archive (sent off-list or not yet received): CA+Vb7hrYyCfVAq=nk8a9GfGW10Fatw3WXZ5=wsJuVqba6MPnYg@mail.gmail.com

author
#5 lore raw

PATCH 1/2 package/libtorrent-rasterbar: new package

CC to buildroot mailing list which somehow didn't go through.

Quoted 278 lines · Philipp Richter
Quoted 7 lines · Thomas Petazzoni

Hello Philipp,

Thanks for your submission, and sorry for the slow response. I have to say that for a first contribution, it looks very good. Still, I have a number of comments, see below.

Ah it's no problem, I know how busy things get. Thank you for the kind words.

Quoted 20 lines · Philipp Richter
Quoted 7 lines
package/libtorrent-rasterbar/Config.in new file +0 -0
new file mode 100644
index 0000000000..0ce9496b70

package/libtorrent-rasterbar/Config.in line 7

Quoted 7 lines
package/libtorrent-rasterbar/Config.in new file +7 -0
@@ -0,0 +1,133 @@
+comment "libtorrent-rasterbar needs a toolchain w/ C++"
+     depends on !BR2_INSTALL_LIBSTDCPP
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR
+     bool "libtorrent-rasterbar"
+     depends on BR2_INSTALL_LIBSTDCPP
+     select BR2_PACKAGE_HOST_PKGCONF

You don't need to select this option. Currently, we don't care about selecting Config.in options for host packages, so for consistency, we also shouldn't do it here.

Quoted 4 lines

In the future, we might add Config.in options for all host packages, and if/when we do this, we'll add the necessary selects. But for the time being, I'd rather not have any of them.

Ok I see.

Quoted 25 lines

package/libtorrent-rasterbar/Config.in line 11

Quoted 4 lines
package/libtorrent-rasterbar/Config.in new file +4 -0
+     select BR2_PACKAGE_BOOST
+     select BR2_PACKAGE_BOOST_CHRONO
+     select BR2_PACKAGE_BOOST_SYSTEM
+     select BR2_PACKAGE_BOOST_RANDOM

Since you select boost, you must inherit all its dependencies, i.e:

depends on BR2_INSTALL_LIBSTDCPP
depends on BR2_TOOLCHAIN_HAS_THREADS
depends on BR2_USE_WCHAR

package/libtorrent-rasterbar/Config.in line 27

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+if BR2_PACKAGE_LIBTORRENT_RASTERBAR
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_LOGGING
+     bool "Logging alerts"
+     default y
+     help
+       Enable support for logging alerts,
+       like log_alert, torrent_log_alert and peer_log_alert.
+
+       Default: yes

Drop those "Default: XYZ" from the help text, we don't have them anywhere, and it's already pretty explicit from the Config.in text.

Quoted 10 lines

package/libtorrent-rasterbar/Config.in line 34

Quoted 6 lines
package/libtorrent-rasterbar/Config.in new file +6 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+     bool "Debug"
+     help
+       Enable debug build.
+
+       Default: no

What does --enable-debug/--disable-debug does? We have a global BR2_ENABLE_DEBUG boolean that enables building with debugging symbols.

Absolutely, it was for the invariant checks which depend on debug being enabled. I was confused as to how to properly add the option and make it explicit to the user.

Quoted 19 lines
Quoted 19 lines
package/libtorrent-rasterbar/Config.in new file +19 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DHT
+     bool "DHT"
+     default y
+     help
+       Enable support for trackerless torrents.
+
+       Default: yes
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_ENCRYPTION
+     bool "Encryption"
+     default y
+     select BR2_PACKAGE_OPENSSL
+     help
+       Enable encryption support.
+       Encryption support is the encrypted peer connection
+       supported by clients such as uTorrent, Azureus and KTorrent.
+
+       Default: yes
+
Quoted 9 lines

package/libtorrent-rasterbar/Config.in line 60

Quoted 6 lines
package/libtorrent-rasterbar/Config.in new file +6 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_EXPORT_ALL_SYMBOLS
+     bool "Export all symbols"
+     help
+       Export all symbols from libtorrent, including non-public ones.
+
+       Default: no

Do we really need an option for this ?

I guess it could be useful if another package that depends on those internal symbols being exposed arrives. But for now I haven't seen anything like that.

Quoted 10 lines

package/libtorrent-rasterbar/Config.in line 68

Quoted 7 lines
package/libtorrent-rasterbar/Config.in new file +7 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_POOL_ALLOCATORS
+     bool "Pool allocators"
+     default y
+     help
+       Enable pool allocators for send buffers using boost::pool<>.
+
+       Default: yes

And this ?

I guess we can use package default of them being on ?

Quoted 12 lines

package/libtorrent-rasterbar/Config.in line 77

Quoted 8 lines
package/libtorrent-rasterbar/Config.in new file +8 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS
+     bool "Invariant checks"
+     default y
+     depends on BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+     help
+       Enable invariant checks.
+
+       Default: yes

This clearly seems like a developer-oriented option, I don't think we should support it in Buildroot.

That's is true, here's the thing though : if debug is enabled they will be switched on automatically and make the library much slower. They can be forced off to have the library with debugging information without expensive checks.
I guess we can go with the default behaviour, the other side of the coin is that he/she might be left scratching their head about the slowness.

Quoted 39 lines
Quoted 13 lines
package/libtorrent-rasterbar/Config.in new file +13 -0
+
+comment "Invariant checks need debug build"
+     depends on !BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+
+if BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS
+
+choice
+     prompt "Invariant checks type"
+     default BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS_YES
+     help
+       Select the type of invariant checks to use.
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS_YES

package/libtorrent-rasterbar/Config.in line 100

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+     bool "yes"
+     help
+       Standard invariant checks.
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS_FULL
+     bool "full"
+     help
+       Turn on extra expensive invariant checks.
+
+endchoice

... and therefore we can remove all of this.

package/libtorrent-rasterbar/Config.in line 110

Quoted 10 lines
package/libtorrent-rasterbar/Config.in new file +10 -0
+
+endif
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEPRECATED_FUNCTIONS
+     bool "Deprecated functions"
+     default y
+     help
+       Enable deprecated functions in the API.
+
+       Default: yes

Ditto, why would we want this option ?

Quoted 5 lines

In general, it feels like you have taken every --enable/--disable of the configure script, and added a Buildroot Config.in option for each of them. I don't think we necessarily want to support each and every configuration option of the upstream package.

Yes, that makes sense. Like the internal symbols option it was also with the idea of "Maybe an application depends on those symbols?" But then the developer would override the makefile themselves anyway I guess.

Quoted 16 lines
Quoted 16 lines
package/libtorrent-rasterbar/Config.in new file +16 -0
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DISK_STATS
+     bool "Disk stats"
+     help
+       Enable disk activity logging feature.
+
+       Default: no
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_EXAMPLES
+     bool "Examples"
+     help
+       Build example files.
+
+       Default: no
+
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_TESTS
Quoted 9 lines

package/libtorrent-rasterbar/Config.in line 131

Quoted 5 lines
package/libtorrent-rasterbar/Config.in new file +5 -0
+     bool "Tests"
+     help
+       Build test files.
+
+       Default: no

We generally don't build tests in the context of Buildroot, so just disable them unconditionally.

Right understood.

Quoted 31 lines
Quoted 7 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +1 -0
new file mode 100644
index 0000000000..19f0a9304d
@@ -0,0 +1,95 @@
+################################################################################

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 11

Quoted 10 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +10 -0
+#
+# libtorrent-rasterbar
+#
+################################################################################
+
+LIBTORRENT_RASTERBAR_VERSION = 1.1.9
+LIBTORRENT_RASTERBAR_SITE = https://github.com/arvidn/libtorrent/releases/download/libtorrent-$(subst .,_,$(LIBTORRENT_RASTERBAR_VERSION))
+LIBTORRENT_RASTERBAR_LICENSE = BSD-2-Clause
+LIBTORRENT_RASTERBAR_LICENSE_FILES = COPYING
+LIBTORRENT_RASTERBAR_DEPENDENCIES = host-pkgconf boost $(if $(BR2_PACKAGE_LIBICONV),libiconv)

I would move the $(if $(BR2_PACKAGE_LIBICONV),libiconv) part inside the iconv condition.

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 13

Quoted 2 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +2 -0
+LIBTORRENT_RASTERBAR_INSTALL_STAGING = YES
+LIBTORRENT_RASTERBAR_CONF_OPTS += --with-boost-libdir="$(STAGING_DIR)/usr/lib"

You can drop the double quotes here.

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 14

Quoted 1 line
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +1 -0
+LIBTORRENT_RASTERBAR_CONF_ENV += CXXFLAGS="$(TARGET_CXXFLAGS) -std=c++11"

So, you need C++11, so your package should have a dependency on gcc >= 4.8.

Quoted 8 lines

package/libtorrent-rasterbar/libtorrent-rasterbar.mk line 17

Quoted 2 lines
package/libtorrent-rasterbar/libtorrent-rasterbar.mk new file +2 -0
+ifeq ($(BR2_ENABLE_LOCALE)$(BR2_PACKAGE_LIBICONV),y)
+LIBTORRENT_RASTERBAR_CONF_OPTS += --with-libiconv

This is where I'd prefer to see:

LIBTORRENT_RASTERBAR_DEPENDENCIES += $(if $(BR2_PACKAGE_LIBICONV),libiconv)

Could you rework the package according to those comments?

Right, thank you for all these hints and comments. I'll rework this, leave more to the package's defaults.

Best Regards,
Philipp Richter

Quoted 8 lines

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

#6 lore raw

PATCH 1/2 package/libtorrent-rasterbar: new package

Quoted 7 lines · Philipp Richter

CC to buildroot mailing list which somehow didn't go through.

Quoted 3 lines · Philipp Richter

On Thu, 11 Oct 2018 at 21:19, Thomas Petazzoni
<[email protected]> wrote:

[snip]

Quoted 1 line
Quoted 1 line
Quoted 1 line · Philipp Richter

[snip]

Quoted 11 lines
Quoted 11 lines
Quoted 10 lines

package/libtorrent-rasterbar/Config.in line 68

Quoted 7 lines
package/libtorrent-rasterbar/Config.in new file +7 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_POOL_ALLOCATORS
+     bool "Pool allocators"
+     default y
+     help
+       Enable pool allocators for send buffers using boost::pool<>.
+
+       Default: yes

And this ?

Quoted 1 line
Quoted 1 line

I guess we can use package default of them being on ?

Yes, sounds good.

Quoted 13 lines
Quoted 13 lines
Quoted 12 lines

package/libtorrent-rasterbar/Config.in line 77

Quoted 8 lines
package/libtorrent-rasterbar/Config.in new file +8 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_INVARIANT_CHECKS
+     bool "Invariant checks"
+     default y
+     depends on BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEBUG
+     help
+       Enable invariant checks.
+
+       Default: yes

This clearly seems like a developer-oriented option, I don't think we should support it in Buildroot.

Quoted 7 lines
Quoted 7 lines

That's is true, here's the thing though : if debug is enabled they will be switched on automatically and make the library much slower. They can be forced off to have the library with debugging information without expensive checks.
I guess we can go with the default behaviour, the other side of the coin is that he/she might be left scratching their head about the slowness.

Better just pass --disable-invariant-checks unconditionally. And add the above
explanation in the commit log.

[snip]

Quoted 16 lines
Quoted 16 lines
Quoted 15 lines

package/libtorrent-rasterbar/Config.in line 110

Quoted 7 lines
package/libtorrent-rasterbar/Config.in new file +7 -0
+config BR2_PACKAGE_LIBTORRENT_RASTERBAR_DEPRECATED_FUNCTIONS
+     bool "Deprecated functions"
+     default y
+     help
+       Enable deprecated functions in the API.
+
+       Default: yes

Ditto, why would we want this option ?

In general, it feels like you have taken every --enable/--disable of the configure script, and added a Buildroot Config.in option for each of them. I don't think we necessarily want to support each and every configuration option of the upstream package.

Quoted 3 lines
Quoted 3 lines

Yes, that makes sense. Like the internal symbols option it was also with the idea of "Maybe an application depends on those symbols?" But then the developer would override the makefile themselves anyway I guess.

It's a bit unlikely that an application that is not in Buildroot yet would
depend on those deprecated symbols.

[snip]

Quoted 2 lines
Quoted 2 lines

Right, thank you for all these hints and comments. I'll rework this, leave more to the package's defaults.

For the defaults, we generally prefer to add explicit disable options. It
depends a bit though. Like e.g. the deprecated functions is something that we might just leave at the package default (i.e. no explicit option). But something like --disable-tests we generally do explicitly. There is no really strict rule (AFAIK).

Regards,
Arnout

Quoted 17 lines
Quoted 12 lines

Best Regards,
Philipp Richter

Quoted 8 lines

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
buildroot mailing list
buildroot at busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF