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

Windows: fix Makefile.Windows #197

Closed
wants to merge 13 commits into from
Closed

Windows: fix Makefile.Windows #197

wants to merge 13 commits into from

Conversation

gvanem
Copy link
Contributor

@gvanem gvanem commented Nov 8, 2021

Extend the Windows-build (for CI) with:

  • Add rules for target/client.exe and target/server.exe.
  • src/crustls.h is no longer generated (why should it?)
  • The C-library is now named target/release/rustls_ffi.lib.
  • Reduce warning with -D_CRT_NONSTDC_NO_WARNINGS.

@gvanem
Copy link
Contributor Author

gvanem commented Nov 8, 2021

After fixing the test.yaml file (that now works), there are issues with an #include <sys/uio.h> in tests/client.c.
Ref: https://github.com/rustls/rustls-ffi/runs/4137631017?check_suite_focus=true

Another PR to clean-up the mess in tests/*.[ch] coming soon.

@jsha jsha mentioned this pull request Nov 10, 2021
Copy link
Collaborator

@jsha jsha left a comment

Choose a reason for hiding this comment

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

This looks good, but still fails in CI with:

link -nologo -incremental:no -debug -map -verbose -out:target/client.exe  common.obj client.obj target/release/rustls_ffi.lib advapi32.lib userenv.lib ws2_32.lib > link.tmp
link: unknown option -- n

According to some googling, it sounds like this is most likely because there is a copy of the MinGW linker in the path ahead of the MSVC linker. I'm not sure how best to resolve. If we can figure out the page we can move it aside.

Fix typo.

Co-authored-by: Jacob Hoffman-Andrews <[email protected]>
@gvanem
Copy link
Contributor Author

gvanem commented Nov 10, 2021

... there is a copy of the cygwin linker in the path ahead of the MSVC linker.

Add a del /Q <Cygwin-root>\bin\link.exe 2> NUL? But what is Cygwin's root on Github Actions?

@jsha
Copy link
Collaborator

jsha commented Nov 10, 2021

I added link /h to the build and got this output:

Microsoft (R) Incremental Linker Version 14.29.30136.0
Copyright (C) Microsoft Corporation.  All rights reserved.

LINK : warning LNK4044: unrecognized option '/h'; ignored
LINK : warning LNK4001: no object files specified; libraries used
LINK : warning LNK4068: /MACHINE not specified; defaulting to X64
LINK : fatal error LNK1561: entry point must be defined
Error: Process completed with exit code 1.

So it seems like we actually are getting the right linker? I'm pretty mystified about why we're getting the "unknown option" error.

@jsha jsha mentioned this pull request Nov 11, 2021
@jsha
Copy link
Collaborator

jsha commented Nov 11, 2021

Thanks for doing this! I messed around with GitHub Actions and got things working. I put the resulting fixes in #211. Closing this PR out in favor of that one.

@jsha jsha closed this Nov 11, 2021
@gvanem
Copy link
Contributor Author

gvanem commented Nov 11, 2021

I'm pretty mystified about why we're getting the "unknown option" error.

The unknown option -- n is possibly due to a MAKEFLAGS env-var in some other step.

jsha added a commit that referenced this pull request Nov 12, 2021
Adapted from #197 by gvanem. I squashed the commits from that PR,
and added my own on top.

We need to remove the link.exe provided by MSYS and Git to make sure
they don't interfere with MSVC's link.exe.

Also, add a #define for strncasecmp, which is not available on Windows.
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.

2 participants