fix build on mingw by mattn · Pull Request #446 · h2o/picotls

mattn · 2022-11-30T07:53:39Z

t/cli ignored on mingw build.

mattn · 2022-11-30T09:07:56Z

Ooops, I put meessage "test for mingw" but currently t/cli is not possible to build on mingw since ns_msg is not found on Windows. So I did lies. 🤔

huitema

This PR will break the existing builds of Picotls using Visual Studio. It replaces occurrences of #ifdef _WINDOWS by #ifdef _WIN32, presumably because _WIN32 is already defined in the Min GW environment, but at the cost of breaking the Visual Studio builds. You could achieve the same results while keeping compatibility with the Visual Studio builds if you replace #ifdef _WINDOWS by:

#if defined(_WINDOWS) || defined(_WIN32)

Also, I do not see the results of the CI checks for this PR. Did they run?

huitema · 2022-11-30T22:33:22Z

        make all VERBOSE=1
        make check
+  windows:
+    name: "${{ matrix.name }}"


The name "windows" is a bit confusing, as we also have a direct port on Windows relying on Visual Studio. Maybe use "mingw" for clarity?

I think that this change broke the CI tests -- they did not run at all after that.

Current implementation of t/cli.c have many UNIX-ish codes. So we can not run it. But this can be useful to check build error for future's changes at least.

huitema · 2022-11-30T22:37:46Z

+        ADD_EXECUTABLE(cli t/cli.c lib/pembase64.c)
+        TARGET_LINK_LIBRARIES(cli picotls-openssl picotls-core)
+    ENDIF ()
    ADD_EXECUTABLE(picotls-esni src/esni.c)


I am not sure why you need to exclude "pembase64.c" on Windows builds. It works well with the direct Windows port, with one caveat: you have to define the preprocessor flag "_WINDOWS".

No. t/cli is not possible to build on Visual Studio.

picotls/t/cli.c

Lines 27 to 41 in 7e97d3e

#include <arpa/inet.h>

#include <assert.h>

#include <errno.h>

#include <fcntl.h>

#include <getopt.h>

#include <inttypes.h>

#include <stdio.h>

#include <string.h>

#include <strings.h>

#include <sys/select.h>

#include <sys/socket.h>

#include <sys/stat.h>

#include <sys/time.h>

#include <sys/types.h>

#include <unistd.h>

huitema · 2022-11-30T22:40:13Z


-#ifdef _WINDOWS
+#ifdef _WIN32
 #include <intrin.h>


Please do not change this code, that would break the native Visual Studio implementation.

If you really prefer using the string "_WIN32", then you could write:

#if defined(_WIN32) || defined(_WINDOWS)

You don't need to define _WINDOWS since _WIN32 is always set on Windows. (even if Windows 64 bit)

huitema · 2022-11-30T22:40:27Z

 {
-#ifdef _WINDOWS
+#ifdef _WIN32
  uint32_t r = 0;


Same remark as above.

I replaced all _WINDOWS to _WIN32 since _WINDOWS is not required for mingw and Visual Studio both.

I am dubious about this because the CI checks are not running after your changes in ci.yml. I see that the appveyor tests were succeeding before the commit test for mingw, which implies that the Visual Studio build was verified, so we may well be good. But we will not know until we have CI tests working gain...

The reason the ci didn't run after this change is because I merged this change from another branch.

https://github.com/mattn/picotls/actions

@kazuho could you please re-run the action for latest changes ?

@mattn Mind pushing an empty commit? I think @huitema is wondering if we could rerun CI on appveyor.

Regarding _WIN32 vs. _WINDOWS, to me it seems we can switch to _WIN32.

Official documentation does state that _WIN32 will always be defined by MSVC compiler (https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170). OTOH, _WINDOWS does not appear in the list. I wonder if it is something Vistual Studio sets (or if it is a convention in the project-level setting).

No, it is not something that Visual Studio sets. It is something that I kind of invented 5 years ago as I was trying to make things work. I think that to be complete, we should also edit the Visual Studio projects that use the _WINDOWS and _WINDOWS64 macros. But I could do that in a different PR.

As soon as I see a correct test run, I will approve the changes.

AFAIK, _WIN32 can be used for checking Windows OS from 12 years ago at least. For example, I used _WIN32 in 2010 that works with both compilers.

https://github.com/mattn/gtktweeter

@mattn I do realize that I made a mistake 5 years ago. As I said, I just need to see the test run...

huitema

All tests are passing, including the appveyor tests. Looks good.

huitema · 2022-12-13T04:46:01Z

We just have to resolve the merging conflicts! Maybe merge the latest "main" into this branch?

mattn · 2022-12-13T09:05:42Z

@huitema Thank you. Fixed.

kazuho

Thank you for the changes. I'll take a look, make necessary changes and merge.

fix build on mingw

765a6da

mattn changed the title ~~fix build on mingw~~ [WIP] fix build on mingw Nov 30, 2022

mattn added 6 commits November 30, 2022 17:03

do not include pthread.h

37f4785

use NOT

88679c0

include sys/time.h for mingw

2f615c0

test for mingw

2189000

disable check

be31c45

disable check

c1bd5cd

mattn changed the title ~~[WIP] fix build on mingw~~ fix build on mingw Nov 30, 2022

huitema requested changes Nov 30, 2022

View reviewed changes

bump GitHub Action

8783407

huitema approved these changes Dec 13, 2022

View reviewed changes

mattn added 3 commits December 13, 2022 14:31

Merge branch 'master' into mingw-port

410660f

use _WIN32

d911291

delete target picotls-esni

ff4d2b2

kazuho reviewed Dec 16, 2022

View reviewed changes

	#include <arpa/inet.h>
	#include <assert.h>
	#include <errno.h>
	#include <fcntl.h>
	#include <getopt.h>
	#include <inttypes.h>
	#include <stdio.h>
	#include <string.h>
	#include <strings.h>
	#include <sys/select.h>
	#include <sys/socket.h>
	#include <sys/stat.h>
	#include <sys/time.h>
	#include <sys/types.h>
	#include <unistd.h>

Conversation

mattn commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattn commented Nov 30, 2022

Uh oh!

huitema left a comment

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!

mattn Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

huitema left a comment

Choose a reason for hiding this comment

Uh oh!

huitema commented Dec 13, 2022

Uh oh!

mattn commented Dec 13, 2022

Uh oh!

kazuho left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattn commented Nov 30, 2022 •

edited

Loading

mattn Nov 30, 2022 •

edited

Loading