Add nojsonrpc qmake CONFIG option #2660

pljones · 2022-06-18T21:29:41Z

Short description of changes

JSON-RPC is introduced with Jamulus 3.9.0 in an initial form. As some potential problems may arise, having a build option to omit it gives the flexibility to (hopefully) avoid such problems.

CHANGELOG: Add nojsonrpc qmake CONFIG option to remove JSON-RPC support if requested

Context

Avoids potential issues with JSON-RPC. Also keeps the builds faster and smaller (headless serveronly nojsonrpc is 2.2M, a full build is 3.4M).

Does this change need documentation? What needs to be documented and how?

We should document it as part of the "How to build the server" documentation. But not mentioning it won't directly cause problems.

Status of this Pull Request

My directories and servers have been built with this and run with this for ages.

What is missing until this pull request can be merged?

Should be good to go but @dtinth and @pgScorpio looking over it would probably be good.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

hoffie

Personally, I'm not too convinced whether the additional logic is worth the benefits.

Avoids potential issues with JSON-RPC. Also keeps the builds faster and smaller (headless serveronly nojsonrpc is 2.2M, a full build is 3.4M).

I'd argue that a runtime-disabled JSON-RPC interface should be equally issue-safe.

Is the 1.2M binary difference solely due to nojsonrpc though? If so, that'd certainly convince me, but I suspect that most of it comes from the disabling of client code and client dependencies.

src/main.cpp

pljones · 2022-06-19T07:47:48Z

Personally, I'm not too convinced whether the additional logic is worth the benefits.

There's almost no logic involved. The benefit is more in terms of control over what's in the build. I don't want RPC (currently). I might never want it in my server and directory builds.

Also, given that concerns (#2614) over it have been raised, I think the benefits are there.

hoffie

If the free extra was an individual commit that had a Fixes: #2574 that would be even nicer... :)

pljones · 2022-06-19T20:45:49Z

If the free extra was an individual commit that had a Fixes: #2574 that would be even nicer... :)

It does appear to :)

peter@fs-peter:~/git/Jamulus-wip$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
peter@fs-peter:~/git/Jamulus-wip$ make distclean; qmake CONFIG+=serveronly; make main.o
...
g++ -c -pipe -O2 -std=gnu++11 -D_REENTRANT -Wall -W -fPIC -DAPP_VERSION=\"3.8.2dev-d0167e79\" -DCUSTOM_MODES -D_REENTRANT -DQT_NO_DEPRECATED_WARNINGS -DHAVE_LRINTF -DHAVE_STDINT_H -DSERVER_ONLY -DOPUS_X86_MAY_HAVE_SSE -DOPUS_X86_MAY_HAVE_SSE2 -DOPUS_X86_MAY_HAVE_SSE4_1 -DOPUS_X86_PRESUME_SSE=1 -DOPUS_X86_PRESUME_SSE2=1 -DCPU_INFO_BY_C -DOPUS_BUILD=1 -DUSE_ALLOCA=1 -DOPUS_HAVE_RTCD=1 -DHAVE_LRINTF=1 -DHAVE_LRINT=1 -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_XML_LIB -DQT_CONCURRENT_LIB -DQT_CORE_LIB -I. -Isrc -Ilibs/opus/include -Ilibs/opus/celt -Ilibs/opus/silk -Ilibs/opus/silk/float -Ilibs/opus/silk/fixed -Ilibs/opus -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem /usr/include/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork -isystem /usr/include/x86_64-linux-gnu/qt5/QtXml -isystem /usr/include/x86_64-linux-gnu/qt5/QtConcurrent -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -I. -I. -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ -o main.o src/main.cpp
src/main.cpp: In function ‘int main(int, char**)’:
src/main.cpp:84:18: warning: variable ‘bShowComplRegConnList’ set but not used [-Wunused-but-set-variable]
   84 |     bool         bShowComplRegConnList       = false;
      |                  ^~~~~~~~~~~~~~~~~~~~~
src/main.cpp:88:18: warning: variable ‘bShowAnalyzerConsole’ set but not used [-Wunused-but-set-variable]
   88 |     bool         bShowAnalyzerConsole        = false;
      |                  ^~~~~~~~~~~~~~~~~~~~
src/main.cpp:89:18: warning: variable ‘bMuteStream’ set but not used [-Wunused-but-set-variable]
   89 |     bool         bMuteStream                 = false;
      |                  ^~~~~~~~~~~
