Page MenuHomePhabricator

eina: Change EAPI to EINA_API in Eina Library
Needs ReviewPublic

Authored by felipealmeida on Thu, Jun 18, 9:38 AM.

Details

Summary

Separate Eina exporting/importing symbols from other libraries. Other
libraries will also be separated.

This makes it easier to rationalize and fix problems when
importing/exporting symbols in platforms such as Windows which
separates importing from exporting.

Also, I've made DLL standard. If built as static needs to define
EINA_STATIC. This way, users of the shared library will not need
to define anything to use it.

Depends on D11995

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17032
felipealmeida created this revision.Thu, Jun 18, 9:38 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

felipealmeida requested review of this revision.Thu, Jun 18, 9:38 AM

This patch exposed two bugs in evas headers which used EAPI as defined in Eina instead of Evas. This is what I mean by reusing the same symbol is impossible to track bugs.

cauebs added a subscriber: cauebs.Thu, Jun 18, 10:01 AM
vtorri requested changes to this revision.Thu, Jun 18, 10:54 AM

i'm against this patch.

  • Even if EAPI is only needed in the header file, we keep EAPI in source file so that it is easier to see the definition of public symbols
  • it's perfectly possible to define correctly EAPI without modifying all the files

beside that, i'm in the mood of removing static support. On Windows, DLL are good.

This revision now requires changes to proceed.Thu, Jun 18, 10:54 AM
felipealmeida added a comment.EditedThu, Jun 18, 11:00 AM

i'm against this patch.

  • Even if EAPI is only needed in the header file, we keep EAPI in source file so that it is easier to see the definition of public symbols

There are sometime warnings when used in definitions. But I'm not against in keeping it. I can re-add them in .c files in a new patch if you want.

  • it's perfectly possible to define correctly EAPI without modifying all the files

This patch fixed two errors that nobody, EVER, noticed. Because EAPI is being reused. This is the right way to go, because it is simple, elegant, more flexible and easier to understand. Wanting to keep a bad thing just for keeping makes no sense.

beside that, i'm in the mood of removing static support. On Windows, DLL are good.

I'm not exactly adding support, I don't think meson builds static libraries in Windows. But if the user decides to use it and finds a way to compile himself, he is not prohibited. However, I made as DLL as the default so it is easier to everybody.

i'm against this patch.

  • Even if EAPI is only needed in the header file, we keep EAPI in source file so that it is easier to see the definition of public symbols

There are sometime warnings when used in definitions. But I'm not against in keeping it. I can re-add them in .c files in a new patch if you want.

  • it's perfectly possible to define correctly EAPI without modifying all the files

This patch fixed two errors that nobody, EVER, noticed. Because EAPI is being reused. This is the right way to go, because it is simple, elegant, more flexible and easier to understand. Wanting to keep a bad thing just for keeping makes no sense.

never noticed because gcc is doing some magic somewhere, which is not the case with vc++

and in case you don't know, i've supported Visual Studio build system for the EFL during years , with only EAPI and not a macro for each library.

beside that, i'm in the mood of removing static support. On Windows, DLL are good.

I'm not exactly adding support, I don't think meson builds static libraries in Windows.

it can, i've done it in fribidi for example

But if the user decides to use it and finds a way to compile himself, he is not prohibited. However, I made as DLL as the default so it is easier to everybody.

are you sure that your patch support efl-one build ?

felipealmeida added a comment.EditedThu, Jun 18, 11:37 AM

i'm against this patch.

[snip]

This patch fixed two errors that nobody, EVER, noticed. Because EAPI is being reused. This is the right way to go, because it is simple, elegant, more flexible and easier to understand. Wanting to keep a bad thing just for keeping makes no sense.

never noticed because gcc is doing some magic somewhere, which is not the case with vc++

It doesn't need magic. Importing and exporting is all the same. If EAPI were fixedly defined by the build system as attribute ((visibility("default"))) it would work for all libraries in GCC while building dynamic libraries. There wouldn't even be a need to undefine. I think it even didn't before you changed.

and in case you don't know, i've supported Visual Studio build system for the EFL during years , with only EAPI and not a macro for each library.

I imagine your pain! Just because something is possible, doesn't mean it is a good thing.

beside that, i'm in the mood of removing static support. On Windows, DLL are good.

I'm not exactly adding support, I don't think meson builds static libraries in Windows.

it can, i've done it in fribidi for example

Why do you want to drop support then?

But if the user decides to use it and finds a way to compile himself, he is not prohibited. However, I made as DLL as the default so it is easier to everybody.

are you sure that your patch support efl-one build ?

