Broker command line options and build executable #772

kleunen · 2020-12-21T20:14:41Z

I added command line options (and config file) to configure the broker.

General options:
  --help                                produce help message
  --cfg arg                             Load configuration file
  --verbose arg (=1)                    set verbose level, possible values:
                                         0 - Fatal
                                         1 - Error
                                         2 - Warning
                                         3 - Info
                                         4 - Debug
                                         5 - Trace

TCP Server options:
  --tcp.port arg (=1883)                default port (TCP)
  --tcp.enable arg (=1)                 0/1

TCP Websocket Server options:
  --ws.port arg (=10080)                default port (TCP)
  --ws.enable arg (=0)                  0/1

TLS Server options:
  --tls.port arg (=8883)                default port (TLS)
  --tls.enable arg (=0)                 0/1
  --tls.certificate arg (=server.crt.pem)
                                        Certificate file
  --tls.private_key arg (=server.key.pem)
                                        Private key file

TLS Websocket Server options:
  --tlsws.port arg (=10443)             default port (TLS)
  --tlsws.enable arg (=0)               0/1

codecov · 2020-12-21T20:31:38Z

Codecov Report

Merging #772 (827ffea) into master (89003fa) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #772   +/-   ##
=======================================
  Coverage   85.05%   85.05%           
=======================================
  Files          61       61           
  Lines        8684     8684           
=======================================
  Hits         7386     7386           
  Misses       1298     1298

redboltz · 2020-12-22T00:45:03Z

I'm not sure why it happened but your PR seems to contain already merged change.
See
aaa61d1

For example

https://github.com/redboltz/mqtt_cpp/pull/772/files#diff-8438e6574a9cbca06fc27190e2dab4239f23e5e53b736428ad7b1d37dec831e7R329

is

aaa61d1#diff-8438e6574a9cbca06fc27190e2dab4239f23e5e53b736428ad7b1d37dec831e7R329

Rebase might solve the problem....?

redboltz · 2020-12-22T00:54:10Z

Basically, I agree to add commandline options to the example broker.
Using boost program_options is good.
Providing full options only is good. I mean shortcut option like -P shouldn't be provided at least this timing.

  --tcp.port arg (=1883)                default port (TCP)
  --tcp.enable arg (=1)                 0/1

But I don't think enabler and port pattern is good.
tcp.port depends on tcp.enable. It is confusing.
example/broker is just an example. So you don't need to care about backward compatibility.

I think that the better approach is only if ---tcp.port=1883 is provided then enable tcp.port, otherwise, disable tcp.port.
I know the well known MQTT (TCP) port is 1883. It is a good value for default.
So, how about this approach ?

Set the default --cfg parameter as broker.conf.
And mqtt_cpp provides the default broker.conf. In the broker.conf, set tcp.port as 1883.

By the way, if both broker.conf and commandline options are provided, then the commandline options should override the broker.conf one. Off course, the override should happen the individual option.

kleunen · 2020-12-22T06:55:43Z

Rebase might solve the problem....?

Yes. Made small mistake there with rebase

kleunen · 2020-12-22T06:57:16Z

What do you think about the executable build by CI now? I think it is useful to add to downloads on release, but maybe only build on tagged commit?

kleunen · 2020-12-22T07:09:42Z

But I don't think enabler and port pattern is good.

But then what if you want to enable tcp and use default port

--tcp.port
Uses default port 1883
--tcp.port=12345
Use specified port?

Or maybe:
--tcp and --tcp.port as option ?

But that would be the same as I have now:

Start with default: (1883)
./broker --tcp

Start with non-default port
./broker --tcp --tcp.port=12345

kleunen · 2020-12-22T07:11:22Z

And what about naming tls and tlsws or mqqts and wss?

kleunen · 2020-12-22T08:01:55Z

The branch is now properly based

kleunen · 2020-12-22T08:07:51Z

Static linking does give a warning, so I am not sure if it is really possible to completely static link the executable:

