ICU-21628 MinGW/Windows incorrect build by LiteratimBi · Pull Request #1733 · unicode-org/icu

LiteratimBi · 2021-05-24T16:52:37Z

Checklist

Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-21628
Required: The PR title must be prefixed with a JIRA Issue number.
Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
Required: Each commit message must be prefixed with a JIRA Issue number.
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable

Mingw* configuration/build/setup has some pitfalls:

1. pkgdata utility produces incorrect datastub filenames when LIBPREFIX is set.

With
LIBPREFIX=lib
createFileNames (from tools/pkgdata/pkgdata.cpp:926) produces liblibicu* names

2. Less important, also pkgdata utility:

--help option message contains incorrect prefixes/suffixes (from tools/pkgdata/pkgtypes.h:139)
defined at struct modes (from tools/pkgdata/pkgdata.cpp:134)

3. Default build PKGDATA_MODE (= dll ) with ENABLE_STATIC

Makefile puts libicudtXX.a static library to bindir (data/Makefile.in:201 install-local target)

4. Platform-dependant definitions of linker flags are inconsistent (example)

ICU Issue link: https://unicode-org.atlassian.net/browse/ICU-21628

CLAassistant · 2021-05-24T16:52:42Z

All committers have signed the CLA.

jefgen · 2021-05-26T18:59:41Z

Thank you very much for creating this PR, signing the CLA, and also for filing a ticket! :)

To be honest, I'm not super familiar with MinGW and its conventions, so I would love if someone else with MinGW experience and expertise could help take a look at these changes.

Recently, there have been other pull-requests from a number of people that also made changes to the ICU MinGW build/config: @longnguyen2004 @autoantwort @pchemguy. (I'm mentioning them here, in case they are interested in taking a look).

I don't know if any of them would have time or not -- but if so, it would be great if they might be able to take a look at the changes in this pull request (since it also changes the MinGW build as well).

Also, I think this change will conflict with some of the changes in PR #1732.
At minimum, we'd need to resolve the merge conflicts, but I wonder if maybe the PR author could look at this change as well.

Regarding this:

Platform-dependant definitions of linker flags are inconsistent

FWIW, I think the PR #1732 was also trying to fix this as well maybe.

jefgen · 2021-05-26T19:03:50Z

icu4c/source/data/Makefile.in

@@ -198,7 +198,7 @@ ifneq ($(ENABLE_STATIC),)
 ifeq ($(PKGDATA_MODE),dll)
 # For MinGW, do we want the DLL to go in the bin location?


I wonder if it might be worth updating this comment. Maybe it should say something that for static builds, it is put in the libdir.

Changed this comment to clarify target (commit)

autoantwort · 2021-05-26T20:13:55Z

icu4c/source/config/mh-mingw

 DATA_STUBNAME = dt
+COMMON_STUBNAME = uc
 I18N_STUBNAME = in
-LIBICU = $(STATIC_PREFIX_WHEN_USED)$(ICUPREFIX)


Have you tested the static mingw build?

It even fixes the static build. Awesome :)

longnguyen2004 · 2021-05-27T04:45:46Z

Hello @LiteratimBi, and thanks for making this PR!
I just did a test build with --enable-static and everything seems good so far, I'll try a build with --enable-shared.

EDIT:
Shared build looks fine for lib, but not for bin
lib folder:

icu
libicudt.dll.a
libicuin.dll.a
libicuio.dll.a
libicutest.dll.a
libicutu.dll.a
libicuuc.dll.a
pkgconfig

bin folder:

derb.exe
escapesrc.exe
genbrk.exe
genccode.exe
gencfu.exe
gencmn.exe
gencnval.exe
gendict.exe
gennorm2.exe
genrb.exe
gensprep.exe
icu-config
icuinfo.exe
icupkg.exe
libicudt69.dll
libicuin69.dll
libicuio69.dll
libicutest69.dll
libicutu69.dll
libicuuc69.dll
liblibicudt69.dll
makeconv.exe
pkgdata.exe
uconv.exe

There's an extra liblibicudt69.dll. Can you check where it comes from?

LiteratimBi · 2021-05-27T06:51:28Z

Hello @LiteratimBi, and thanks for making this PR!
I just did a test build with --enable-static and everything seems good so far, I'll try a build with --enable-shared.

EDIT:
Shared build looks fine for lib, but not for bin
lib folder:
icu
libicudt.dll.a
libicuin.dll.a
libicuio.dll.a
libicutest.dll.a
libicutu.dll.a
libicuuc.dll.a
pkgconfig
bin folder:
derb.exe
escapesrc.exe
genbrk.exe
genccode.exe
gencfu.exe
gencmn.exe
gencnval.exe
gendict.exe
gennorm2.exe
genrb.exe
gensprep.exe
icu-config
icuinfo.exe
icupkg.exe
libicudt69.dll
libicuin69.dll
libicuio69.dll
libicutest69.dll
libicutu69.dll
libicuuc69.dll
liblibicudt69.dll
makeconv.exe
pkgdata.exe
uconv.exe
There's an extra liblibicudt69.dll. Can you check where it comes from?

