After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 795045 - More mingw build fixes
More mingw build fixes
Status: RESOLVED FIXED
Product: pango
Classification: Platform
Component: win32
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-win32 maintainers
pango-maint
Depends on:
Blocks:
 
 
Reported: 2018-04-07 05:52 UTC by Christoph Reiter (lazka)
Modified: 2018-04-09 13:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
build: move usp10 before gdi32 (1.98 KB, patch)
2018-04-07 05:52 UTC, Christoph Reiter (lazka)
committed Details | Review
pangowin32: fix script cache hash key for 64bit builds (2.99 KB, patch)
2018-04-07 05:53 UTC, Christoph Reiter (lazka)
none Details | Review
meson: skip pangoxft headers for the Windows gtk-doc build (785 bytes, patch)
2018-04-08 17:19 UTC, Christoph Reiter (lazka)
committed Details | Review
pangowin32: fix script cache hash key for 64bit builds (3.06 KB, patch)
2018-04-09 13:40 UTC, Christoph Reiter (lazka)
committed Details | Review

Description Christoph Reiter (lazka) 2018-04-07 05:52:32 UTC
Created attachment 370624 [details] [review]
build: move usp10 before gdi32

Without this pango on mingw64 tries to lookup up the Script* functions
in gdi32 and fails. It already fails at the build stage because the
introspection dump crashes with a missing entry point error.

Moving usp10 before gdi32 makes things work.

This might be related to the warning in the uniscribe docs:
"Important Starting with Windows 8: To maintain the ability to run on
Windows 7, a module that uses Uniscribe must specify Usp10.lib
before gdi32.lib in its library list."
Comment 1 Christoph Reiter (lazka) 2018-04-07 05:53:32 UTC
Created attachment 370625 [details] [review]
pangowin32: fix script cache hash key for 64bit builds

It joins the HFONT and script key to a gint64 and uses this as a
hash key, but on 64bit Windows hfont doesn't fit in 32bit.

This also breaks the build with meson where -Werror=pointer-to-int-cast
is enabled by default.

Instead of using the gint64 hash functions add our own key type and implement
matching hash and equality functions.
Comment 2 Christoph Reiter (lazka) 2018-04-08 17:19:35 UTC
Created attachment 370660 [details] [review]
meson: skip pangoxft headers for the Windows gtk-doc build

The API isn't available on Windows and gtk-doc fails with a
linker error otherwise.
Comment 3 Ignacio Casal Quinteiro (nacho) 2018-04-09 07:00:36 UTC
Review of attachment 370624 [details] [review]:

Fine for me but I would probably add a comment in the meson.build file as well...
Comment 4 Ignacio Casal Quinteiro (nacho) 2018-04-09 07:02:44 UTC
Review of attachment 370660 [details] [review]:

Fine for me
Comment 5 Ignacio Casal Quinteiro (nacho) 2018-04-09 07:10:57 UTC
Review of attachment 370625 [details] [review]:

Apart from the coding style which need some fixing, please have a look at the comment inline.

::: pango/pangowin32-shape.c
@@ +379,3 @@
+script_cache_key_hash_func (script_cache_key *key) {
+    gint64 temp = key->script;
+    return g_direct_hash (key->hfont) ^ g_int64_hash (&temp);

As far as I understand we are doing all of this to use 64 bit for hfont but then here you are using g_direct_hash which returns a guint, also you do int64_hash of the script while the script is a gint32, should they be reversed?
Comment 6 Christoph Reiter (lazka) 2018-04-09 08:22:45 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #5)
> Review of attachment 370625 [details] [review] [review]:
> 
> Apart from the coding style which need some fixing, please have a look at
> the comment inline.
> 
> ::: pango/pangowin32-shape.c
> @@ +379,3 @@
> +script_cache_key_hash_func (script_cache_key *key) {
> +    gint64 temp = key->script;
> +    return g_direct_hash (key->hfont) ^ g_int64_hash (&temp);
> 
> As far as I understand we are doing all of this to use 64 bit for hfont but
> then here you are using g_direct_hash which returns a guint, also you do
> int64_hash of the script while the script is a gint32, should they be
> reversed?

(disclaimer: First time using the hash API).

GHashFunc uses guint for the hash as a return value as do all builtin hash functions. The hashes don't have to be unique, so this shouldn't matter.

I'm using g_int64_hash because there exists no g_int32_hash in glib and using the existing one seemed preferable.
Comment 7 Ignacio Casal Quinteiro (nacho) 2018-04-09 08:24:32 UTC
You have g_int_hash which is basically the same as int32_hash
Comment 8 Christoph Reiter (lazka) 2018-04-09 08:40:52 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #7)
> You have g_int_hash which is basically the same as int32_hash

In practice, yes. Should I change it?
Comment 9 Ignacio Casal Quinteiro (nacho) 2018-04-09 08:46:56 UTC
I think it would be more clear yes. Also, do we need to use int64_hash for the font instead of direct hash?
Comment 10 Christoph Reiter (lazka) 2018-04-09 08:51:37 UTC
> I think it would be more clear yes

ok

> Also, do we need to use int64_hash for the font instead of direct hash?

I don't think so, HFONT is a pointer type.
Comment 11 Ignacio Casal Quinteiro (nacho) 2018-04-09 08:54:51 UTC
OK, just double checking with you since that is what I understood from the commit message. Maybe change the commit message to clarify a bit better.
Comment 12 Christoph Reiter (lazka) 2018-04-09 12:46:50 UTC
Comment on attachment 370624 [details] [review]
build: move usp10 before gdi32

pushed and added a comment in meson.build pointing to this bug
Comment 13 Christoph Reiter (lazka) 2018-04-09 13:40:28 UTC
Created attachment 370692 [details] [review]
pangowin32: fix script cache hash key for 64bit builds

* Mentioned that HFONT is a pointer type in the commit message
* Use g_int_hash instead of g_int64_hash
* Coding style fixes
Comment 14 Ignacio Casal Quinteiro (nacho) 2018-04-09 13:44:01 UTC
Review of attachment 370692 [details] [review]:

Fix the style issue and go ahead with it.

::: pango/pangowin32-shape.c
@@ +383,3 @@
+
+static gboolean
+script_cache_key_equal_func (script_cache_key *a, script_cache_key *b)

one param per line
Comment 15 Christoph Reiter (lazka) 2018-04-09 13:49:35 UTC
Done, thanks!

MSYS2 now uses meson to build pango: https://github.com/Alexpux/MINGW-packages/pull/3570