Bug 77833 – Get rid of Source/autotools/webkit.m4

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Priit Laes (IRC: plaes)]

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.

[Priit Laes (IRC: plaes)]

Created attachment 125527 [details]
webkit-drop-webkit-m4.patch

[Priit Laes (IRC: plaes)]

Created attachment 125528 [details]
webkit-drop-webkit-m4.patch

[WebKit Review Bot]

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.

[Priit Laes (IRC: plaes)]

Created attachment 125531 [details]
webkit-drop-webkit-m4.patch

[Priit Laes (IRC: plaes)]

Created attachment 125532 [details]
webkit-drop-webkit-m4.patch

One more missed tab :S

[Martin Robinson]

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?

[Priit Laes (IRC: plaes)]

(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`.

[Martin Robinson]

(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?

[Priit Laes (IRC: plaes)]

(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)

[Kalev Lember]

> > > +        *-*-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

[Priit Laes (IRC: plaes)]

Created attachment 125693 [details]
webkit-drop-webkit-m4.patch

Uses now AM_PATH_GLIB, also added correct lib for ICU on mingw.

[Martin Robinson]

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?

[Priit Laes (IRC: plaes)]

Created attachment 125769 [details]
webkit-drop-webkit-m4.patch

Attached correct patch. Thanks for catching that.

[Martin Robinson]

Comment on attachment 125769 [details]
webkit-drop-webkit-m4.patch

Thanks!

[WebKit Review Bot]

Comment on attachment 125769 [details]
webkit-drop-webkit-m4.patch

Clearing flags on attachment: 125769

Committed r107016: <http://trac.webkit.org/changeset/107016>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.