According to size, libicudt69.dll is pre-build wrapper and liblibicudt69.dll is the correct datastub

Are you shure that you using correct branch (git checkout mingw-make)?
If yes, autoconf output might be helpfull

Mine bindir looks so:

total 42368
   327900 May 27 09:23 derb.exe
   534607 May 27 09:23 escapesrc.exe
   317569 May 27 09:23 genbrk.exe
   297824 May 27 09:23 genccode.exe
   315461 May 27 09:23 gencfu.exe
   296130 May 27 09:23 gencmn.exe
   311228 May 27 09:23 gencnval.exe
   325227 May 27 09:23 gendict.exe
   398208 May 27 09:23 gennorm2.exe
   510968 May 27 09:23 genrb.exe
   311538 May 27 09:23 gensprep.exe
    25605 May 27 09:23 icu-config
   298983 May 27 09:23 icuinfo.exe
   308523 May 27 09:23 icupkg.exe
 28870610 May 27 09:23 libicudt69.dll
  4878882 May 27 09:23 libicuin69.dll
   176751 May 27 09:23 libicuio69.dll
   352897 May 27 09:23 libicutest69.dll
   772231 May 27 09:23 libicutu69.dll
  2648525 May 27 09:23 libicuuc69.dll
   348183 May 27 09:23 makeconv.exe
   344525 May 27 09:23 pkgdata.exe
   354520 May 27 09:23 uconv.exe

longnguyen2004 · 2021-05-27T07:35:11Z

I merged your branch on top of main. I'll try doing a build without it and see what happens.

longnguyen2004 · 2021-05-27T12:49:40Z

Hmm, maybe I did something wrong. I'll do some more investigation.
If anyone's interested in reproducing it, I'm cross-compiling from WSL2, using llvm-mingw.

srl295

looks reasonable to me

See unicode-org#1733

jira-pull-request-webhook · 2021-05-27T15:24:25Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

longnguyen2004 · 2021-05-28T04:05:49Z

I'm still getting liblibicudt69.dll for some reason. Can someone do a cross-compile from WSL/Ubuntu and see if the problem persists?

longnguyen2004 · 2021-05-28T06:40:00Z

icu4c/source/tools/pkgdata/pkgdata.cpp

        } else {
-            sprintf(libFileNames[LIB_FILE], "%s%s%s",
-                    (strstr(libName, "icudt") ? "lib" : ""),
-                    pkgDataFlags[LIBPREFIX],
+            sprintf(libFileNames[LIB_FILE], "%s%s",
+                    (strstr(libName, "libicudt") ? "" : pkgDataFlags[LIBPREFIX]),
                    libName);


Also add the same check to the #else case, since it fixes the problem for cross-compiling.
pkgdata --verbose output:

LD_LIBRARY_PATH=/home/KurumiGaming/icu/icu4c/source/build-native/stubdata:/home/KurumiGaming/icu/icu4c/source/build-native/tools/ctestfw:/home/KurumiGaming/icu/icu4c/source/build-native/lib:$LD_LIBRARY_PATH /home/KurumiGaming/icu/icu4c/source/build-native/bin/pkgdata -O ../data/icupkg.inc -q -c -s /home/KurumiGaming/icu/icu4c/source/build-cross-shared/data/out/build/icudt69l -d ../lib -e icudt69 -T ./out/tmp -p icudt69l -m dll -r 69 -L libicudt ./out/tmp/icudata.lst -v # pkgdata: Reading ./out/tmp/icudata.lst.. # Reading options file ../data/icupkg.inc ... # Writing package file ./out/tmp/icudt69l.dat .. # libFileName[LIB_FILE] = liblibicudt # libFileName[LIB_FILE_VERSION] = liblibicudt69.dll ...

...which indicates that it uses the #else case.
This is just a small part of a more general problem. As it is right now, pkgdata is really only suitable for native builds, since the platform-specific bits are being added in using macros, not by checking the host triplet.

This is just a small part of a more general problem. As it is right now, pkgdata is really only suitable for native builds, since the platform-specific bits are being added in using macros, not by checking the host triplet.

Yeah this is a real problem. I build icu with vcpkg, but vcpkg sets --with-cross-build= when building mingw on windows because host-triplet=x64-windows and target-triplet=x64-mingw-static. And with --with-cross-build= the build fails because of the problem you described

We can handle that on vcpkg, by not requiring the host dependency on Windows triplets. But the problem with pkgdata not suitable for cross compiling is still there, and needs to be fixed in the future.

Yeah I know and I have done that locally. I only wanted to highlight that this is even a problem when you cross build from windows to mingw

autoantwort · 2021-06-05T18:56:30Z

I'm still getting liblibicudt69.dll for some reason. Can someone do a cross-compile from WSL/Ubuntu and see if the problem persists?

Did you also get this problem with #1735?

longnguyen2004 · 2021-06-06T00:32:38Z

It seems to be less intrusive than this PR, I'll do a build today and see if it works

autoantwort · 2021-06-08T14:18:41Z

It seems to be less intrusive than this PR, I'll do a build today and see if it works

Do you already have results? :)

longnguyen2004 · 2021-06-08T16:53:29Z

Oh sorry, I forgot to comment on this one :)
The build works fine, but as expected, the static build puts the data library in the bin folder, and is also named icudt.a, which is incorrect. This is probably a problem in pkgdata.
And the native vs cross compile problem is still present on both PR, once again due to pkgdata (remember what I said about compile-time macros?), so neither of these would be a complete fix.

