Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split out connectors into separate libraries #224

Open
cinemast opened this issue Jan 4, 2018 · 11 comments
Open

Split out connectors into separate libraries #224

cinemast opened this issue Jan 4, 2018 · 11 comments

Comments

@cinemast
Copy link
Owner

cinemast commented Jan 4, 2018

I plan to split out the connectors into a separate (or multiple) repositor(y/ies).

Rationale:

  • The number of connectors kind of escalated (which is a very good thing, because so many people actually contribute to this project).
    • Sadly I had to remove some Connectors (e.g. UDP Connector), because I could not take care well enough and I don't want to ship untested or flaky code
    • warning: currently some connectors are not enabled by default, because I don't trust them enough.
  • I no longer want to prevent releases with critical bugs in the core framework (JSON-RPC handling) just because some connector is behaving weird (My last release cost me 3 days because of that).
  • I hope this will allow for more agile releases (within an hour).
  • CI jobs will be shorter in the future and therefore also allow for quicker releases
  • It is already kind of hard to add connectors, especially if they require additional dependencies (e.g. redis, curl, libmicrohttpd).
    • People have to rewrite a lot of cmake logic, including find modules etc.
    • I think it should be more easy to add those in a stable way
    • I no longer want to throw away connectors just because they prevent a release from working.
  • It makes it easier for packages if distributions. They can now decide which connectors they want to package in additional packages, aside from the core library. The current state does not allow that.
    • e.g. in Debian the package now requires curl, libmicrohttpd, libhiredes, etc. which might not be what everybody needs.
  • By moving the connectors into a different repository, It also makes it easier to test them isolated from the rest of the code (no more messing with cmake files).
  • Allows fuzzing more easily and independent of the core framework / connectors and vice versa.
  • It will finally allow for a proper windows build which is currently non-existend from my point of view.
  • Windows users can then use their own windows based connectors e.g. No more weird #ifdef stuff in connectors code.
@cinemast
Copy link
Owner Author

cinemast commented Jan 4, 2018

Before I do this, I would love some comments/insights from people already using this library in some way.

Please share your opinions and contribute to this thread.

@debris @chfast @mgorny @alexandre-poirot @bcachet @gogo40 @SE2Dev @MiLk @joegen @cblp @tvannoy @jdmichaud

@cblp
Copy link
Contributor

cblp commented Jan 4, 2018

I don't use it already

@jdmichaud
Copy link
Contributor

Although it seems a good idea, it might be overkill. I mean, it's not like plugins in a complex application (e.g. sublime text). Connectors are really part of your library.
I am curious as to why it took 3 days for your to perform your release. You have a CI with tests and coverage running, so anyone modifying a connector has to go through the whole CI and ensure everything works before being able to merge.
That being said, I do agree your build infra could be simplified. Adding a connector is more complicated that it should be.

@ghost
Copy link

ghost commented Jan 4, 2018

I agree that adding connectors should be easier. I also like the idea of being able to separate connectors out into separate packages, reducing the amount of extra dependencies. Separating the building and testing of connectors from the main library code would also be nice.

I think it's worth refactoring the library to make adding, building, and testing connectors easier and more modular. Separating them into a separate repository might be overkill, though; perhaps refactoring the build system and directory structure could accomplish the most important goals. I definitely think you should move forward with something that makes connectors easier to add and maintain, regardless of what the final solution is.

@chfast
Copy link
Contributor

chfast commented Jan 5, 2018

I agree it might be overkill, unless there is another package that can use connectors only. For my experience, maintaining 2 repos instead of 1 is more, not less work.

Are the connectors JSON-RPC agnostic? If yes, long term it may be useful to separate those.

@joegen
Copy link
Contributor

joegen commented Jan 5, 2018

I'd rather not. I agree that connectors are part of the library. I would advise that this project just convert the connectors to plugins and have separate release cycles. If the plugin is untested, then mark it as unstable. Another would be to use separate module directories for experimental, unstable and release connectors in the plugin hierarchy.

@cinemast
Copy link
Owner Author

cinemast commented Jan 5, 2018

Thanks for your input. I now agree two repositories might be an overkill.
But I think each connector should be split out into a separate library (plug-in) and separate test suites (this would avoid a lot of #ifdefs).
Additionally this would reduce the number of dependencies for the core project and external dependencies will only get pulled-in by each connector, not by the client/server library itself.

@chfast connectors are only not JSON-RPC agnostic (they do require the header interfaces), thanks for pointing that out.

So new build-outputs could be:

  • libjsonrpc-server.so

  • libjsonrpc-server-test

  • libjsonrpc-client.so

  • libjsonrpc-client-test

  • libjsonrpc-connector-microhttpd.so

  • libjsonrpc-connector-microhttpd-test

  • libjsonrpc-connector-curl.so

  • libjsonrpc-connector-curl-test

  • ....

This allows a client/server to compile with only linking the dependencies that are actually needed:

g++ jsonrpcserver.cpp -ljsonrpc-server -ljsonrpc-connector-microhttpdserver
g++ jsonrpcclient.cpp -ljsonrpc-client -ljsonrpc-connector-curl

The source structure could than be the following:

  • src/client/{.h,.cpp, test.cpp}
  • src/server/{.h,.cpp, test.cpp}
  • src/connector/microhttpd/{.h,.cpp, test.cpp}
  • src/connector/curl/{.h,.cpp, test.cpp}

@cinemast
Copy link
Owner Author

cinemast commented Jan 5, 2018

@jdmichaud about the 3 days.

It was mostly my own stupdity. I had an unreproducible dep: catch, the unit testing library. Additionally some connectors produced flaky tests under OSX and sometimes linux.

@tvannoy
Copy link
Contributor

tvannoy commented Jan 5, 2018

I really like the idea of having each connector be it's own library that can be linked in at compile-time.

I think the directory structure @cinemast just proposed would work well. Keeping the connectors in the main libjson-rpc-cpp repo but separating the connectors into their own libraries and test suites is currently my preferred solution.

@mgorny
Copy link
Contributor

mgorny commented Jan 6, 2018

I'm not using this library myself anymore but I'm still maintaining the package in Gentoo. I'm happy with whatever works for you guys, as long as it can be built sanely and doesn't use horrible hacks ;-).

@cinemast cinemast changed the title Split out connectors into separate repository Split out connectors into separate libraries Nov 3, 2018
@cinemast
Copy link
Owner Author

The v2 branch contains the basic ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants