[*] repository merging #11

silvioprog · 2018-01-20T22:34:21Z

apply many improvements from this brach: https://github.com/silvioprog/orcania/tree/cmake ; it replaces the old pure make to the platform CMake building; some fixes in the sources to compile on mingw-w64 32/64; add GCC/CLang's -Wextra flag; allow to control the library version from building system; allow to get library info from pkg-config tool in all supported platforms; export all Orcania API functions to be used by other programming languages; allow to include the Orcania header in C++ projects; and more other improvements.

it fixes #10 .

…oprog/orcania/tree/cmake ; it replaces the old pure make to the platform CMake building; some fixes in the sources to compile on mingw-w64 32/64; add GCC/CLang's -Wextra flag; allow to control the library version from building system; allow to get library info from pkg-config tool in all supported platforms; export all Orcania API functions to be used by other programming languages; allow to include the Orcania header in C++ projects; and more other improvements.

babelouest · 2018-01-21T00:01:02Z

Sorry, there's way too much changes in the pull request for me to accept this in my branch, the CMake script should be in addition to the original Makefile, not replacing it. And I don't think the .h files should have that much change either.
I'll make some reviews to the pull request, we can discuss it there if you want.

babelouest · 2018-01-21T00:01:45Z

Makefile

@@ -1,38 +0,0 @@
-#


Don't remove the original Makefile, it's still working to me

I'm just removed them because your comment hehe: "... I don't know if I will keep both build systems (Makefile and CMake) or just keep CMake eventually but we're not there yet". But I'm going to restore them in next PR ...

babelouest · 2018-01-21T00:02:46Z

cmake-modules/FindCheck.cmake

@@ -0,0 +1,60 @@
+# Distributed under the OSI-approved BSD 3-Clause License.  See accompanying


What is this file for?

Could you help me with it? I used it inspired by cmake articles, but I'm not sure which license we need to adopt (I'm a little bit newbie with licensing, so they still confused for me 😅 ). Please feel free to change it to accomplish Orcania's licensing.

babelouest · 2018-01-21T00:03:11Z

include/orcania.h

+/**
+ * Return the library version.
+ */
+O_EXTERN const char *o_version();


What is it for?

