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

CMake support #7

Closed
FlorianRhiem opened this issue Oct 10, 2018 · 12 comments
Closed

CMake support #7

FlorianRhiem opened this issue Oct 10, 2018 · 12 comments
Labels
Enhancement New feature or request

Comments

@FlorianRhiem
Copy link
Contributor

Hello and thank you for your work!
Currently there are no files for any build systems aside from the shell script calling g++ that you provide with the examples. I think it'd be nice to have a CMakeLists.txt along with the source code to build and install the library, run the tests, etc. Would that be alright for you, or is the lack of something like that based on the project's zero dependency principle? If you simply don't use CMake yourself, then I could prepare a pull request during the weekend.

@lganzzzo
Copy link
Member

Hello @FlorianRhiem , thank you for your interest and readiness to help!
All infrastructural pull-requests that make devs life easier are definitely very welcome!

@FlorianRhiem
Copy link
Contributor Author

Good to hear! I just started setting something up and noticed that the way you include your files, you require your users to clone the repo or download the source as oatpp. Any other directory name will lead to failed includes. GitHub for example will give users an oatpp-master directory if they download the source as zip file, so this would need to be manually renamed.
Was the omission of that directory intentional? If not, I would suggest moving all files that are expected to exist in an oatpp directory to that directory. The big downside of this, obviously is that it isn't compatible with the current examples.

@lganzzzo
Copy link
Member

The way it is right now is intentional.

Current schema of includes looks as follows:

Include paths are treated as "<module>/path/to/file"
Currently oatpp has multiple modules (git submodules) to be included on demand.

  • oatpp (basic framework) - no dependency
  • oatpp-swagger (swagger-ui) - no dependency except oatpp itself
  • oatpp-consul (Consul client - see consul.io) - no dependency except oatpp itself
  • oatpp-libressl (TLS based on libressl) - libressl required

To include new submodule:
in lib folder
$ git submodule add <url>

The idea to ship oatpp as full source-code (as git submodules) is to enable user:

  • To easily debug everything
  • Make process of contribution easy and natural (you found the bug you fixed it, you pushed the PR)

Such structure of the project is not final, and in some sens is an experiment.

@FlorianRhiem
Copy link
Contributor Author

Alright, I see the benefit in submodule use. I'll just use the parent as include directory for building then.


The three parser::json::mapping tests fail with uncaught exceptions. Are these still a work-in-progress or am I missing a required part of test setup?

#include "oatpp/test/parser/json/mapping/DeserializerTest.hpp"
#include "oatpp/test/parser/json/mapping/DTOMapperPerfTest.hpp"
#include "oatpp/test/parser/json/mapping/DTOMapperTest.hpp"

int main() {
  OATPP_RUN_TEST(oatpp::test::parser::json::mapping::DeserializerTest);
  OATPP_RUN_TEST(oatpp::test::parser::json::mapping::DTOMapperPerfTest);
  OATPP_RUN_TEST(oatpp::test::parser::json::mapping::DTOMapperTest);
  return 0;
}

The deserializer test fails with:

libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: ASSERT[FAILED]:obj1->strF

Both DTOMapper tests result in:

libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: [oatpp::kafka::protocol::mapping::Serializer::writeField]: Unknown data type

@lganzzzo
Copy link
Member

Tests currently are not in the actual state. I plan to get back to them some time later.

You may just comment-out them for now

@lganzzzo
Copy link
Member

Hey @FlorianRhiem

I just checked these tests, and they should work just fine.
I copy-pasted as-is your code and it worked.

Can you please add this and check ones again:

#include "oatpp/test/parser/json/mapping/DeserializerTest.hpp"
#include "oatpp/test/parser/json/mapping/DTOMapperPerfTest.hpp"
#include "oatpp/test/parser/json/mapping/DTOMapperTest.hpp"

#include "oatpp/core/concurrency/SpinLock.hpp"

#include <iostream>

class Logger : public oatpp::base::Logger {
private:
  oatpp::concurrency::SpinLock::Atom m_atom;
public:
  
  Logger() : m_atom(false) {}
  
  void log(v_int32 priority, const std::string& tag, const std::string& message) override {
    oatpp::concurrency::SpinLock lock(m_atom);
    std::cout << tag << ":" << message << "\n";
  }
  
};

int main() {
  
  oatpp::base::Environment::init();
  oatpp::base::Environment::setLogger(new Logger()); // set Logger to print logs
  
  OATPP_RUN_TEST(oatpp::test::parser::json::mapping::DeserializerTest);
  OATPP_RUN_TEST(oatpp::test::parser::json::mapping::DTOMapperPerfTest);
  OATPP_RUN_TEST(oatpp::test::parser::json::mapping::DTOMapperTest);
  
  oatpp::base::Environment::setLogger(nullptr); // free Logger
  
  std::cout << "\nEnvironment:\n";
  std::cout << "objectsCount = " << oatpp::base::Environment::getObjectsCount() << "\n";
  std::cout << "objectsCreated = " << oatpp::base::Environment::getObjectsCreated() << "\n\n";
  
  oatpp::base::Environment::destroy();
  
  return 0;
}

@FlorianRhiem
Copy link
Contributor Author

Thank you for the code example and verifying that it should work. It still crashes for me (macOS 10.12 using AppleClang 9) as obj1->strF is NULL, but I will try to figure out what's causing this so that I know it's not an issue with the build itself.

@lganzzzo
Copy link
Member

Strange, because I also tested on MacOS 10.13 ...
Does code from the example-projects work fine on your machine?

If you manage to find out the reason, that's will be great.

Please let me know if you have any questions.

@FlorianRhiem
Copy link
Contributor Author

I've found the cause of the issue: In Deserializer::readValue, typeName is "String" yet fails the equality comparison with oatpp::data::mapping::type::__class::String::CLASS_NAME.

You are using constexpr static const char* const for typenames and rely on the pointers being equal to avoid a costly string comparison. This works well when oat++ is built as a static library, but fails when it is built as shared library. If the typename is turned non-constexpr and its definition is moved to core/data/mapping/type/Primitive.cpp, then it seems to work as the generation of a symbol is enforced and that symbol will have the same value.

@FlorianRhiem
Copy link
Contributor Author

To clarify: With "seems to work" I mean that it works as long as only the (moved) String CLASS_NAME is used and fails as soon as the test checks for deserializing an int32, as would be expected.

@lganzzzo
Copy link
Member

I see,
this is something I should think over in order to deliver suitable solution.

Thanks a lot, this is great help and great issue:)

@lganzzzo
Copy link
Member

Hey @FlorianRhiem
Thank you for PR!
I've seen it. I have to think this issue over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants