Bug 95237 – [EFL] Rewrite the EFL-related Find modules

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Raphael Kubo da Costa (:rakuco)]

[EFL] Rewrite the EFL-related Find modules

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161046 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161078 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161078 [details]
Patch

Attachment 161078 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13665455

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> (From update of attachment 161078 [details])
> Attachment 161078 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13665455

It looks like the bot has a bad setup and has an outdated version of the EFL installed in /usr.

[Gyuyoung Kim]

Comment on attachment 161078 [details]
Patch

Attachment 161078 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13654579

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161109 [details]
New attempt with a better bot setup

[Gyuyoung Kim]

Comment on attachment 161109 [details]
New attempt with a better bot setup

Attachment 161109 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13682032

[Gyuyoung Kim]

Comment on attachment 161109 [details]
New attempt with a better bot setup

Attachment 161109 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13685018

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> (From update of attachment 161109 [details])
> Attachment 161109 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13682032

This one looks like <http://trac.webkit.org/changeset/120113> showing it should not have been committed. I'll ping drott about this.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161181 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161181 [details]
Patch

Attachment 161181 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13685183

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161243 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161243 [details]
Patch

Attachment 161243 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13691305

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161284 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 161284 [details]
Patch

Attachment 161284 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13685442

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161324 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 161324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161324&action=review

Please try a clean build with shared core, because I did that locally and got some linking errors.

CMakeFiles/MiniBrowser.dir/__/__/EWebLauncher/url_bar.c.o:url_bar.c:function on_urlbar_key_down: error: undefined reference to 'ecore_file_exists'

> ChangeLog:16
> +        o It depended on pkg-config being present for the searches to
> +        succeed.

We have other libraries using pkg-config, we depend on it anyway.

> ChangeLog:22
> +        o It obtained the library definitions from pkg-config, so
> +        ${FOO_LIBRARIES} would be set to something like "foo;bar" which
> +        expanded to "-lfoo -lbar" to the linker. If a wrong -L<path> was
> +        passed before that, the wrong library installation would end up
> +        being picked up.

If the -L path supplied by pkgconfig is wrong, it should be fixed on the library .pc upstream (not hack a fix on WebKit). I'm not aware of any build error because of that anyway. IMO this change would make it much more difficult to maintain than blindly use PKG_CHECK_MODULES which AFAIK just works and takes a couple of lines of code.

> ChangeLog:29
> +        o Due to the problem above, we also needed to set the LINK_FLAGS
> +        property for each target with the value of ${FOO_LDFLAGS}, which was
> +        also obtained from pkg-config and sort of compensated the fact that
> +        the libraries did not use absolute paths and added the required -L
> +        paths. This also included dependencies for these libraries, so we
> +        ended up including libraries indirectly, which is bad.

LD_FLAGS are not evil, check the example bellow:
$ pkg-config --libs evas
-pthread -L/opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Dependencies/Root/lib -levas -leina -lrt

Looks like you are just ignoring -pthread, -leina and -lrt, since EVAS_LIBRARIES is set to the full path of libevas.so.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 161462 [details]
Make it build with SHARED_CORE=ON

[Raphael Kubo da Costa (:rakuco)]

Long reply ahead :-)

