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

fix build on mingw #446

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

fix build on mingw #446

wants to merge 11 commits into from

Conversation

mattn
Copy link
Contributor

@mattn mattn commented Nov 30, 2022

t/cli ignored on mingw build.

@mattn mattn changed the title fix build on mingw [WIP] fix build on mingw Nov 30, 2022
@mattn
Copy link
Contributor Author

mattn commented Nov 30, 2022

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. 🤔

@mattn mattn changed the title [WIP] fix build on mingw fix build on mingw Nov 30, 2022
Copy link
Collaborator

@huitema huitema left a comment

Choose a reason for hiding this comment

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

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?

@@ -76,3 +76,39 @@ jobs:
cmake .. -DOPENSSL_ROOT_DIR="$(brew --prefix ${OPENSSL})"
make all VERBOSE=1
make check
windows:
name: "${{ matrix.name }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mattn mattn Nov 30, 2022

Choose a reason for hiding this comment

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

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.

CMakeLists.txt Outdated
IF (NOT WIN32)
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>

@@ -18,7 +18,7 @@
#include <stdint.h>
#include <stddef.h>

#ifdef _WINDOWS
#ifdef _WIN32
#include <intrin.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -298,7 +298,7 @@ static inline void copy_bytes_unaligned(uint8_t *out, const uint8_t *in, size_t

static inline uint32_t count_trailing_zeroes(uint32_t x)
{
#ifdef _WINDOWS
#ifdef _WIN32
uint32_t r = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same remark as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

@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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@huitema huitema left a comment

Choose a reason for hiding this comment

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

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

@huitema
Copy link
Collaborator

huitema commented Dec 13, 2022

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

@mattn
Copy link
Contributor Author

mattn commented Dec 13, 2022

@huitema Thank you. Fixed.

Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

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

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