/usr/bin/clang++ -DBOOST_ASIO_NO_DEPRECATED -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -O3 -DNDEBUG -static -static-libgcc -static-libstdc++ CMakeFiles/broker.dir/broker.cpp.o -o broker  /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_log-mt-x64.a /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_program_options-mt-x64.a /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_system-mt-x64.a /usr/lib/x86_64-linux-gnu/libz.a /usr/lib/x86_64-linux-gnu/libssl.a /usr/lib/x86_64-linux-gnu/libcrypto.a -ldl /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_date_time-mt-x64.a /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_chrono-mt-x64.a /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_filesystem-mt-x64.a /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_regex-mt-x64.a /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_thread-mt-x64.a -lpthread /opt/hostedtoolcache/boost/1.72.0/x64/lib/libboost_atomic-mt-x64.a 
/usr/lib/x86_64-linux-gnu/libcrypto.a(dso_dlfcn.o): In function `dlfcn_globallookup':
(.text+0x11): warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/lib/x86_64-linux-gnu/libcrypto.a(b_addr.o): In function `BIO_lookup_ex':
(.text+0xcfc): warning: Using 'getaddrinfo' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/lib/x86_64-linux-gnu/libcrypto.a(b_sock.o): In function `BIO_gethostbyname':
(.text+0x71): warning: Using 'gethostbyname' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

kleunen · 2020-12-22T08:27:42Z

I removed the changes to CI from this PR, I will make a separate PR for this

kleunen · 2020-12-22T08:29:05Z

CMakeLists.txt

 ENDIF ()

 IF (MQTT_USE_TLS)
    FIND_PACKAGE (OpenSSL REQUIRED)
    IF (MQTT_USE_STATIC_OPENSSL)
-        FIND_PACKAGE (ZLIB REQUIRED)
+         IF (UNIX AND NOT APPLE)


This is also related to making a static build with CI, so I should move this to separate PR

Do you move this to other PR ? Or is it required to pass CI ?

Is moved to other PR

redboltz · 2020-12-22T08:58:13Z

But I don't think enabler and port pattern is good.

But then what if you want to enable tcp and use default port

--tcp.port
Uses default port 1883
--tcp.port=12345
Use specified port?

Or maybe:
--tcp and --tcp.port as option ?

I mean if --tcp.port is not passed, then disable tcp port.
See https://wandbox.org/permlink/WVoBGEGkaHOWv4xd

If user writes broker.conf as follows:

[tcp]
port=5678

Use 5678 as TCP port,
See https://wandbox.org/permlink/rpFpFb7jsg36vAL8
Be careful, there is broker.conf tab on wandbox.

If user set command line option, then it is used:
See https://wandbox.org/permlink/NIC74audyQE2ImMl
Be careful, there is Runtime options: text box on the left of the code.

And we can provide the default broker.conf in example directory.
It can be copied to build directory using cmake COPY operation.

kleunen · 2020-12-22T09:07:18Z

If user set command line option, then it is used:
See https://wandbox.org/permlink/NIC74audyQE2ImMl
Be careful, there is Runtime options: text box on the left of the code.

But this means you always have to specify the port, to enable TCP protocol

redboltz · 2020-12-22T09:09:22Z

If user set command line option, then it is used:
See https://wandbox.org/permlink/NIC74audyQE2ImMl
Be careful, there is Runtime options: text box on the left of the code.

But this means you always have to specify the port, to enable TCP protocol

Yes. It is no problem.
User should provide TCP port every time. It is better than unexpected port opens silently.
If user don't want to provide command line options, user can use broker.conf.
And broker.conf is provided by default.
So user can simple run ./broker then port 1883 is opened as TCP port.

redboltz · 2020-12-22T09:11:47Z

And what about naming tls and tlsws or mqqts and wss?

You mean this ?

	TCP	WS
NON TLS	mqtt	ws
TLS	mqtts	wss

I know this is widely used but I don't think it is good name for options.
Because it is not well structured.
ws and wss are used for generic websocket, not only mqtt.
I think that it is similar level as tcp.
I mean it is unbalance that only tcp and tls port options has mqtt information.

Here is the straight forward notation that reflects protocol layer:

	TCP	WS
NON TLS	tcp.port	tcp.ws.port
TLS	tcp.tls.port	tcp.tls.ws.port

I think that it is too complicated.

How about this?

	TCP	WS
NON TLS	tcp.port	ws.port
TLS	tls.port	wss.port

There is no mqtt information. It is redundant because the broker is for mqtt.
tcp, tls, ws, and wss are similar level.

redboltz · 2020-12-22T09:18:14Z

If user set command line option, then it is used:
See https://wandbox.org/permlink/NIC74audyQE2ImMl
Be careful, there is Runtime options: text box on the left of the code.

But this means you always have to specify the port, to enable TCP protocol

Yes. It is no problem.
User should provide TCP port every time. It is better than unexpected port opens silently.
If user don't want to provide command line options, user can use broker.conf.
And broker.conf is provided by default.
So user can simple run ./broker then port 1883 is opened as TCP port.

May be we need config file not found case process:
https://wandbox.org/permlink/THnT9s38bivxN63P

kleunen · 2020-12-22T09:25:27Z

TCP WS
NON TLS tcp.port ws.port
TLS tls.port wss.port
There is no mqtt information. It is redundant because the broker is for mqtt.
tcp, tls, ws, and wss are similar level.

Yes I think this is best option

redboltz · 2020-12-22T09:32:51Z

test/system/test_server_tls.hpp

            ),
-            std::move(ctx),
+            std::forward<boost::asio::ssl::context>(ctx),


Suggested change

std::forward<boost::asio::ssl::context>(ctx),

MQTT_NS::force_move(ctx),

ctx is rvalue-reference not forwarding reference in your code.

redboltz · 2020-12-22T09:33:23Z

test/system/test_server_tls_ws.hpp

            ),
-            std::move(ctx),
+            std::forward<boost::asio::ssl::context>(ctx),


Suggested change

std::forward<boost::asio::ssl::context>(ctx),

MQTT_NS::force_move(ctx),

ctx is rvalue-reference not forwarding reference in your code.

kleunen · 2020-12-22T10:17:40Z

I think I changed spaces to tabs now, the coding rules does not says tabs vs spaces ?

redboltz · 2020-12-22T10:39:27Z

I think I changed spaces to tabs now, the coding rules does not says tabs vs spaces ?

I will add this. Please use spaces.

kleunen · 2020-12-22T10:43:01Z

Error copying file "/Users/runner/work/mqtt_cpp/mqtt_cpp/example/server.key.pem" to "/Users/runner/work/_temp/example/broker.key.pem".

on macos.

Strange ...

kleunen · 2020-12-22T17:29:59Z

Maybe include a systemd init script for running the broker as well ?

redboltz · 2020-12-30T01:15:07Z

Sorry for my late merge. I'm under winter holidays :)

kleunen force-pushed the build_artifact branch from 7aadd03 to a78e4f6 Compare December 21, 2020 20:15

kleunen force-pushed the build_artifact branch from a78e4f6 to f520af6 Compare December 22, 2020 08:00

kleunen force-pushed the build_artifact branch from f520af6 to 80f9787 Compare December 22, 2020 08:25

kleunen commented Dec 22, 2020

View reviewed changes

redboltz reviewed Dec 22, 2020

View reviewed changes

kleunen force-pushed the build_artifact branch from 80f9787 to 4d1f151 Compare December 22, 2020 10:05

kleunen force-pushed the build_artifact branch 3 times, most recently from d2d7f64 to b048759 Compare December 22, 2020 12:08

Broker command line options and build executable

827ffea

kleunen force-pushed the build_artifact branch 2 times, most recently from a2a21d0 to 827ffea Compare December 22, 2020 13:27

redboltz merged commit b5e5f57 into redboltz:master Dec 30, 2020

redboltz mentioned this pull request Dec 30, 2020

CI make build of broker #773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broker command line options and build executable #772

Broker command line options and build executable #772

kleunen commented Dec 21, 2020 •

edited

Loading

codecov bot commented Dec 21, 2020 •

edited

Loading

redboltz commented Dec 22, 2020

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020 •

edited

Loading

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen Dec 22, 2020

redboltz Dec 22, 2020

kleunen Dec 23, 2020

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz commented Dec 22, 2020

redboltz commented Dec 22, 2020 •

edited

Loading

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz Dec 22, 2020

redboltz Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz commented Dec 30, 2020

	std::forward<boost::asio::ssl::context>(ctx),
	MQTT_NS::force_move(ctx),

Broker command line options and build executable #772

Broker command line options and build executable #772

Conversation

kleunen commented Dec 21, 2020 • edited Loading

codecov bot commented Dec 21, 2020 • edited Loading

Codecov Report

redboltz commented Dec 22, 2020

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020 • edited Loading

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen Dec 22, 2020

Choose a reason for hiding this comment

redboltz Dec 22, 2020

Choose a reason for hiding this comment

kleunen Dec 23, 2020

Choose a reason for hiding this comment

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz commented Dec 22, 2020

redboltz commented Dec 22, 2020 • edited Loading

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz Dec 22, 2020

Choose a reason for hiding this comment

redboltz Dec 22, 2020

Choose a reason for hiding this comment

kleunen commented Dec 22, 2020

redboltz commented Dec 22, 2020

kleunen commented Dec 22, 2020

kleunen commented Dec 22, 2020

redboltz commented Dec 30, 2020

kleunen commented Dec 21, 2020 •

edited

Loading

codecov bot commented Dec 21, 2020 •

edited

Loading

kleunen commented Dec 22, 2020 •

edited

Loading

redboltz commented Dec 22, 2020 •

edited

Loading