Bug 77833 - Get rid of Source/autotools/webkit.m4
Summary: Get rid of Source/autotools/webkit.m4
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-05 07:42 PST by Priit Laes (IRC: plaes)
Modified: 2012-02-07 17:16 PST (History)
5 users (show)

See Also:


Attachments
webkit-drop-webkit-m4.patch (58 bytes, text/plain)
2012-02-05 08:26 PST, Priit Laes (IRC: plaes)
no flags Details
webkit-drop-webkit-m4.patch (12.02 KB, patch)
2012-02-05 08:28 PST, Priit Laes (IRC: plaes)
no flags Details | Formatted Diff | Diff
webkit-drop-webkit-m4.patch (12.12 KB, patch)
2012-02-05 09:33 PST, Priit Laes (IRC: plaes)
no flags Details | Formatted Diff | Diff
webkit-drop-webkit-m4.patch (12.12 KB, patch)
2012-02-05 09:35 PST, Priit Laes (IRC: plaes)
mrobinson: review-
Details | Formatted Diff | Diff
webkit-drop-webkit-m4.patch (12.12 KB, patch)
2012-02-06 12:35 PST, Priit Laes (IRC: plaes)
mrobinson: review-
Details | Formatted Diff | Diff
webkit-drop-webkit-m4.patch (12.19 KB, patch)
2012-02-06 22:37 PST, Priit Laes (IRC: plaes)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Priit Laes (IRC: plaes) 2012-02-05 07:42:18 PST
The original purpose of Source/autotools/webkit.m4 file was to have a single location for storing common configure scripts between JSC and WebKit builds, but apparently this split-build support never materialized.
Comment 1 Priit Laes (IRC: plaes) 2012-02-05 08:26:20 PST
Created attachment 125527 [details]
webkit-drop-webkit-m4.patch
Comment 2 Priit Laes (IRC: plaes) 2012-02-05 08:28:12 PST
Created attachment 125528 [details]
webkit-drop-webkit-m4.patch
Comment 3 WebKit Review Bot 2012-02-05 08:32:28 PST
Attachment 125528 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/autotools/webkit.m4',..." exit_code: 1

configure.ac:417:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:418:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:421:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:422:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:425:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:426:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:427:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:428:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:430:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:431:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:432:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:433:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:434:  Line contains tab character.  [whitespace/tab] [5]
configure.ac:440:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 14 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Priit Laes (IRC: plaes) 2012-02-05 09:33:39 PST
Created attachment 125531 [details]
webkit-drop-webkit-m4.patch
Comment 5 Priit Laes (IRC: plaes) 2012-02-05 09:35:57 PST
Created attachment 125532 [details]
webkit-drop-webkit-m4.patch

One more missed tab :S
Comment 6 Martin Robinson 2012-02-05 17:29:00 PST
Comment on attachment 125532 [details]
webkit-drop-webkit-m4.patch

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

This is a really great cleanup.

> Source/autotools/webkit.m4:-7
> -m4_define([initialized], [no])

Why can we remove this?

> configure.ac:394
> +PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION])

I believe it's better to use AM_PATH_GLIB_2_0($GLIB_REQUIRED_VERSION) here, because it has support for pulling in GLib modules individually. See http://trac.webkit.org/changeset/106485. I'd like to have this patch in master too.

> configure.ac:437
> +if test "$with_unicode_backend" = "icu"; then
> +    # TODO: use pkg-config (after CFLAGS in .pc files are cleaned up)
> +    case "$host" in
> +        *-*-darwin*)
> +            UNICODE_CFLAGS="-I$srcdir/Source/JavaScriptCore/icu -I$srcdir/Source/WebCore/icu"
> +            UNICODE_LIBS="-licucore"
> +            ;;
> +        *-*-mingw*)
> +            UNICODE_CFLAGS=""
> +            UNICODE_LIBS="-licuin -licuuc"
> +            ;;
> +        *)
> +            AC_PATH_PROG(icu_config, icu-config, no)
> +            if test "$icu_config" = "no"; then
> +                AC_MSG_ERROR([Cannot find icu-config. The ICU library is needed.])
> +            fi
> +
> +            # We don't use --cflags as this gives us a lot of things that we don't
> +            # necessarily want, like debugging and optimization flags
> +            # See man (1) icu-config for more info.
> +            UNICODE_CFLAGS=`$icu_config --cppflags`
> +            UNICODE_LIBS=`$icu_config --ldflags-libsonly`
> +            ;;
> +    esac
> +fi

Can we use this moment to remove this work-around. Do you mind confirming that the pkg-config files for Windows are functional, at least?
Comment 7 Priit Laes (IRC: plaes) 2012-02-06 00:44:54 PST
(In reply to comment #6)
> (From update of attachment 125532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125532&action=review
> 
> This is a really great cleanup.
> 
> > Source/autotools/webkit.m4:-7
> > -m4_define([initialized], [no])
> 
> Why can we remove this?

I assume its original purpose was checking whether WEBKIT_INIT had been already called. And I couldn't find it used anywhere else.

> 
> > configure.ac:394
> > +PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION])
> 
> I believe it's better to use AM_PATH_GLIB_2_0($GLIB_REQUIRED_VERSION) here, because it has support for pulling in GLib modules individually. See http://trac.webkit.org/changeset/106485. I'd like to have this patch in master too.

How about this then:
PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION
    gmodule-2.0 gobject-2.0 gthread-2.0 gio-2.0])