Good question. Probably needs to add -DEINA_BUILD there.

and in case you don't know, i've supported Visual Studio build system for the EFL during years , with only EAPI and not a macro for each library.

I imagine your pain! Just because something is possible, doesn't mean it is a good thing.

when you are alone, doing the less things is the better, especially if it works. And it does not mean it is a bad thing

beside that, i'm in the mood of removing static support. On Windows, DLL are good.

I'm not exactly adding support, I don't think meson builds static libraries in Windows.

it can, i've done it in fribidi for example

Why do you want to drop support then?

i've added static linking in fribidi because fribidi developpers want it. . But current fribidi code does not correctly export symbols. I had to fix that for ewpi. if they want static linking, it's their problem, not mine

static lib may have disadvantages compares to shared lib on Windows (they are not versioned, singletons, rtti, debug symbols, etc...)

also, with dll, we can make starting time faster with delay loading

are you sure that your patch support efl-one build ?

Good question. Probably needs to add -DEINA_BUILD there.

and in case you don't know, i've supported Visual Studio build system for the EFL during years , with only EAPI and not a macro for each library.

I imagine your pain! Just because something is possible, doesn't mean it is a good thing.

when you are alone, doing the less things is the better, especially if it works. And it does not mean it is a bad thing

Indeed. But after this change, other changes become way easier. Using EAPI is like walking on eggs. It is extremely fragile. And errors are far from easier to track. Which adds up in maintenance time. After this change, which not small, but trivial (just search and replace), things gets easier to understand, track errors and fix them. We still have in our branch (expertise solutions's) hard-to-track errors of multiple definitions because of EAPI.

i've added static linking in fribidi because fribidi developpers want it. . But current fribidi code does not correctly export symbols. I had to fix that for ewpi. if they want static linking, it's their problem, not mine

static lib may have disadvantages compares to shared lib on Windows (they are not versioned, singletons, rtti, debug symbols, etc...)

also, with dll, we can make starting time faster with delay loading

I see.

Readded EINA_API to C files as recommeded by vtorri

Remove modification to EINA_DEPRECATED that is not important for this patch

felipealmeida edited the summary of this revision. (Show Details)

Fix efl-one compilation by depending on D11995

now look at what i have written :

diff --git a/meson.build b/meson.build
index fb7ace8cbd..e835dfa2e4 100644
--- a/meson.build
+++ b/meson.build
@@ -429,6 +429,13 @@ foreach package : subprojects
       '-DNEED_RUN_IN_TREE=1',
       '-DEFL_BUILD=1',
     ]
+    if sys_windows == true
+      if get_option('efl-one') == true
+        package_c_args += '-DEFL_ONE_BUILD'
+      else
+        package_c_args += '-DEFL_@0@_BUILD'.format(package_name.to_upper())
+      endif
+    endif
     if (package[3])
       subdir(join_paths(local_lib, package_name))
       set_variable(package_name + '_eo_files', pub_eo_files)
diff --git a/src/lib/eina/eina_types.h b/src/lib/eina/eina_types.h
index b93e99f483..fd57b828da 100644
--- a/src/lib/eina/eina_types.h
+++ b/src/lib/eina/eina_types.h
@@ -38,12 +38,8 @@
 #endif
 
 #ifdef _WIN32
-# ifdef EFL_BUILD
-#  ifdef DLL_EXPORT
-#   define EAPI __declspec(dllexport)
-#  else
-#   define EAPI
-#  endif
+# if defined(EFL_ONE_BUILD) || defined(EFL_EINA_BUILD)
+#  define EAPI __declspec(dllexport)
 # else
 #  define EAPI __declspec(dllimport)
 # endif
diff --git a/src/lib/eo/Eo.h b/src/lib/eo/Eo.h
index 0ebbd53e9c..cd8460372e 100644
--- a/src/lib/eo/Eo.h
+++ b/src/lib/eo/Eo.h
@@ -11,12 +11,8 @@
 #define EOLIAN
 
 #ifdef _WIN32
-# ifdef EFL_BUILD
-#  ifdef DLL_EXPORT
-#   define EAPI __declspec(dllexport)
-#  else
-#   define EAPI
-#  endif
+# if defined(EFL_ONE_BUILD) || defined(EFL_EO_BUILD)
+#  define EAPI __declspec(dllexport)
 # else
 #  define EAPI __declspec(dllimport)
 # endif
diff --git a/src/lib/eolian/Eolian.h b/src/lib/eolian/Eolian.h
index 3d3db0ee7b..c0e4810651 100644
--- a/src/lib/eolian/Eolian.h
+++ b/src/lib/eolian/Eolian.h
@@ -6,12 +6,8 @@
 #endif
 
 #ifdef _WIN32
