[tre, libmagic] Windows + mingw support by longnguyen2004 · Pull Request #17769 · microsoft/vcpkg

longnguyen2004 · 2021-05-10T10:01:36Z

After a year...

Describe the pull request

What does your PR fix?

Fixes [libmagic] Clean build failure on Windows #11832
Also fixes a small bug in vcpkg_configure_make where uuid isn't removed on mingw triplets.
Which triplets are supported/not supported? Have you updated the CI baseline?

Windows + mingw triplets, Yes
Does your PR follow the maintainer guide?

Yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

dg0yt · 2021-05-10T10:15:07Z

scripts/cmake/vcpkg_configure_make.cmake

IIRC this is what I initially proposed in #17137, but it didn't seem to find acceptance.

So now we have 2 different approaches to the same problem, let's see which one gets accepted first.
Since on MSVC triplets, uuid.lib gets stripped entirely, I'm doing the same with mingw.

longnguyen2004 · 2021-05-10T11:07:36Z

Also, now that we have host dependencies, theoretically libmagic can be cross compiled as well, but that's for another day.

ports/libmagic/0001-Use-libtre.patch

Neumann-A · 2021-05-10T11:16:20Z

ports/libmagic/0001-Use-libtre.patch

pass -I"<vcpkginclude>/tre" instead of patching

Can you help me with passing it to MSVC? No matter what I do, it doesn't seem to get through.

Now that I think about it, MXE users will have a problem with this, since VCPKG_C_FLAGS are not passed through.

Neumann-A · 2021-05-10T11:17:11Z

ports/libmagic/0002-Change-zlib-lib-name-to-match-CMake-output.patch

AC_SEARCH_LIBS ! due to debug suffix on windows. Search for z zlib zlibd

Search for z zlib zlibd

Is this the right order for finding zlibd before zlib in debug builds?

We don't have to worry about which one gets found first, since only one will be available in each build.

as longnguyen2004 said. This is not cmake where we have to deal with both prefixes/libdirs being visible.

For z, fair enough. But not in general. This is part of the "gdal completely broken on Linux" story. On Linux, there is always a system path, in addition to the debug path. And generally there is no control about the user environment (manifest mode, registries, overlay ports).

@dg0yt We can probably uses -nostdinc/-nostdlib and manually add back in the necessary include and library paths, but that gets tedious fast.

JackBoosY · 2021-05-11T09:51:08Z

What you made contains the same changes as #17137.

longnguyen2004 · 2021-05-11T10:04:45Z

@JackBoosY I'll keep my change here for the sake of CI, when one of them is merged I'll remove it.

dg0yt · 2021-05-11T17:33:01Z

file requires a native file of the same version to compile the magic file, so we have to add file itself as a host dependency (is it possible?)

Maybe a host dependency of a non-core default feature on a non-default feature?

No way to pass the tool path explicitly instead of modifying PATH?

longnguyen2004 · 2021-05-11T17:37:46Z

It's possible for a port to depend on itself (I just figured it out after reading the docs...)

No way to pass the tool path explicitly instead of modifying PATH?

Yeah, but that's not a problem since it's pretty straightforward, also it's the build system that needs it, not vcpkg.

ports/libmagic/portfile.cmake

longnguyen2004 · 2021-05-13T08:57:38Z

@Neumann-A Can you check x86-windows and see if there's a workaround?
The build system detects the correct file, but file --version returns D:\installed\x64-windows\tools\libmagic\bin\file.exe-5.40 for some reason, while the build expect file-x.xx, thus it fails to detect the version.
Removing the whole check might be good, since we're going to build file natively after all, but that sounds sketchy to me.

EDIT: https://github.com/file/file/blob/03b6dcb4a24455207ef4094560c334fbc38875bd/src/file.c#L204
This line only checks for '/' in argv[0], which is not the case on Windows (?)

Neumann-A · 2021-05-13T13:28:53Z

This line only checks for '/' in argv[0], which is not the case on Windows (?)

Yeah probably need to fix that + adding ${EXEEXT} in the Makefile doing the check.
Also install the magic.mgc file into tools/libmagic/share/misc so that the tool automatically picks it up

longnguyen2004 · 2021-05-14T05:27:12Z

Eh...git rebase -i messes everything up :)

strega-nil-ms

This looks good to me, just needs a minor change

ports/libmagic/portfile.cmake

dg0yt · 2021-05-16T13:19:47Z

On mingw, the build failed:

C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ./.libs/libmagic.a(compress.o):compress.c:(.text+0xae): undefined reference to `ioctl'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ./.libs/libmagic.a(compress.o):compress.c:(.text+0x108): undefined reference to `__imp_select'
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/10.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: ./.libs/libmagic.a(compress.o):compress.c:(.text+0x12e): undefined reference to `ioctl'
collect2.exe: error: ld returned 1 exit status

longnguyen2004 · 2021-05-16T13:43:22Z

@dg0yt Try applying this MSYS2 patch https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-file/0001-file-5.38-mingw-build.patch

longnguyen2004 · 2021-05-18T02:03:50Z

@dg0yt Is the problem solved?

dg0yt · 2021-05-18T05:10:51Z

@dg0yt Is the problem solved?

Yes, mingw build succeeds now. 👍

longnguyen2004 · 2021-05-27T06:43:54Z

Now that #17137 is merged, I'll take that fix off of my PR.

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>

strega-nil-ms · 2021-05-28T17:09:53Z

Alright, great, thanks for the awesome work @longnguyen2004 :)

longnguyen2004 force-pushed the libmagic-windows branch from acb6ee8 to 58d81e3 Compare May 10, 2021 10:06

dg0yt reviewed May 10, 2021

View reviewed changes

Neumann-A reviewed May 10, 2021

View reviewed changes

ports/libmagic/0001-Use-libtre.patch Outdated Show resolved Hide resolved

Neumann-A reviewed May 10, 2021

View reviewed changes

longnguyen2004 mentioned this pull request May 11, 2021

Strip "-l" suffix before removing "uuid" from libraries #17780

Closed

dg0yt mentioned this pull request May 11, 2021

[vcpkg] Fix lib uuid handling for x64-mingw-dynamic #17137

Merged

longnguyen2004 force-pushed the libmagic-windows branch 3 times, most recently from 12798c7 to a640f58 Compare May 11, 2021 06:46

JackBoosY self-assigned this May 12, 2021

JackBoosY added the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label May 12, 2021

JackBoosY suggested changes May 13, 2021

View reviewed changes

ports/libmagic/portfile.cmake Outdated Show resolved Hide resolved

JackBoosY added the requires:author-response label May 13, 2021

longnguyen2004 force-pushed the libmagic-windows branch from 9e403d2 to 49adac8 Compare May 14, 2021 05:23

longnguyen2004 force-pushed the libmagic-windows branch 3 times, most recently from f899987 to 4db21fb Compare May 14, 2021 13:17

strega-nil-ms approved these changes May 14, 2021

View reviewed changes

ports/libmagic/portfile.cmake Outdated Show resolved Hide resolved

Neumann-A reviewed May 14, 2021

View reviewed changes

ports/libmagic/portfile.cmake Outdated Show resolved Hide resolved

Neumann-A reviewed May 14, 2021

View reviewed changes

ports/libmagic/portfile.cmake Outdated Show resolved Hide resolved

JackBoosY removed the requires:author-response label May 15, 2021

JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. requires:discussion labels May 19, 2021

longnguyen2004 and others added 9 commits May 27, 2021 13:48

[tre] mingw support

cf17992

[libmagic] Windows support

daed3f2

[libmagic] Apply @Neumann-A suggestions

908f50f

[libmagic] Add host dependency

432b022

Co-authored-by: Jack·Boos·Yu <47264268+JackBoosY@users.noreply.github.com>

[libmagic] Copy the magic file to the corresponding tools dir

af3f3fc

[ci baseline] libmagic now passes

0b03ef5

Update ports/libmagic/portfile.cmake

17a4952

[libmagic] Import patch from MSYS2

629d860

[vcpkg baseline] Update libmagic, tre

9171c81

longnguyen2004 force-pushed the libmagic-windows branch from 45ddf7d to 9171c81 Compare May 27, 2021 06:58

Merge remote-tracking branch 'origin' into libmagic-windows

dd246b9

JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. requires:discussion labels May 27, 2021

JackBoosY approved these changes May 28, 2021

View reviewed changes

strega-nil-ms merged commit a29126f into microsoft:master May 28, 2021

Conversation

longnguyen2004 commented May 10, 2021

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longnguyen2004 commented May 10, 2021

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

JackBoosY commented May 11, 2021

Uh oh!

longnguyen2004 commented May 11, 2021

Uh oh!

dg0yt commented May 11, 2021

Uh oh!

longnguyen2004 commented May 11, 2021

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Neumann-A commented May 13, 2021

Uh oh!

longnguyen2004 commented May 14, 2021

Uh oh!

strega-nil-ms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dg0yt commented May 16, 2021

Uh oh!

longnguyen2004 commented May 16, 2021

Uh oh!

longnguyen2004 commented May 18, 2021

Uh oh!

dg0yt commented May 18, 2021

Uh oh!

longnguyen2004 commented May 27, 2021

Uh oh!

strega-nil-ms commented May 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

longnguyen2004 commented May 13, 2021 •

edited

Loading