(In reply to comment #17)
> Please try a clean build with shared core, because I did that locally and got some linking errors.

Fixed, thanks!

> > ChangeLog:16
> > +        o It depended on pkg-config being present for the searches to
> > +        succeed.
>
> We have other libraries using pkg-config, we depend on it anyway.

True enough, however our build system itself is progressively getting rid of the dependency on pkg-config and works fine without having to resort to .pc files; last time I checked there was some confusion the KDE and CMake communities as to how usable pkg-config was on platforms such as Windows, so not making things still work without having to rely on it is a plus.

> > ChangeLog:22
> > +        o It obtained the library definitions from pkg-config, so
> > +        ${FOO_LIBRARIES} would be set to something like "foo;bar" which
> > +        expanded to "-lfoo -lbar" to the linker. If a wrong -L<path> was
> > +        passed before that, the wrong library installation would end up
> > +        being picked up.
>
> If the -L path supplied by pkgconfig is wrong, it should be fixed on the library .pc upstream (not hack a fix on WebKit). I'm not aware of any build error because of that anyway. IMO this change would make it much more difficult to maintain than blindly use PKG_CHECK_MODULES which AFAIK just works and takes a couple of lines of code.

I disagree that this approach is a hack; it is quite the opposite, I'd say.

The .pc files normally provide the right -L flags; what usually causes problems is the way we use pkg-config's output as it is parsed by CMake without even using FIND_LIBRARY() and FIND_PATH().

pkg-config can output the values in a .pc in many different ways, and CMake creates many different variables corresponding to those output modes. One of those variables is ${FOO_LIBRARIES}, which contains the output of `pkg-config --libs-only-l'. Due to the naming similarity with the variable that is normally set by Find modules after a FIND_LIBRARY() call, we end up using that ${FOO_LIBRARIES} directly in TARGET_LINK_LIBRARIES() calls, even if, contrary to what would happen if we had correctly used FIND_LIBRARY(), the value obtained from pkg-config only contains entries such as "foo;bar" instead of "/path/to/libfoo.so;/other/path/to/libbar.so". This has caused problems such as the one solved by <http://trac.webkit.org/changeset/126178>, since if libfoo.so is not in a directory in the linker's standard path the lookup will fail if "-lfoo" is passed instead of its absolute path.

To work around that, we were setting the LINK_FLAGS target property on our libraries and executables with the value of ${FOO_LDFLAGS}, which contains the output of `pkg-config --libs'. Besides containing the "-L<dir>" values, it also contains output which is already part of --libs-only-l and thus duplicates the value in ${FOO_LIBRARIES}.

Could we use ${FOO_LIBRARY_DIRS} instead, which contains the output of `pkg-config --libs-only-L'? Maybe, but this is all work which could be avoided by simply using one CMake's core commands designed specifically for what we are trying to do.

In summary, we'd better use pkg-config only as a hint if it is available on the system, but not more than that.

> > ChangeLog:29
> > +        o Due to the problem above, we also needed to set the LINK_FLAGS
> > +        property for each target with the value of ${FOO_LDFLAGS}, which was
> > +        also obtained from pkg-config and sort of compensated the fact that
> > +        the libraries did not use absolute paths and added the required -L
> > +        paths. This also included dependencies for these libraries, so we
> > +        ended up including libraries indirectly, which is bad.
>
> LD_FLAGS are not evil, check the example bellow:
> $ pkg-config --libs evas
> -pthread -L/opt/tmpsantos/projects/webkit-efl-2/WebKitBuild/Dependencies/Root/lib -levas -leina -lrt

% ./Tools/efl/run-with-jhbuild pkg-config --libs evas
-levas -L/usr/home/rakuco/dev/WebKit/WebKitBuild/Dependencies/Root/lib -leina -pthread

Which means in some cases -levas could even be passed to the linker before its actual path.

> Looks like you are just ignoring -pthread, -leina and -lrt, since EVAS_LIBRARIES is set to the full path of libevas.so.

That is OK -- instead of obtaining these extra libraries implicitly and forwarding them to the linker, we should properly look for them in an explicitly way instead. This is exactly what happened in bug 93030, for example.

[Thiago Marcos P. Santos]

Comment on attachment 161462 [details]
Make it build with SHARED_CORE=ON

LGTM.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 163351 [details]
Rebased patch

[Kenneth Rohde Christiansen]

Comment on attachment 163351 [details]
Rebased patch

r=me as it is looked over by thiago santos

[Raphael Kubo da Costa (:rakuco)]

Created attachment 163370 [details]
Patch for landing. Assign copyright ot Intel.

[Raphael Kubo da Costa (:rakuco)]

And finally landed in <http://trac.webkit.org/changeset/128191> \o/