src/main.cpp:93:18: warning: variable ‘bNoAutoJackConnect’ set but not used [-Wunused-but-set-variable]
   93 |     bool         bNoAutoJackConnect          = false;
      |                  ^~~~~~~~~~~~~~~~~~
peter@fs-peter:~/git/Jamulus-wip$

peter@fs-peter:~/git/Jamulus-wip$ git log master..
d0167e79 2022-06-18 (HEAD -> pljones/work-in-progress, origin/pljones/work-in-progress, origin/feature/add-nojsonrpc-build-option, feature/add-nojsonrpc-build-option) Add nojsonrpc qmake CONFIG option [Peter L Jones]
peter@fs-peter:~/git/Jamulus-wip$ make distclean; qmake CONFIG+=serveronly; make main.o
...
g++ -c -pipe -O2 -std=gnu++11 -D_REENTRANT -Wall -W -fPIC -DAPP_VERSION=\"3.8.2dev-d0167e79\" -DCUSTOM_MODES -D_REENTRANT -DQT_NO_DEPRECATED_WARNINGS -DHAVE_LRINTF -DHAVE_STDINT_H -DSERVER_ONLY -DOPUS_X86_MAY_HAVE_SSE -DOPUS_X86_MAY_HAVE_SSE2 -DOPUS_X86_MAY_HAVE_SSE4_1 -DOPUS_X86_PRESUME_SSE=1 -DOPUS_X86_PRESUME_SSE2=1 -DCPU_INFO_BY_C -DOPUS_BUILD=1 -DUSE_ALLOCA=1 -DOPUS_HAVE_RTCD=1 -DHAVE_LRINTF=1 -DHAVE_LRINT=1 -DQT_NO_DEBUG -DQT_WIDGETS_LIB -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_XML_LIB -DQT_CONCURRENT_LIB -DQT_CORE_LIB -I. -Isrc -Ilibs/opus/include -Ilibs/opus/celt -Ilibs/opus/silk -Ilibs/opus/silk/float -Ilibs/opus/silk/fixed -Ilibs/opus -isystem /usr/include/x86_64-linux-gnu/qt5 -isystem /usr/include/x86_64-linux-gnu/qt5/QtWidgets -isystem /usr/include/x86_64-linux-gnu/qt5/QtGui -isystem /usr/include/x86_64-linux-gnu/qt5/QtNetwork -isystem /usr/include/x86_64-linux-gnu/qt5/QtXml -isystem /usr/include/x86_64-linux-gnu/qt5/QtConcurrent -isystem /usr/include/x86_64-linux-gnu/qt5/QtCore -I. -I. -I/usr/lib/x86_64-linux-gnu/qt5/mkspecs/linux-g++ -o main.o src/main.cpp
peter@fs-peter:~/git/Jamulus-wip$

ann0see

If I find some time, I‘ll test it

src/main.cpp

ann0see

Ok. Thanks. It works. Wouldn't we want to exit with an error code (exit(1)) if an unsupported cli option is given?

pljones · 2022-06-22T21:24:25Z

Wouldn't we want to exit with an error code (exit(1)) if an unsupported cli option is given?

It's all a bit hit and miss currently. Some do, some don't. If you run the server build and don't say -s, is that an error? If you run the non-GUI build and don't say -n, is that an error? Well, it was inconsistent before and it's still inconsistent now.

So for JSON-RPC, if you ask for it in a non-JSON-RPC build, well, it's just ignored - but not silently.

For the options that only apply in certain modes, checking was inconsistent...

ann0see · 2022-06-23T05:53:10Z

I think from the user point of view, there's a difference between forgetting an option (-s) from a specialised build (=serveronly) (which is predictably an user error but we can predict what is wanted) or adding an option which we cannot assume to be "default on" like some jsonrpc option to a non jsonrpc enabled build (we can assume the user explicitly requested it but the option is disabled during compiling).

But it's certainly debatable.

ann0see · 2022-07-25T17:32:50Z

@pljones I added "if requested" to the changelog entry.

pljones · 2022-07-26T06:26:15Z

It seems redundant to me. All the config options need to be requested.

ann0see · 2022-07-26T06:35:46Z

Then maybe turn it around:

Users can disable JSON-RPC by adding the nojsonrpc CONFIG option during build time

pljones · 2022-07-26T15:50:21Z

That doesn't follow the style guide for the ChangeLog. It's meant to be a (brief) past tense statement of what's changed in the code base.