srl295 · 2021-06-10T14:57:39Z

Jenkins, test this.

autoantwort · 2021-06-27T18:54:37Z

@longnguyen2004 when I understand the last comments on the other pr correctly this pr has less problems than the other pr right?

longnguyen2004 · 2021-06-28T02:33:57Z

@longnguyen2004 when I understand the last comments on the other pr correctly this pr has less problems than the other pr right?

Yes that's correct, but this PR is also changing a lot of other things that I don't fully understand, so I say we should explore other approaches first, like fixing up pkgdata by itself, before settling on this.

autoantwort · 2021-07-03T10:50:23Z

Yes that's correct, but this PR is also changing a lot of other things that I don't fully understand, so I say we should explore other approaches first, like fixing up pkgdata by itself, before settling on this.

Imho this should be merged anyway because it fixes the icu static mingw builds that are currently broken and does not build at all.

longnguyen2004 · 2021-07-03T11:12:24Z

@LiteratimBi Can you apply my suggestion in #1733 (comment) before moving on? This is a workaround for the cross compiling problem I had before.

autoantwort · 2021-08-21T13:19:22Z

Seems like @LiteratimBi does not respond anymore

srl295 · 2023-01-06T18:32:43Z

@longnguyen2004 @autoantwort do you want to make a new PR?

This comment has been minimized.

Sign in to view

LiteratimBi marked this pull request as draft May 26, 2021 13:01

LiteratimBi marked this pull request as ready for review May 26, 2021 13:32

markusicu assigned jefgen May 26, 2021

markusicu requested a review from srl295 May 26, 2021 17:59

jefgen reviewed May 26, 2021

View reviewed changes

jefgen mentioned this pull request May 26, 2021

ICU-21627 - Fix Makefile bugs #1732

Closed

autoantwort reviewed May 26, 2021

View reviewed changes

LiteratimBi marked this pull request as draft May 27, 2021 06:57

srl295 previously approved these changes May 27, 2021

View reviewed changes

LiteratimBi marked this pull request as ready for review May 27, 2021 15:22

ICU-21628 MinGW/Windows incorrect build

ee2701e

See unicode-org#1733

LiteratimBi dismissed srl295’s stale review via ee2701e May 27, 2021 15:24

LiteratimBi force-pushed the mingw-make branch from a881ab9 to ee2701e Compare May 27, 2021 15:24

LiteratimBi requested review from jefgen and srl295 May 27, 2021 15:27

longnguyen2004 suggested changes May 28, 2021

View reviewed changes

longnguyen2004 mentioned this pull request May 31, 2021

[icu] fix mingw builds microsoft/vcpkg#18175

Closed

dg0yt mentioned this pull request Sep 19, 2021

[icu] Fix mingw builds microsoft/vcpkg#20253

Merged

		@@ -198,7 +198,7 @@ ifneq ($(ENABLE_STATIC),)
		ifeq ($(PKGDATA_MODE),dll)
		# For MinGW, do we want the DLL to go in the bin location?

Uh oh!

Conversation

LiteratimBi commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Mingw* configuration/build/setup has some pitfalls:

1. pkgdata utility produces incorrect datastub filenames when LIBPREFIX is set.

2. Less important, also pkgdata utility:

3. Default build PKGDATA_MODE (= dll ) with ENABLE_STATIC

4. Platform-dependant definitions of linker flags are inconsistent (example)

Uh oh!

CLAassistant commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

jefgen commented May 26, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longnguyen2004 commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiteratimBi commented May 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

longnguyen2004 commented May 27, 2021

Uh oh!

longnguyen2004 commented May 27, 2021

Uh oh!

srl295 left a comment

Choose a reason for hiding this comment

Uh oh!

jira-pull-request-webhook bot commented May 27, 2021

Uh oh!

longnguyen2004 commented May 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

autoantwort commented Jun 5, 2021

Uh oh!

longnguyen2004 commented Jun 6, 2021

Uh oh!

autoantwort commented Jun 8, 2021

Uh oh!

longnguyen2004 commented Jun 8, 2021

Uh oh!

srl295 commented Jun 10, 2021

Uh oh!

autoantwort commented Jun 27, 2021

Uh oh!

longnguyen2004 commented Jun 28, 2021

Uh oh!

autoantwort commented Jul 3, 2021

Uh oh!

longnguyen2004 commented Jul 3, 2021

Uh oh!

autoantwort commented Aug 21, 2021

Uh oh!

srl295 commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LiteratimBi commented May 24, 2021 •

edited

Loading

CLAassistant commented May 24, 2021 •

edited

Loading

longnguyen2004 commented May 27, 2021 •

edited

Loading

LiteratimBi commented May 27, 2021 •

edited

Loading