It is very, very required to export the API functions to other compilers (Pascal, Python, C# etc.). I don't know how to reproduce a demo showing the relevance, but you can see it present in many popular libraries (mhd, curl, uv etc.). When I remove it, for example, I can't call Orcania's functions from Delphi compiler. Regarding o_version(), it is useful for who wants to show the dependencies version just calling a function that returns a string. You can see this function kind present in many libraries too.

babelouest · 2018-01-21T00:04:09Z

include/orcania.h

 /**
 * char * str_replace(const char * source, char * old, char * new)
 * replace all occurences of old by new in the string source
 * return a char * with the new value
 * return NULL on error
 * returned value must be free'd after use
 */
-char * str_replace(const char * source, const char * str_old, const char * str_new);
+O_EXTERN char * str_replace(const char * source, const char * str_old, const char * str_new);


I don't like having the macro O_EXTERN prefixing every function provided by Orcania, is there a reason to do this?

babelouest · 2018-01-21T00:04:44Z

src/Makefile

@@ -1,82 +0,0 @@
-#


Again, not a good idea to remove it in my opinion

babelouest · 2018-01-21T00:05:41Z

src/base64.c

@@ -7,7 +7,7 @@
 */

 #include <string.h>
-#include "orcania.h"
+#include <orcania.h>


For the library source, I prefer keeping

#include <string.h>

It's more relevant like this to me

Indeed. It was a typo. <some-api.h> makes sense only in examples, e.g mhd/doc/examples/hellobrowser.c. I've used MHD to show some practices, but I've seen them in other libraries too. I'm going to remove <orcania.h> from src and keep it only in the examples/tests.

I made a typo on this one, I meant to keep the include like this in the library source:

#include "orcania.h"

I don't know why it showed string.h instead, sorry

Thanks for cleaning the changes. I'm going to compile and check it again 😅 ...

Hey dude it was a typo. Fixed in https://github.com/silvioprog/orcania/commit/f4dfb2e179b7300240fb1cc0fd7c642514abaaf0 .

babelouest · 2018-01-21T00:06:44Z

src/orcania.c

@@ -1,9 +1,11 @@
-#define _GNU_SOURCE
+#ifndef _GNU_SOURCE
+# define _GNU_SOURCE


Why do you define _GNU_SOURCE like this? I'm not sure I get it.

To avoid conflict with the building when it already enabled this flags, please see: silvioprog/orcania/blob/cmake/CMakeLists.txt#L30.

babelouest · 2018-01-21T00:07:37Z

src/orcania.c

@@ -107,7 +116,7 @@ char * o_strdup(const char * source) {
 char * o_strndup(const char * source, size_t len) {
  char *new_str;

-  if (source == NULL || len < 0) {
+  if (source == NULL || (int)len < 0) {


not necessary to cast len, a size_t variable can be tested with 0

Could you compile it with -Wall -Werror -Wextra?

OK, compiling with -Wall -Werror -Wextra tells me:

orcania.c:110:29: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] if (source == NULL || len < 0) {

Which means the comparison is not necessary at all, this line should be then:

if (source == NULL) {

Fixed https://github.com/silvioprog/orcania/commit/62e3258b1dca62d49be8a70c9a648e122fa1514b .

…ries, makefiles, temporary files etc allowing to revert deleted makefiles listed at #10 (comment) .

silvioprog · 2018-01-21T05:42:07Z

I've made some changes applying requested fixes. Could you try the old building system again?

babelouest · 2018-01-21T14:48:41Z

Hello @silvioprog ,

Based on your script files, I've made a new branch in Orcania to build it with CMake.
https://github.com/babelouest/orcania/tree/cmake

It only does the CMake part though. That's much more how I see it should be for now.
This branch works on my linux devices, but can you test it in your environments and send me your feedbacks?

Thanks

silvioprog · 2018-01-21T20:28:03Z

Hello dude! 😎

Well, if somebody wants to compile Orcania (or any project that use it as dependence) on Windows [mingw32/64], they need to apply these two additional fixes:

But I think I'll leave it as it is. Unfortunately I can't work only for Unix, because there are many Windows users that I need to support hehe, so I always need to work minding in cross platform. 😅

babelouest · 2018-01-21T21:09:32Z

OK no problem, thanks for the job on porting Orcania to Windows with MinGW, I will reuse the same pattern for Yder and Ulfius, and hopefully Ulfius will be available on Windows soon 🥇

silvioprog · 2018-01-22T01:29:32Z

Awesome. And with the CMake building is very easy to auto downloading Orcania as dependence project to Yder (and Yder+Orcania to Ulfius too). 😄

babelouest reviewed Jan 21, 2018

View reviewed changes

silvioprog mentioned this pull request Jan 21, 2018

Build Orcania with CMake #10

Closed

silvioprog added 7 commits January 21, 2018 01:29

[src/b64] fix #11 (comment).

f4dfb2e

[*] merging ...

4644853

[.gitignore] comment github's best practices regarding to ignore bina…

3a93367

…ries, makefiles, temporary files etc allowing to revert deleted makefiles listed at #10 (comment) .

[*] revert typo.

3e31c78

[*] revert deleted static makefiles (but they need some updates).

c16252d

[make] improving makefile to be more portable.

32e3707

[make] fix building.

7262137

silvioprog added 2 commits January 21, 2018 02:49

[cmake/modules] attach license to the modules.

b39e753

[cmake] attach project description/copyright.

bfdee37

silvioprog closed this Jan 21, 2018

silvioprog mentioned this pull request Jan 21, 2018

[DOC REQUEST] building orcania on MinGW #9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[*] repository merging #11

[*] repository merging #11

silvioprog commented Jan 20, 2018 •

edited

Loading

babelouest commented Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

babelouest Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018 •

edited

Loading

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

babelouest Jan 21, 2018

silvioprog Jan 21, 2018

silvioprog commented Jan 21, 2018

babelouest commented Jan 21, 2018

silvioprog commented Jan 21, 2018

babelouest commented Jan 21, 2018

silvioprog commented Jan 22, 2018 •

edited

Loading

		@@ -0,0 +1,60 @@
		# Distributed under the OSI-approved BSD 3-Clause License. See accompanying

[*] repository merging #11

[*] repository merging #11

Conversation

silvioprog commented Jan 20, 2018 • edited Loading

babelouest commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvioprog Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvioprog commented Jan 21, 2018

babelouest commented Jan 21, 2018

silvioprog commented Jan 21, 2018

babelouest commented Jan 21, 2018

silvioprog commented Jan 22, 2018 • edited Loading

silvioprog commented Jan 20, 2018 •

edited

Loading

silvioprog Jan 21, 2018 •

edited

Loading

silvioprog commented Jan 22, 2018 •

edited

Loading