Build: Added nojsonrpc qmake CONFIG option to remove JSON-RPC support
This has the "tag", a statement of what's been changed, and states the effect it has clearly.

ann0see · 2022-07-26T16:04:28Z

Hmm. I don’t think we have a style guide for the ChangeLog yet. My thought is that the changelog is a place for brief description and documentation for users (who might not know the Code but have some technical background). So the question is: What does the user need to know about this change? What can he do now?

I think your suggestion is still OK.

pljones · 2022-07-26T16:28:54Z

Mmm, the Pull Request template says a short, end-user understandable sentence in past tense. Given the "end user" here is someone who's decided find out what the build options are, I still think what I've suggested complies.

ann0see · 2022-07-26T16:52:51Z

As I said, I'm ok with your suggestion

pljones requested review from a team and gilgongo and removed request for a team June 18, 2022 21:29

hoffie reviewed Jun 18, 2022

View reviewed changes

src/main.cpp Show resolved Hide resolved

src/main.cpp Show resolved Hide resolved

hoffie approved these changes Jun 19, 2022

View reviewed changes

pljones linked an issue Jun 19, 2022 that may be closed by this pull request

main.cpp: unused-but-set-variable warnings for command line options with CONFIG+=serveronly #2574

Closed

pljones force-pushed the feature/add-nojsonrpc-build-option branch from d0167e7 to 37d58b8 Compare June 19, 2022 21:09

ann0see reviewed Jun 22, 2022

View reviewed changes

src/main.cpp Show resolved Hide resolved

ann0see added the needs documentation PRs requiring documentation changes or additions label Jun 22, 2022

pljones added 2 commits June 22, 2022 17:48

jamulussoftware#2574 Clean up unused messages

fdb904b

Add nojsonrpc qmake CONFIG option

10bdc8f

pljones force-pushed the feature/add-nojsonrpc-build-option branch from 37d58b8 to 10bdc8f Compare June 22, 2022 16:54

ann0see approved these changes Jun 22, 2022

View reviewed changes

pljones added this to the Release 3.9.0 milestone Jun 23, 2022

pljones merged commit 392e02a into jamulussoftware:master Jun 23, 2022

pljones deleted the feature/add-nojsonrpc-build-option branch June 23, 2022 06:20

ann0see mentioned this pull request Jul 4, 2022

COMPILING.md: Add table for compile time arguments #2707

Merged

4 tasks

ann0see removed the needs documentation PRs requiring documentation changes or additions label Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nojsonrpc qmake CONFIG option #2660

Add nojsonrpc qmake CONFIG option #2660

pljones commented Jun 18, 2022 •

edited by ann0see

Loading

hoffie left a comment

pljones commented Jun 19, 2022

hoffie left a comment

pljones commented Jun 19, 2022

ann0see left a comment

ann0see left a comment

pljones commented Jun 22, 2022 •

edited

Loading

ann0see commented Jun 23, 2022 •

edited

Loading

ann0see commented Jul 25, 2022

pljones commented Jul 26, 2022

ann0see commented Jul 26, 2022

pljones commented Jul 26, 2022 •

edited

Loading

ann0see commented Jul 26, 2022

pljones commented Jul 26, 2022 •

edited

Loading

ann0see commented Jul 26, 2022

Add nojsonrpc qmake CONFIG option #2660

Add nojsonrpc qmake CONFIG option #2660

Conversation

pljones commented Jun 18, 2022 • edited by ann0see Loading

Checklist

hoffie left a comment

Choose a reason for hiding this comment

pljones commented Jun 19, 2022

hoffie left a comment

Choose a reason for hiding this comment

pljones commented Jun 19, 2022

ann0see left a comment

Choose a reason for hiding this comment

ann0see left a comment

Choose a reason for hiding this comment

pljones commented Jun 22, 2022 • edited Loading

ann0see commented Jun 23, 2022 • edited Loading

ann0see commented Jul 25, 2022

pljones commented Jul 26, 2022

ann0see commented Jul 26, 2022

pljones commented Jul 26, 2022 • edited Loading

ann0see commented Jul 26, 2022

pljones commented Jul 26, 2022 • edited Loading

ann0see commented Jul 26, 2022

pljones commented Jun 18, 2022 •

edited by ann0see

Loading

pljones commented Jun 22, 2022 •

edited

Loading

ann0see commented Jun 23, 2022 •

edited

Loading

pljones commented Jul 26, 2022 •

edited

Loading

pljones commented Jul 26, 2022 •

edited

Loading