Page MenuHomePhabricator

Refactor EFL_MAIN* and efl_exit* from elm into efl
Closed, ResolvedPublic

Description

We need to not have to include elementary for command line apps

jpeg added a comment.Oct 24 2017, 8:49 PM

Efl_Ui.h ?

This is an open question, see the ml for more debate. But we could have Efl_Core and Efl_Ui (maybe Efl_Net is good on its own).

If we want to make it easy for folk to find them when contributing then I'd recommend not having mainloop looking tools in the ui area...

jpeg added a comment.Oct 25 2017, 3:51 AM

Not sure what you have in mind... The UI needs the mainloop?

According to the examples I have seen we're using efl_main and efl_exit for any EFL app.
But not all efl apps use a UI...

Am I wrong about that?

either use efl.loop or efl.core... but efl.ui is not an option.

Hum, I will look into moving the EFL_MAIN macro into ecore headers.

Ok, here is the obvious problem. This macro does the initialization/shutdown of elementary. I could pull a trick in Ecore.h (define a macro that only init up to Ecore) and Elementary.h (define that same macro that init up to Elementary) so that Elementary.h version always win. Technically I should find a way for every single header to get there init inserted in the macro as you include the header.

It does seem overkill to me and quite complex. If we had one header for Efl new interface to include (let's say Efl_Neo.h), and we don't make a difference between all of the above library, it would be quite easier. Any strong argument on why not to merge all of it ?

jpeg added a comment.Oct 25 2017, 7:50 PM

Or just define EFL_MAIN_CORE that only initializes up to ecore / net?

Why do we want developers to have to make these decisions? This looks like unnecessary complexity to work around our library layout...

jpeg added a comment.Oct 26 2017, 12:23 AM

Okay so now I'm getting confused. EFL_MAIN() (or ELM_MAIN()) is there to be the most convenient to the most people who use EFL, i.e. people who make UI applications.
The macro does not prevent anyone from doing the init manually in order to save the loading of Elementary...

I was not aware that we asserted that apps made with EFL should be GUI apps - the examples and our repos have had many command line apps over the years...

What I think we are aiming for is a way to include a minimal header (ideally top level) and use standard looking entry points for all of the apps you can make with this library. Currently I have to include Elementary.h to use these entry points which seems counter intuitive. It also seems an overhead for developers to use two different entrypoints/methods depending on the type of app they are building.

jpeg added a comment.Oct 26 2017, 1:13 AM

So... I am still confused. I can't understand what you want, beyond a single "Efl_Next.h" (or whatever the name). That means it would include everything from Eina.h to Elementary.h. In C the include is mostly harmless (note: not true in C++).

As for the macro now. Removing the quicklaunch / elm_init from EFL_INIT() means app devs will have to manually init & shutdown elementary.
Keeping elm_init() in the macro has overhead for cmd line only apps. If the choice isn't left to the developer, what then?

I am honestly confused as I can't get to what you have in mind.

I don't have a solution in mind.
I am just looking for something completely straight forward.
Actually I don't care if elm is included behind the scenes - it simply should not be on the developer to see that strangeness.

zmike added a comment.Oct 26 2017, 7:19 AM

I know we've lost a number of active efl developers in the past because we refused to provide facilities for creating non-gui applications using efl or even building efl without gui libs. We might consider making this easier in the future.

There is a lot going on here !

@jpeg I like the Efl_Next name. I think I am going to roll with that one, if we decide to have only one package.

@ajwillia.ms this would solve one problem. pkg-config of only one package and only one header to include. Simplify thinks, a lot, I think.

@zmike and @ajwilia.ms, so here is the thing. We can go with increased complexity and have a EFL_CORE_INIT() for just the bare minimum (aka just ecore) along with an EFL_UI_INIT() (for everything up to elementary), but in that case we increase the complexity (As it requires also to go with two package an Efl_Core and another Efl_Ui). We have to choose here, on where we are going. I am fine with both solution and kind of leaning towards more option being better, but the tradeof being complexity, I am not sure it is worse it.

jpeg added a comment.Oct 26 2017, 6:50 PM
In T6262#102989, @zmike wrote:

I know we've lost a number of active efl developers in the past because we refused to provide facilities for creating non-gui applications using efl or even building efl without gui libs. We might consider making this easier in the future.

I can remember such a case but the problem was much wider than providing a simple non-UI init facility: basically all of EFL had to be able to compile without any graphics deps (X/WL, but also no jpeg/png/...).
I think right now this is not a priority BUT we should keep in mind as we move towards actually merging the libs (with a new build system like meson maybe?).

Basically I think a single header is good (not "Elementary.h"), EFL_MAIN is fine as it is, but it doesn't prevent us from adding more convenience for non-UI apps (EFL_CORE_MAIN and Efl_Core.h). The current approach for our convenience is "what can do more can do less".

Note: In another ticket (T5938) I mentioned that we need a "API target" API (like efl_api_set(1, 18) for instance) because it is the only way we can implement bug/behavior compatibility.
This needs to be either super well documented (i.e. appear in ALL eo examples) or be necessary as an argument of EFL_MAIN: EFL_MAIN(1, 18) (I don't like this idea).
We already have efl_build_version_set and _EFL_APP_VERSION_SET() but this only works to detect which EFL version a binary was built against, not which one it was coded for & tested with.

My grain of salt:

Separating the Efl_Core/Efl_Ui (and even Efl_Net) seem very fine to me. I strongly dislike the single header solution.
I prefer the implementation of our macros/stuff to be slightly complicated as long as the separation of core/ui is preserved, but it does not seem to be that weird to do.
What would be wrong with:

Efl_Core.h
/* Define if undefined */
#ifndef EFL_MAIN
# #define EFL_MAIN() core_specific()
#endif
Efl_Ui.h
/* Override the definition */
#ifdef EFL_MAIN
# undef EFL_MAIN
#endif
#define EFL_MAIN() ui_specific()

so the user does:

#include <Efl_Ui.h>

EFL_MAIN() /* <---- Ui stuff */
#include <Efl_Core.h>

EFL_MAIN() /* <---- Core stuff */
#include <Efl_Ui.h>
#include <Efl_Core.h>

EFL_MAIN() /* <---- Ui stuff */
#include <Efl_Core.h>
#include <Efl_Ui.h>

EFL_MAIN() /* <---- Ui stuff */

?

I was thinking more about the impact of what happen if you do Efl_Net.h ? Or with efreet and eldbus ? Should dbus be part of core, or just of UI ? What about Efl_Net.h ? Should we do the same trick or not ? It does start to increase complexity for each header. That was were I was going.

zmike added a comment.Oct 27 2017, 4:22 PM

dbus and networking support have no relation to UI layers conceptually or functionally.

We do accept uri as filename when setting them on image for example. This require network.

For dbus, things like application menu depends on it.

So UI layers do depends on those two and if there is a priority order to make it would be core < net < ui, but that doesn't answer my question, do we care about dbus to have its own dedicated EFL_MAIN or not ? Does it make sense to have an application that is dbus only or not ? Would such an application be just core + dbus or most likely core + net + dbus ? That will be dictating the answer to do we need an EFL_MAIN macro in EFL_Dbus.h.

zmike added a comment.Oct 27 2017, 5:31 PM

I've written applications which use only dbus with no UI.

jayji added a comment.Oct 28 2017, 7:16 AM

Okay, now I better understand where you were going. How about the rationale: if you use a module, it is that you will use it (#include in C, import in python, something else in some other language). So if you do the following in C:

#include <Efl_Net.h>
#include <Efl_Core.h>

you will need both Net and Core. I believe Efl_Net.h will include Efl_Core.h under the hood, even if we had:

#include <Efl_Net.h>

would be functionnaly equivalent to the first code excerpt (we require both Net and Core).

What if every "module" (Core, Net, Ui, Dbus, anything) would advertize itself upon inclusion, such as:

lang=c name=Efl_Net.h
#define __EFL_NET_IS_REQUIRED__
/* ... */
#include "Efl_General.h" /* Another header */
lang=c name=Efl_Core.h
#define __EFL_CORE_IS_REQUIRED__
/* ... */
#include "Efl_General.h" /* Another header */
lang=c name=Efl_Dbus.h
#define __EFL_DBUS_IS_REQUIRED__
/* ... */
#include "Efl_General.h" /* Another header */

The header guards would even be sufficient.
With the wildcard Efl_General.h (first name that occured to me) that could provide things such as:

lang=c name=Efl_General.h
#define __EFL_MAIN_CONSTRUTORS__ \
   NET(efl_net_stuff() \
   UI(efl_ui_stuff()) \
   DBUS(efl_dbus_stuff()
 
#ifdef __EFL_NET_IS_REQUIRED__
# define NET(...) __VA_ARGS__
#else
# define NET(...)
#endif

/* Same for other things */

define EFL_MAIN() \
  do_stuff(); \
  __EFL_MAIN_CONSTRUTORS__ \
  do_other_stuff()

It seems to me that with something like this, we can easily handle an arbitrary number of modules (Core/Net/Ui/Dbus/Whatever) without having complex maintainance, while being very flexible.This seems to also make room for specific tricks.
What do you think?

i really dislike this kind of approach... IMO efl_main should be in the core lib and headers.

The UI header may include more or less stuff (but I'd provide an Efl.h that does include all, leaving Efl/Ui.h with only UI stuff), but it shouldn't replace the symbol that's used... or require some defines to select what's to include or not... if you do that, then how that's better than simply let the user manually #include X?

As for linking and initialization, that's a different issue. IMO linking should be left to project managers (ie: autoconf, cmake... xcode) that will select what's to compile in.

then for library initialization, I always found explicit eina_init(); ecore_init() ... kinda of weird. We should decide if we're auto-init library if they are linked in or auto-init during library entry point -- easy for elm (ie: auto-init on window creation), easy for efl.net (during dialer/server creation)... not so easy for things like eina. Thus my preference is to auto-init once linked.

@barbieri : The problem is that Efl.h is already taken and not everything was protected by the beta define which is why I think we need a new header to merge them all. I like Efl_Next.h for that purpose.

As for auto-init library, is the link time auto init something supported widely and reliably ? I am thinking there are historical reason why we do that here, not sure if that still apply (Windows mobile and Symbian didn't support that back in the time).

I am not a fan of auto-init during library entry point at all. Also we to start up the main loop in the main, which is what EFL_MAIN does (This allow us to pick efl_main symbol from outside the binary and use it directly for registering quicklaunch and friends).

If link time init is not reliable for us, we should go with @jayji proposal.

If we intend to provide Efl_Core.h, Efl_Net.h and Efl_Ui.h (as mentioned on another thread) then we probably don't need a single Efl(_Next).h - including what you want to use makes sense - if we can avoid having to define a different entry point and macro based on the highest level entry point.

i.e. cmdline includes Efl_Core.h and has EFL_MAIN - then I want to add Net so adding Efl_Net.h is the only change required?

That seems neat enough and solves this complex naming issue - all 3 of them can include whatever hidden common .h to provide the necessary lookups and hooks...

cedric closed this task as Resolved.Nov 9 2017, 4:09 PM

Fixed by bd83a76393a1e5ceb7f725916a7eb9d41305f788 . EFL_MAIN is now available with Efl_Core.h

ajwillia.ms reopened this task as Open.Nov 14 2017, 7:20 AM

I feel the need to reopen this as it is not working as expected. With just eina and efl as deps I cannot use EFL_MAIN.
It seems like I need to depend on ecore as well, but even if I do I still get compile errors.

I had thought that efl dep and Efl_Core.h (and maybe Efl.h) is all that we need but it appears not.

FAILED: src/efl_example_cmdline@exe/cmdline_main.c.o
cc  -Isrc/efl_example_cmdline@exe -Isrc -I../src -I. -I../ -I/opt/efler/include/efl-1 -I/opt/efler/include/eina-1 -I/opt/efler/include/eina-1/eina -I/opt/efler/include/eo-1 -I/opt/efler/include/ecore-1 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu99 -O0 -g -pthread -MMD -MQ 'src/efl_example_cmdline@exe/cmdline_main.c.o' -MF 'src/efl_example_cmdline@exe/cmdline_main.c.o.d' -o 'src/efl_example_cmdline@exe/cmdline_main.c.o' -c ../src/cmdline_main.c
../src/cmdline_main.c:8:11: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
 EAPI_MAIN void
           ^~~~
In file included from /opt/efler/include/efl-1/Efl.h:8:0,
                 from ../src/cmdline_main.c:5:
../src/cmdline_main.c: In function ‘main’:
/opt/efler/include/ecore-1/efl_general.h:77:29: warning: implicit declaration of function ‘ecore_main_loop_get’; did you mean ‘ecore_main_loop_quit’? [-Wimplicit-function-declaration]
      efl_event_callback_add(ecore_main_loop_get(), EFL_LOOP_EVENT_ARGUMENTS, efl_main, NULL); \
                             ^
/opt/efler/include/eo-1/Eo.h:2027:36: note: in definition of macro ‘efl_event_callback_add’
    efl_event_callback_priority_add(obj, desc, \
                                    ^~~
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
/opt/efler/include/ecore-1/efl_general.h:77:52: error: ‘EFL_LOOP_EVENT_ARGUMENTS’ undeclared (first use in this function); did you mean ‘EFL_UI_EVENT_DRAG_END’?
      efl_event_callback_add(ecore_main_loop_get(), EFL_LOOP_EVENT_ARGUMENTS, efl_main, NULL); \
                                                    ^
/opt/efler/include/eo-1/Eo.h:2027:41: note: in definition of macro ‘efl_event_callback_add’
    efl_event_callback_priority_add(obj, desc, \
                                         ^~~~
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
/opt/efler/include/ecore-1/efl_general.h:77:52: note: each undeclared identifier is reported only once for each function it appears in
      efl_event_callback_add(ecore_main_loop_get(), EFL_LOOP_EVENT_ARGUMENTS, efl_main, NULL); \
                                                    ^
/opt/efler/include/eo-1/Eo.h:2027:41: note: in definition of macro ‘efl_event_callback_add’
    efl_event_callback_priority_add(obj, desc, \
                                         ^~~~
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
/opt/efler/include/ecore-1/efl_general.h:77:78: error: ‘efl_main’ undeclared (first use in this function); did you mean ‘efl_pack’?
      efl_event_callback_add(ecore_main_loop_get(), EFL_LOOP_EVENT_ARGUMENTS, efl_main, NULL); \
                                                                              ^
/opt/efler/include/eo-1/Eo.h:2028:41: note: in definition of macro ‘efl_event_callback_add’
          EFL_CALLBACK_PRIORITY_DEFAULT, cb, data)
                                         ^~
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
/opt/efler/include/ecore-1/efl_general.h:77:29: warning: passing argument 1 of ‘efl_event_callback_priority_add’ makes pointer from integer without a cast [-Wint-conversion]
      efl_event_callback_add(ecore_main_loop_get(), EFL_LOOP_EVENT_ARGUMENTS, efl_main, NULL); \
                             ^
/opt/efler/include/eo-1/Eo.h:2027:36: note: in definition of macro ‘efl_event_callback_add’
    efl_event_callback_priority_add(obj, desc, \
                                    ^~~
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
In file included from /opt/efler/include/efl-1/Efl.h:8:0,
                 from ../src/cmdline_main.c:5:
/opt/efler/include/eo-1/Eo.h:262:17: note: expected ‘Eo * {aka struct _Eo_Opaque *}’ but argument is of type ‘int’
 EOAPI Eina_Bool efl_event_callback_priority_add(Eo *obj, const Efl_Event_Description *desc, Efl_Callback_Priority priority, Efl_Event_Cb cb, const void *data);
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /opt/efler/include/ecore-1/Efl_Core.h:82:0,
                 from ../src/cmdline_main.c:6:
/opt/efler/include/ecore-1/efl_general.h:80:14: warning: implicit declaration of function ‘efl_loop_begin’; did you mean ‘efl_pack_begin’? [-Wimplicit-function-declaration]
      ret__ = efl_loop_begin(ecore_main_loop_get());                     \
              ^
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
/opt/efler/include/ecore-1/efl_general.h:80:12: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
      ret__ = efl_loop_begin(ecore_main_loop_get());                     \
            ^
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~
/opt/efler/include/ecore-1/efl_general.h:81:15: warning: implicit declaration of function ‘efl_loop_exit_code_process’ [-Wimplicit-function-declaration]
      real__ = efl_loop_exit_code_process(ret__);                        \
               ^
../src/cmdline_main.c:24:1: note: in expansion of macro ‘EFL_MAIN’
 EFL_MAIN()
 ^~~~~~~~

OK, to make this work I need to:

  • Include a dependency on *ecore*
  • Add #define EFL_OBJECT_BETA 1 to my headers
  • remove EAPI_MAIN from the main definition

After that it works.

Item 1 is disappointing - can we avoid this?
Number 2 is a new define to me - it's only checked in 2 places, is it correct or should it be EFL_EO_API_SUPPORT or EFL_BETA_API_SUPPORT instead?
I think that 3 means another item to move out of ELM.

Can these be fixed as part of this or is it a wider problem?

Yes, this should be fixed now. You should be able to just rely on Efl_Core.h after e5d84da864214b9f772808040f22e1614889a25f, 619d0f3cff023414feb8f2aaeebf468806c50b44 and 814ffb9b6bd50d498bfd98f4b8a75f1cad3cc0cf. Thanks for catching them all and sorry for not seeing it before.

cedric closed this task as Resolved.Nov 14 2017, 2:42 PM