-# ifdef EFL_BUILD
-#  ifdef DLL_EXPORT
-#   define EAPI __declspec(dllexport)
-#  else
-#   define EAPI
-#  endif
+# if defined(EFL_ONE_BUILD) || defined(EFL_EOLIAN_BUILD)
+#  define EAPI __declspec(dllexport)
 # else
 #  define EAPI __declspec(dllimport)
 # endif
diff --git a/src/lib/evil/evil_private.h b/src/lib/evil/evil_private.h
index d87ac75423..94771c3b90 100644
--- a/src/lib/evil/evil_private.h
+++ b/src/lib/evil/evil_private.h
@@ -30,12 +30,8 @@ extern "C" {
 # undef EAPI
 #endif
 
-#ifdef EFL_BUILD
-# ifdef DLL_EXPORT
-#  define EAPI __declspec(dllexport)
-# else
-#  define EAPI
-# endif
+#if defined(EFL_ONE_BUILD) || defined(EFL_EVIL_BUILD)
+# define EAPI __declspec(dllexport)
 #else
 # define EAPI __declspec(dllimport)
 #endif

5 modified files, 4 libraries (evil, eina, eolian, eo), patch of 95 lines. I've verified that for each of these 4 libs, EAPI is dllexport, and for eolian_gen, it's dllimport

i've removed the possibility to have static linking but if other devs don't want that, i'm ok with that

Independend from if this is a good idea or not, things that are not clear to me:

  • How to do that in eolian ? Every eolian API is defined with EOAPI, which is defined to be EAPI & EAPI_WEAK with this approach you either need to patch eolian to take the correct preprocessor directives (then eolian needs to guess in which lib it is), or you need to redefine EOAPI which is then reintroducing the thing you are trying to solve
  • How to handle EAPI's in modules ?

Sidenote: The font API in evas should not have EAPI, it is declared private, and not used anywhere outside evas. The second fix in evas can simply be fixed by using the correct header... no need to declare that API there.

walac accepted this revision.Fri, Jun 19, 5:31 AM

That patch seems a step towards in the right direction IMHO. Things get tricky when EFL try to use each other since, in some places, EAPI must be __declspec(dllimport) and in others __declspec(dllexport), which leads walking on thin ice by playing around with the preprocessor. I understand that each library should define its XXX_API macro. The static library support is a win, but not the main point of the patch.

@walac your prefer this patch which modifies 143 files, ONLY for eina, to my patch, which modifies 5 files for 4 libraries, with the same result ?? that's crazy

do you realize that, with this kind of changes, you are going to change almost all the files of the EFL ?

walac added a comment.Fri, Jun 19, 7:39 AM

do you realize that, with this kind of changes, you are going to change almost all the files of the EFL ?

The point is that name collision is a known issue with the C program language, and things get even worse when macros get to the playground. That is why I prefer the longer patch that isolates namespaces for each library than the shorter one. The fundamental problem is naming conflicting and if we don't fix it there will always be corner cases that will punch us on the face sooner or later.

with my patch,there can't be any, by the very definition of EFL_XXX_BUILD that i am doing

Independend from if this is a good idea or not, things that are not clear to me:

  • How to do that in eolian ? Every eolian API is defined with EOAPI, which is defined to be EAPI & EAPI_WEAK with this approach you either need to patch eolian to take the correct preprocessor directives (then eolian needs to guess in which lib it is), or you need to redefine EOAPI which is then reintroducing the thing you are trying to solve
  • How to handle EAPI's in modules ?

I already have a patch for Eolian which gets a macro name. This way third-party users can use their own macro as well. Which is necessary for this to work correctly for users to use inheritance.

Sidenote: The font API in evas should not have EAPI, it is declared private, and not used anywhere outside evas. The second fix in evas can simply be fixed by using the correct header... no need to declare that API there.

felipealmeida added a comment.EditedFri, Jun 19, 11:07 AM

[snip]

Sidenote: The font API in evas should not have EAPI, it is declared private, and not used anywhere outside evas. The second fix in evas can simply be fixed by using the correct header... no need to declare that API there.

Yeah sure, there a multiple ways to fix it. But nobody found the error because EAPI is a mess. If it weren't used EAPI in the first place, the error was already caught and fixed the right way.

felipealmeida added a comment.EditedFri, Jun 19, 11:12 AM

with my patch,there can't be any, by the very definition of EFL_XXX_BUILD that i am doing

Except where there is, like the errors in evas. EAPI is a mess, which creates very strict conventions for that not fail. Let's just list some of these strictness from the top of my head:

  • Have to have #undef/#redef in every entry-point header (like xxxx_private.h too, because they can be included by modules).
  • Every C file must include its own header after all the others, or they will get undefined EAPI's or wrong EAPI definition (nothing more sweet than handling header-ordering), but it will not work to include twice because of header guards. This ordering must take in account also when other libraries include other libraries.
  • Every non-entry-point header must assume EAPI was defined before it and can't include other library's headers otherwise EAPI definitions get broken. Of course people will include it, because it makes sense. But they will just break compilation on Windows.

This is all _just_ from the top of my head. There are obviously more and more complex cases.

And worse of all, Linux developers can't EVER find the problem because it will never fail for them. Which means that this strictness is a maintenance nightmare.

Since when we measure quality by lines of code?

felipealmeida added a comment.EditedFri, Jun 19, 11:29 AM

now look at what i have written :

[snip code]

5 modified files, 4 libraries (evil, eina, eolian, eo), patch of 95 lines. I've verified that for each of these 4 libs, EAPI is dllexport, and for eolian_gen, it's dllimport

Why should eolian_gen be dllimport? What happens if I #include a .eo.h directly ? Like C++ bindings do? Do I get dllimport even if I implement the class in my own library? Do I get a warning if that happens? Or that will just work in Linux and break horribly in Windows?

Maybe you think I don't know how this works? I do know, but it is horrible! It is small, and horrible!

I remember the last time we tried to be super smart with C preprocessor. Using macros for APIs. I'm glad I killed that idea then! And I've been doing preprocessor meta-programming for almost a decade. Even have a Javabind library for C++ to create C++ classes that interact with JNI directly in C++ through preprocessor metaprogramming which I wrote 8 years ago (https://github.com/felipealmeida/javabind/blob/master/demo/native/native.cpp). But in this case, reusing EAPI is just a very very bad workaround for something that worked fine in Linux, but must be replaced for something sane for multiplatform, if we consider Windows an important platform.

i've removed the possibility to have static linking but if other devs don't want that, i'm ok with that

now look at what i have written :

[snip code]

5 modified files, 4 libraries (evil, eina, eolian, eo), patch of 95 lines. I've verified that for each of these 4 libs, EAPI is dllexport, and for eolian_gen, it's dllimport

Why should eolian_gen be dllimport?

EAPI of evil, eina and eolian must be dllimport when evil_private.h, Eina.h and Eolian.h are included in eolian_gen source files

What happens if I #include a .eo.h directly ? Like C++ bindings do?

EAPI willfollow the same rule

Do I get dllimport even if I implement the class in my own library? Do I get a warning if that happens? Or that will just work in Linux and break horribly in Windows?

Maybe you think I don't know how this works? I do know, but it is horrible! It is small, and horrible!

it's small and it works, and if there are problems, we will solve them as they come

if not, good luck in convincing dev to modify all the source files for the Visual Studio port.

felipealmeida added a comment.EditedFri, Jun 19, 12:08 PM

now look at what i have written :

[snip code]

5 modified files, 4 libraries (evil, eina, eolian, eo), patch of 95 lines. I've verified that for each of these 4 libs, EAPI is dllexport, and for eolian_gen, it's dllimport

Why should eolian_gen be dllimport?

EAPI of evil, eina and eolian must be dllimport when evil_private.h, Eina.h and Eolian.h are included in eolian_gen source files

What happens if I #include a .eo.h directly ? Like C++ bindings do?

EAPI willfollow the same rule

So it will not work and break windows without a single warning? That's what I mean by being bad. How can you not see that breaking without warning is a bad thing?

Do I get dllimport even if I implement the class in my own library? Do I get a warning if that happens? Or that will just work in Linux and break horribly in Windows?

Maybe you think I don't know how this works? I do know, but it is horrible! It is small, and horrible!

it's small and it works, and if there are problems, we will solve them as they come

It doesn't. As shown above.

if not, good luck in convincing dev to modify all the source files for the Visual Studio port.

You're the only one that voice against it until now. I'd hope you would prefer Windows to be easier to maintain. But you seem to be more concerned that I touch the files in EFL.

walac added a comment.Mon, Jun 22, 7:26 AM

@walac your prefer this patch which modifies 143 files, ONLY for eina, to my patch, which modifies 5 files for 4 libraries, with the same result ?? that's crazy

My understanding is, for example, if I am in a .c file in, say, ecore, and include both eina.h and ecore.h, I would get an error with mutiple definitions of EAPI, with one definition being __declspec(dllimport) and the other __declspec(dllexport). Is that right or am I missing something?