> > configure.ac:437
> > +if test "$with_unicode_backend" = "icu"; then
> > +    # TODO: use pkg-config (after CFLAGS in .pc files are cleaned up)
> > +    case "$host" in
> > +        *-*-darwin*)
> > +            UNICODE_CFLAGS="-I$srcdir/Source/JavaScriptCore/icu -I$srcdir/Source/WebCore/icu"
> > +            UNICODE_LIBS="-licucore"
> > +            ;;
> > +        *-*-mingw*)
> > +            UNICODE_CFLAGS=""
> > +            UNICODE_LIBS="-licuin -licuuc"
> > +            ;;
> > +        *)
> > +            AC_PATH_PROG(icu_config, icu-config, no)
> > +            if test "$icu_config" = "no"; then
> > +                AC_MSG_ERROR([Cannot find icu-config. The ICU library is needed.])
> > +            fi
> > +
> > +            # We don't use --cflags as this gives us a lot of things that we don't
> > +            # necessarily want, like debugging and optimization flags
> > +            # See man (1) icu-config for more info.
> > +            UNICODE_CFLAGS=`$icu_config --cppflags`
> > +            UNICODE_LIBS=`$icu_config --ldflags-libsonly`
> > +            ;;
> > +    esac
> > +fi
> 
> Can we use this moment to remove this work-around. Do you mind confirming that the pkg-config files for Windows are functional, at least?

Not sure about Windows, but I can't even get sensible information on Linux unless I play with `pkg-config --variable foo`.
Comment 8 Martin Robinson 2012-02-06 00:52:34 PST
(In reply to comment #7)

> > > configure.ac:394
> > > +PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION])
> > 
> > I believe it's better to use AM_PATH_GLIB_2_0($GLIB_REQUIRED_VERSION) here, because it has support for pulling in GLib modules individually. See http://trac.webkit.org/changeset/106485. I'd like to have this patch in master too.
> 
> How about this then:
> PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION
>     gmodule-2.0 gobject-2.0 gthread-2.0 gio-2.0])

It seems better to use the autoconfig glue that GLib provides. Is there any reason the PKG_CHECK_MODULES version is superior?
Comment 9 Priit Laes (IRC: plaes) 2012-02-06 09:27:50 PST
(In reply to comment #8)
> (In reply to comment #7)
> 
> > > > configure.ac:394
> > > > +PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION])
> > > 
> > > I believe it's better to use AM_PATH_GLIB_2_0($GLIB_REQUIRED_VERSION) here, because it has support for pulling in GLib modules individually. See http://trac.webkit.org/changeset/106485. I'd like to have this patch in master too.
> > 
> > How about this then:
> > PKG_CHECK_MODULES([GLIB], [glib-2.0 >= $GLIB_REQUIRED_VERSION
> >     gmodule-2.0 gobject-2.0 gthread-2.0 gio-2.0])
> 
> It seems better to use the autoconfig glue that GLib provides. Is there any reason the PKG_CHECK_MODULES version is superior?

There's only thing favoring AM_PATH_GLIB - it tries to check (by compiling a small sample) whether library works correctly. Also, if you check the source of the AM_PATH_GLIB it's using pkg-config internally.

And last but not least, AM_PATH_GLIB seems to be just a historical relic reminding us about the good old times when pkg-config didn't yet exist (pre-2006)
Comment 10 Kalev Lember 2012-02-06 10:53:28 PST
> > > +        *-*-mingw*)
> > > +            UNICODE_CFLAGS=""
> > > +            UNICODE_LIBS="-licuin -licuuc"

In the MinGW environments I've seen, it's "icui18n" instead of "icuin", see e.g. bug 77837. I guess that's one more reason to switch to pkg-config for ICU detection.


> > Can we use this moment to remove this work-around. Do you mind confirming that the pkg-config files for Windows are functional, at least?
> 
> Not sure about Windows, but I can't even get sensible information on Linux unless I play with `pkg-config --variable foo`.

Yeah, ICU pkg-config files should be fine on Windows.

But I think we might have problems with Debian that appears to be not packaging ICU .pc files. Although I bet it's just an oversight and they'd fix it if we started discovering ICU through pkg-config.

http://packages.debian.org/sid/amd64/libicu-dev/filelist
Comment 11 Priit Laes (IRC: plaes) 2012-02-06 12:35:24 PST
Created attachment 125693 [details]
webkit-drop-webkit-m4.patch

Uses now AM_PATH_GLIB, also added correct lib for ICU on mingw.
Comment 12 Martin Robinson 2012-02-06 15:23:34 PST
Comment on attachment 125693 [details]
webkit-drop-webkit-m4.patch

It doesn't look like this one is actually using AM_PATH_GLIB_2_0. Did you upload the correct patch?
Comment 13 Priit Laes (IRC: plaes) 2012-02-06 22:37:57 PST
Created attachment 125769 [details]
webkit-drop-webkit-m4.patch

Attached correct patch. Thanks for catching that.
Comment 14 Martin Robinson 2012-02-07 16:28:32 PST
Comment on attachment 125769 [details]
webkit-drop-webkit-m4.patch

Thanks!
Comment 15 WebKit Review Bot 2012-02-07 17:16:54 PST
Comment on attachment 125769 [details]
webkit-drop-webkit-m4.patch

Clearing flags on attachment: 125769

Committed r107016: <http://trac.webkit.org/changeset/107016>
Comment 16 WebKit Review Bot 2012-02-07 17:16:59 PST
All reviewed patches have been landed.  Closing bug.