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

Add mingw32-w64/wine CI build #922

Merged
merged 2 commits into from
May 2, 2021
Merged

Conversation

sipa
Copy link
Contributor

@sipa sipa commented Apr 17, 2021

No description provided.

.cirrus.yml Outdated Show resolved Hide resolved
@real-or-random
Copy link
Contributor

Nice!

Concept ACK

@sipa sipa force-pushed the 202104_mingw branch 2 times, most recently from 5f4e39b to 45218a2 Compare April 18, 2021 17:11
@real-or-random
Copy link
Contributor

ok it seems there was a reason why -static was there:
https://cirrus-ci.com/task/5119836217933824?logs=test#L185
libtool: warning: undefined symbols not allowed in x86_64-w64-mingw32 shared libraries; building static only (and a few other messages then when linking the benchmarks)

Maybe ./configure --disable-shared does the job then without specifying CFLAGS. In the end it doesn't matter for CI, I was just curious because we could use this to add build docs for mingw.

@real-or-random
Copy link
Contributor

Can you also change
file *tests || true to file *tests* || true ? Otherwise it won't match the ".exe"...

@sipa
Copy link
Contributor Author

sipa commented Apr 18, 2021

@real-or-random That wasn't the reason actually. I tried this first in the minisketch repository (see sipa/minisketch#33), and there the issue is that C++ builds to building against shared libgcc by default (something related to being able to catch exceptions across libraries), but I couldn't get that to work with Wine. For C, static libgcc is the default, so this problem doesn't occur. I've just removed it.

@real-or-random real-or-random mentioned this pull request Apr 19, 2021
6 tasks
@real-or-random
Copy link
Contributor

Ok makes sense. We can take care of more improvements to mingw later then (#923).

ACK mod the file thing.

@sipa
Copy link
Contributor Author

sipa commented Apr 24, 2021

Can you also change
file *tests || true to file tests || true ? Otherwise it won't match the ".exe"...

Done.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e2750b1

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK e2750b1

@jonasnick
Copy link
Contributor

Hm any idea why wine complains so loudly (on cirrus). Is that supposed to be ignored?

it looks like wine32 is missing, you should install it.
as root, please execute "apt-get install wine32"

@sipa
Copy link
Contributor Author

sipa commented Apr 29, 2021

It's only testing 64-bit binaries, so it doesn't need wine32 I think. We could probably just add it to the dockerfile to silence it.

@real-or-random
Copy link
Contributor

AFAIU wine32 would be required to run 32-bit binaries, which we don't need.

I'm not sure why Wine warns. Maybe because 32bit is still a common thing on Windows and it's an issue that many people run into?

.cirrus.yml Outdated Show resolved Hide resolved
ci/linux-debian.Dockerfile Show resolved Hide resolved
ci/linux-debian.Dockerfile Outdated Show resolved Hide resolved
@real-or-random
Copy link
Contributor

@sipa Ok, I played around with the docker container. If I'm not mistaken:

  • If you install wine (which pulls in wine64 as dependency), then wine64 <cmd> (or wine64-stable <cmd>) works without the warning
  • If you install just wine64, then WINESERVER=/usr/lib/wine/wineserver64 wine64-stable <cmd> works without the warning. (Indeed, without the env variable wineserver can't be found.)

Both are fine but the first is more natural...

By the way, wine xcopy works better as a dummy command. There's no true on windows.

@sipa
Copy link
Contributor Author

sipa commented Apr 30, 2021

@real-or-random Ok, used your first variant, and also changed true -> xcopy.

@real-or-random
Copy link
Contributor

real-or-random commented May 1, 2021

Ok, great. Now there seems to be one final hiccup:

WARNING: could not read 16 bytes from /dev/urandom; falling back to insecure PRNG

( https://cirrus-ci.com/task/4912068852711424?logs=test#L258 )

I can sometimes reproduce this in the Docker image and then the seed always has some random bytes in front followed by zeros. You can cherry-pick this fix: real-or-random@625b1aa, see https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-160#generic-text-routine-mappings.

This makes a difference with mingw builds on Wine, where the subsequent
fread() may abort early in the default text mode.

The Microsoft C docs say:
"In text mode, CTRL+Z is interpreted as an EOF character on input."
@sipa
Copy link
Contributor Author

sipa commented May 2, 2021

@real-or-random Cherry picked.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ed5a199

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK ed5a199

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