Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check __MINGW32__ instead of __MINGW64__ #2469

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

KristofferC
Copy link
Contributor

Both __MINGW32__ and __MINGW64__ are defined on 64-bit mingw but only __MINGW32__ on 32-bit mingw. Without this, one hits something like

[13:07:17] In file included from /workspace/srcdir/notcurses-3.0.0/src/lib/internal.h:10:0,
[13:07:17]                  from /workspace/srcdir/notcurses-3.0.0/src/lib/automaton.c:2:
[13:07:17] /workspace/srcdir/notcurses-3.0.0/src/compat/compat.h:72:10: fatal error: poll.h: No such file or directory
[13:07:17]  #include <poll.h>
[13:07:17]           ^~~~~~~~

when trying to compile for 32-bit windows.

@dankamongmen
Copy link
Owner

looks like we need a rebase, but yeah, agreed

@KristofferC
Copy link
Contributor Author

Rebased

@dankamongmen dankamongmen merged commit a3c3793 into dankamongmen:master Dec 13, 2021
@KristofferC KristofferC deleted the kc/mingw32 branch December 13, 2021 17:23
@michaelsbradleyjr
Copy link
Collaborator

michaelsbradleyjr commented Dec 13, 2021

Sorry for the post-merge question/comment, but to which MSYS2 environment is this applicable? Per #2352 (comment), notcurses only supports UCRT because it's the only one with support for UTF8. Is this for the sake of a 32-bit MSYS2 Clang environment, or...?

@KristofferC
Copy link
Contributor Author

I am working on creating a wrapper for notcurses for the Julia language. In short, in Julia, to provide binaries to users, we cross-compile binaries to a large number of architectures (from a single Alpine linux system) and the Julia package manager downloads the correct pre-compiled binary for the user's architecture which is then dlopened to be called into by the wrapper package. The issue that this PR fixed came up when building it for the i686-w64-mingw32 target. For more context see JuliaPackaging/Yggdrasil#4059.

If there are other reasons why the resulting artifacts would not properly work on that target, then indeed this PR was sort of pointless. But I can't test it until I have the library built, heh.

@michaelsbradleyjr
Copy link
Collaborator

michaelsbradleyjr commented Dec 13, 2021

Check the the MSYS2 environments page. The CLANG32 environment is advertised as using ucrt for the C library, while the MINGW32 environment is advertised as using msvcrt for the C library; the latter likely won't work because of lack of proper support for UTF8.

However, in practice, naming in the the world of MSYS2 can be a bit confusing, e.g. __MINGW32__ might be defined in the CLANG32 environment because we know for sure that __MINGW64__ is defined in the UCRT64 environment. Anyway, it's a worth shot and maybe it will work out, I definitely don't intend to discourage your efforts, and best of luck!

@dankamongmen
Copy link
Owner

However, in practice, naming in the the world of MSYS2 can be a bit confusing, e.g. __MINGW32__ might be defined in the CLANG32 environment because we know for sure that __MINGW64__ is defined in the UCRT64 environment. Anyway, it's a worth shot and maybe it will work out, I definitely don't intend to discourage your efforts, and best of luck!

i'm pretty sure that CI success verifies that we're ok here

@michaelsbradleyjr
Copy link
Collaborator

michaelsbradleyjr commented Dec 13, 2021

i'm pretty sure that CI success verifies that we're ok here

Based on what @KristofferC reported:

Both __MINGW32__ and __MINGW64__ are defined on 64-bit mingw....

It definitely won't mess anything up that's already working, so it's not a bad change, for sure. I'm not suggesting it shouldn't have been merged.

I was just asking some questions to try and head off misunderstandings in the case that the MINGW32 environment, which uses msvcrt instead of ucrt, doesn't work (it probably won't because of lack of UTF8 support).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants