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

add option BUILD_CORE_ONLY or similar to listfile #207

Closed
CayOest opened this issue Mar 17, 2023 · 4 comments · Fixed by #208
Closed

add option BUILD_CORE_ONLY or similar to listfile #207

CayOest opened this issue Mar 17, 2023 · 4 comments · Fixed by #208

Comments

@CayOest
Copy link
Contributor

CayOest commented Mar 17, 2023

Hi,

I appreciate all the time you put into this great project.
It accelerated our project so much.

But there are a few things, though, I would like to complain about: :)

  1. The cpprest dependency

  2. The removal of the cmake option

  3. The confusing term "core".

  4. Well, we can handle the synchronicity of the "core" option on our own without problems: just throw away pplx and just use the std c++11 libs for asynchronicity. They should actually be better, but even if not: they are going to be good enough in general to not use a non-standard library. In my humble opinion, you should throw away the "fat" option altogether.

  5. But unfortunately, it seems to me that the CMake option BUILD_CORE_ONLY (or similar) disappeared. So we have to build the whole stuff we don't really need, i. e. cpprestsdk.
    We actually don't like cpprestsdk and forcibly shut it out from all our projects, because it was just too unreliable in Linux environments.

  6. Furthermore, I might add, at the beginning, we had no clue about the two "competing libraries": etcd-cpp-apiv3-core and etcd-cpp-apiv3.
    My intuition was that the "core" lib was the basis for the "real" lib. So I shipped both of them....
    My client's intuition was the same. So they included both of them...

The result was: our clients ran into "enum double registration" problems in Protobuf because both libs tried to register some protobuf enums independently.
It took quite a while to figure this out. For both of us, because client said: "your problem, dunno!", we said: "your problem: you cannot load protobuf twice!"... until we finally got it: "well, yeah the core the is not the core but just a slim version of the real thing. or a COPY of the real thing." Which led protobuf to not work.

My suggestion therefore is:

  1. Treat etcd-cpp-apiv3-core and etcd-cpp-apiv3 as real either-or alternatives; Either build and install the whole thing or just the "core".
  2. Don't name it "core", because it is really confusing as said above: name it - for instance - "lean, slim, light" - whatever.
  3. Make this WAY MORE TRANSPARENT than before, not just a footnote. (It is just a sentence in your README, and actually thanks to the guy who exported this to vcpkg: as a "Usage Note": NEVER TRY BOTH AT ONCE!
    But this should actually be not a footnote or a "usage note", but implemented as - again - an EITHER-OR-OPTION. This can be done.
    Otherwise your users will run into these kinds of problems over and over again.

Actually: advice #2 is the best. Do this! Name it "slim", "lean", "light", "lite", etc. but never "core"! :) Thanks in advance for all the painstaking debugging removed in the future! :)

And actually, the solution without cpprestsdk is by far the best - in the long run.

Awesome work, thank you very much!

@CayOest
Copy link
Contributor Author

CayOest commented Mar 17, 2023

Sorry, the formatting was broken after publishing. Nvm, won't fix that.

@CayOest
Copy link
Contributor Author

CayOest commented Mar 17, 2023

point 4-6 refer to point 1-3, sorry, I won't edit the original text.

@sighingnow
Copy link
Member

Treat etcd-cpp-apiv3-core and etcd-cpp-apiv3 as real either-or alternatives; Either build and install the whole thing or just the "core".

Good point. I will do it that way. But the default async runtime with cpprestsdk would still be the default option as compatibility reasons. There would be only etcd-cpp-api-core and etcd-cpp-api would be a cmake alias if the sync version is required.

But this should actually be not a footnote or a "usage note", but implemented as - again - an EITHER-OR-OPTION. This can be done.

Good suggestions. Will behave in that way.

@sighingnow
Copy link
Member

Added in #208.

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

Successfully merging a pull request may close this issue.

2 participants