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

lib/easy_lock.h: include synchapi.h on Windows #8997

Closed
wants to merge 1 commit into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Jun 13, 2022

To make sure the SRWLOCK* macros are present

Follow-up to 23af112

@bagder bagder added the Windows label Jun 13, 2022
@bagder bagder marked this pull request as ready for review Jun 13, 2022
@bagder
Copy link
Member Author

@bagder bagder commented Jun 13, 2022

2>c:\projects\curl\lib\easy_lock.h(27) : fatal error C1083: Cannot open include file: 'synchapi.h': No such file or directory

@jay
Copy link
Member

@jay jay commented Jun 13, 2022

If the target version is >= 0x600 (Vista) then SRWLOCK_INIT should be defined. If it's not defined with that target version there's either a problem with our includes or the user's. I wouldn't ifdef SRWLOCK_INIT as well because it could silently build without thread safety rather than build error which is better.

@bagder
Copy link
Member Author

@bagder bagder commented Jun 13, 2022

If the target version is >= 0x600 (Vista) then SRWLOCK_INIT should be defined

One of the appveyor builds fails on that line where it isn't defined. I have no clue why...

@bagder
Copy link
Member Author

@bagder bagder commented Jun 13, 2022

example build error

@jay
Copy link
Member

@jay jay commented Jun 13, 2022

I've had trouble reproducing these msys2 build environments. In this case I get more errors and I also get the missing SRWLOCK_INIT error. If I include windows.h in easy_lock.h then there's no missing SRWLOCK_INIT error but I don't understand what it is about this specific build that causes this problem and not other builds.

For reference this is the build config:

curl/appveyor.yml

Lines 158 to 171 in 9651198

- APPVEYOR_BUILD_WORKER_IMAGE: "Visual Studio 2022"
BUILD_SYSTEM: CMake
PRJ_GEN: "MSYS Makefiles"
PRJ_CFG: Debug
OPENSSL: OFF
SCHANNEL: OFF
ENABLE_UNICODE: OFF
HTTP_ONLY: OFF
TESTING: ON
SHARED: OFF
DISABLED_TESTS: "!1139 !1501"
ADD_PATH: "C:\\MinGW\\bin;C:\\msys64\\usr\\bin"
MSYS2_ARG_CONV_EXCL: "/*"
BUILD_OPT: -k

This is how I attempted to simulate it on my computer:

set "PATH=C:\MinGW\bin;C:\msys64\usr\bin;%PATH%"
git clone -q --branch=master https://github.com/curl/curl.git C:\projects\curl
cd /d C:\projects\curl
git checkout -qf 965119855d74c918372099bdcf81efa76b7a6988
mkdir C:\projects\curl-build
cd /d C:\projects\curl-build

cmake C:\projects\curl ^
  -G"MSYS Makefiles" ^
  -DCURL_USE_OPENSSL=OFF ^
  -DCURL_USE_SCHANNEL=OFF ^
  -DHTTP_ONLY=OFF ^
  -DBUILD_SHARED_LIBS=OFF ^
  -DBUILD_TESTING=ON ^
  -DCURL_WERROR=ON ^
  -DENABLE_DEBUG=ON ^
  -DENABLE_UNICODE=OFF ^
  -DCMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE="" ^
  -DCMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG="" ^
  -DCMAKE_INSTALL_PREFIX="C:/CURL" ^
  -DCMAKE_BUILD_TYPE=Debug && ^
cmake --build . --config Debug --parallel 2 --clean-first -- -k

The other errors seem to be caused by werror:

In file included from /C/projects/curl/lib/dynbuf.c:23:
/C/projects/curl/lib/curl_setup.h:330:45: error: 'struct _stati64' declared insi
de parameter list will not be visible outside of this definition or declaration
[-Werror]
  330 | #  define struct_stat                struct _stati64
      |                                             ^~~~~~~~
/C/projects/curl/lib/curl_setup.h:336:43: note: in expansion of macro 'struct_st
at'
  336 |    int curlx_win32_stat(const char *path, struct_stat *buffer);
      |                                           ^~~~~~~~~~~
In file included from /C/projects/curl/lib/easy.c:23:
/C/projects/curl/lib/curl_setup.h:330:45: error: 'struct _stati64' declared insi
de parameter list will not be visible outside of this definition or declaration
[-Werror]
  330 | #  define struct_stat                struct _stati64
      |                                             ^~~~~~~~
/C/projects/curl/lib/curl_setup.h:336:43: note: in expansion of macro 'struct_st
at'
  336 |    int curlx_win32_stat(const char *path, struct_stat *buffer);
      |                                           ^~~~~~~~~~~
cc1: all warnings being treated as errors
/C/projects/curl/lib/easy.c: In function 'global_init':
/C/projects/curl/lib/easy.c:171:6: error: implicit declaration of function 'Curl
_win32_init'; did you mean 'Curl_none_init'? [-Werror=implicit-function-declarat
ion]
  171 |   if(Curl_win32_init(flags)) {
      |      ^~~~~~~~~~~~~~~
      |      Curl_none_init
/C/projects/curl/lib/easy.c:171:6: error: nested extern declaration of 'Curl_win
32_init' [-Werror=nested-externs]

I don't know why I get those errors and the CI doesn't.

@bagder bagder added the on-hold label Jun 14, 2022
@jay
Copy link
Member

@jay jay commented Jun 16, 2022

I just revisited this and I still don't know what's causing it. Interesting though HAVE_WINDOWS_H isn't defined which is probably something. edit: Unless... it's not supposed to be defined in msys? hm. Then why is Curl_win32_init called.

@vszakats
Copy link
Member

@vszakats vszakats commented Jun 16, 2022

Based on lines ADD_PATH=C:\MinGW\bin; and Check for working C compiler: C:/MinGW/bin/gcc.exe, the example build might be using old Mingw (not mingw-w64). There is also The C compiler identification is GNU 9.2.0, so I'm not completely certain.

Old Mingw relies on an outdated set of Win32 headers (while mingw-w64 shares them with WINE and are fresh and more accurate). The latest old Mingw headers available for download miss synchapi.h altogether, and also the symbol SRWLOCK_INIT, and even its lower-level equivalent RTL_SRWLOCK_INIT.

In this case it's not realistic to replicate the missing header bits (they are too complicated) in curl sources, so disabling this feature seem to be the only option.

@mback2k
Copy link
Member

@mback2k mback2k commented Jun 16, 2022

And remember msys != mingw builds. msys is a linux emulation layer while mingw is cross-compilation for Windows. So if you compile for msys, then you are basically using emulated Linux APIs and not Windows APIs. Although msys-shell is still relevant if you compile using mingw in order to run autotools and our testsuite. Then Windows binaries (*.exe) are called from the Linux emulated shell environment. Similarily to how WSL can call Windows and Linux binaries intermixed with each other.

@vszakats
Copy link
Member

@vszakats vszakats commented Jun 16, 2022

@mback2k: Indeed. I could be wrong, but I surmised this is a Mingw build, otherwise _WIN32_WINNT wouldn't be defined (for an MSYS one), and then the code part with the warning would not be reached? Does that sound correct?

(This contradicts PRJ_GEN: "MSYS Makefiles", suggesting an MSYS-targeted build though.)

UPDATE: Or perhaps _WIN32_WINNT is manually defined? And to fix it, we'd also need to guard this section with a defined(WIN32) or defined(_WIN32), which surely shouldn't be manually defined and also not predefined in MSYS-mode.

@mback2k
Copy link
Member

@mback2k mback2k commented Jun 16, 2022

You are correct, the build @jay is referring to is actually a MinGW one. The CMake generator specifies the shell, not the compiler it seems.

@jay
Copy link
Member

@jay jay commented Jun 21, 2022

Possibly original mingw is too old for the threaded support. It seems weird that we would use original mingw with msys2, since msys2 comes with the updated mingw-w64. I don't think including synchapi.h is necessary, curl_setup should have already included windows.h which would have included the macro if it was available.

Just to see if it's possible I added a change that reverts the previous changes and defines SRWLOCK and SRWLOCK_INIT for original mingw.

@jay jay force-pushed the bagder/easy-lock-win-include branch from b54276f to fbbaee4 Compare Jun 22, 2022
@vszakats
Copy link
Member

@vszakats vszakats commented Jun 23, 2022

Unless I'm overlooking something, a possible new data point here: It seems we're fighting our own build settings. CMake will default to ENABLE_INET_PTON=ON which will then explicitly set _WIN32_WINNT for Vista. This happens regardless of the environment. So in effect, CMake Windows builds will force Vista builds in all environments, even in those that do not support it, like old-mingw.

@jay's existing fix remains valid (it implements the missing env bits in curl's sources), but to avoid this class of issues, I think it'd be useful to fix CMakeLists.txt to not bump up _WIN32_WINNT in the background by default. And not even conditionally, I'd say. Specifying the build target should be the user's explicit (or the toolchain's implicit) choice. The build flow should follow that choice. (The only exception here is when curl requires a specific minimum build target (XP nowadays). That is fine to enforce.)

@jay
Copy link
Member

@jay jay commented Jun 24, 2022

take2 seems to have solved this so if @bagder doesn't mind I will squash it down (effectively removing his proposed changes) and it can go through CI again and be reviewed.

So in effect, CMake Windows builds will force Vista builds in all environments, even in those that do not support it, like old-mingw.

I don't know the reasons for this. IIRC original mingw has a really low default target like 0x400 or something.

@vszakats
Copy link
Member

@vszakats vszakats commented Jun 24, 2022

@jay: It happens in our own ./CMakeLists.txt (see around option(ENABLE_INET_PTON ...). I think _WIN32_WINNT would best be left alone in it, except perhaps setting it to XP (as the minimum required for curl), when not specified by the user. To me the curl-specific CURL_TARGET_WINDOWS_VERSION configuration also looks more confusing than using the universal -D_WIN32_WINNT=0xnnnn. Latter works in every project and every build-system.

@bagder
Copy link
Member Author

@bagder bagder commented Jun 24, 2022

@jay my changes were useless so please get rid of them!

vszakats added a commit to vszakats/curl that referenced this issue Jun 24, 2022
CMake has a setting `ENABLE_INET_PTON` defaulting to `ON`, which
does nothing else than fixing the Windows build target to Vista.
This was done even when the toolchain did not have Vista support
(e.g. original MinGW), breaking such builds.

On other systems it did not make a user-facing difference, because
libcurl has its own pton() implementation, so it works equally well
with or without Vista's inet_pton().

This patch drops the setting `ENABLE_INET_PTON`. inet_pton() is now
used whenever building for Vista or newer, either when requested
manually via
  `CURL_TARGET_WINDOWS_VERSION=0x600`,
or by default with modern toolchains (e.g. mingw-w64). Older envs
will fall back to curl's pton().

Ref: curl#9027 (comment)
Ref: curl#8997 (comment)

Closes #xxxx
vszakats added a commit to vszakats/curl that referenced this issue Jun 24, 2022
CMake has a setting `ENABLE_INET_PTON` defaulting to `ON`, which
does nothing else than fixing the Windows build target to Vista.
This was done even when the toolchain did not have Vista support
(e.g. original MinGW), breaking such builds.

On other systems it did not make a user-facing difference, because
libcurl has its own pton() implementation, so it works equally well
with or without Vista's inet_pton().

This patch drops the setting `ENABLE_INET_PTON`. inet_pton() is now
used whenever building for Vista or newer, either when requested
manually via
  `CURL_TARGET_WINDOWS_VERSION=0x600`,
or by default with modern toolchains (e.g. mingw-w64). Older envs
will fall back to curl's pton().

Ref: curl#9027 (comment)
Ref: curl#8997 (comment)

Closes #xxxx
vszakats added a commit to vszakats/curl that referenced this issue Jun 24, 2022
- Select Windows XP target only if there was no user attempt to set it
  manually, either via `CURL_TARGET_WINDOWS_VERSION`,
  `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0xnnnn` or
  `-DCMAKE_C_FLAGS=/D_WIN32_WINNT=0xnnnn`.

  `CURL_TARGET_WINDOWS_VERSION` can now be deleted or deprecated, in
  favour of the universal way (above) to set the Windows target.

- CMake has a setting `ENABLE_INET_PTON` defaulting to `ON`, which
  does nothing else than fixing the Windows build target to Vista.
  This was done even when the toolchain did not have Vista support
  (e.g. original MinGW), breaking such builds.

  On other systems it did not make a user-facing difference, because
  libcurl has its own pton() implementation, so it works equally well
  with or without Vista's inet_pton().

  This patch drops the setting `ENABLE_INET_PTON`. inet_pton() is now
  used whenever building for Vista or newer, either when requested
  manually via or by default with modern toolchains (e.g. mingw-w64).
  Older envs will fall back to curl's pton().

  Ref: curl#9027 (comment)
  Ref: curl#8997 (comment)

Closes #xxxx
vszakats added a commit to vszakats/curl that referenced this issue Jun 24, 2022
The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults instead.

- CMake had a setting `ENABLE_INET_PTON` defaulting to `ON`, which
  did nothing else than fixing the Windows build target to Vista.
  This was done even when the toolchain did not have Vista support
  (e.g. original MinGW), breaking such builds.

  On other systems it did not make a user-facing difference, because
  libcurl has its own pton() implementation, so it works equally well
  with or without Vista's inet_pton().

  This patch drops this setting. inet_pton() is now used whenever
  building for Vista or newer, either when requested manually via or
  by default with modern toolchains (e.g. mingw-w64). Older envs will
  fall back to curl's pton().

  Ref: curl#9027 (comment)
  Ref: curl#8997 (comment)

- Deprecate `CURL_TARGET_WINDOWS_VERSION` in favour of the universal
  way of setting the Windows target version: `-D_WIN32_WINNT=0xnnnn`.
  This can be passed to CMake like this:
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0xnnnn`

  `CURL_TARGET_WINDOWS_VERSION` was introduced in early 2021, so it
  is also an option to delete it, if we have an agreement on that.

- When the user did no select a Windows target version manually, stop
  explicitly targeting Windows XP, and instead use whatever the
  toolchain default is.

  This may pose an issue with old toolchains defaulting to pre-XP
  targets. In such case you must manually target Windows XP via:
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501`

Closes #xxxx
vszakats added a commit to vszakats/curl that referenced this issue Jun 24, 2022
The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults instead. This gives back
control to the user. Curl is able to adapt to what's available.
This also brings CMake closer to how autotools and `Makefile.m32`
works in this regard.

- CMake had a setting `ENABLE_INET_PTON` defaulting to `ON`, which
  did nothing else than fixing the Windows build target to Vista.
  This was done even when the toolchain did not have Vista support
  (e.g. original MinGW), breaking such builds.

  On other systems it did not make a user-facing difference, because
  libcurl has its own pton() implementation, so it works equally well
  with or without Vista's inet_pton().

  This patch drops this setting. inet_pton() is now used whenever
  building for Vista or newer, either when requested manually via or
  by default with modern toolchains (e.g. mingw-w64). Older envs will
  fall back to curl's pton().

  Ref: curl#9027 (comment)
  Ref: curl#8997 (comment)

- Deprecate `CURL_TARGET_WINDOWS_VERSION` in favour of the universal
  way of setting the Windows target version: `-D_WIN32_WINNT=0xnnnn`.
  This can be passed to CMake like this:
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0xnnnn`

  `CURL_TARGET_WINDOWS_VERSION` was introduced in early 2021, so it
  is also an option to delete it, if we have an agreement on that.

- When the user did no select a Windows target version manually, stop
  explicitly targeting Windows XP, and instead use whatever the
  toolchain default is.

  This may pose an issue with old toolchains defaulting to pre-XP
  targets. In such case you must manually target Windows XP via:
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501`

Closes #xxxx
vszakats added a commit to vszakats/curl that referenced this issue Jun 24, 2022
The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults instead. This gives back
control to the user. Curl is able to adapt to what's available.
This also brings CMake closer to how autotools and `Makefile.m32`
behaves in this regard.

- CMake had a setting `ENABLE_INET_PTON` defaulting to `ON`, which
  did nothing else than fixing the Windows build target to Vista.
  This was done even when the toolchain did not have Vista support
  (e.g. original MinGW), breaking such builds.

  On other systems it did not make a user-facing difference, because
  libcurl has its own pton() implementation, so it works equally well
  with or without Vista's inet_pton().

  This patch drops this setting. inet_pton() is now used whenever
  building for Vista or newer, either when requested manually via or
  by default with modern toolchains (e.g. mingw-w64). Older envs will
  fall back to curl's pton().

  Ref: curl#9027 (comment)
  Ref: curl#8997 (comment)

- Deprecate `CURL_TARGET_WINDOWS_VERSION` in favour of the universal
  way of setting the Windows target version: `-D_WIN32_WINNT=0xnnnn`.
  This can be passed to CMake like this:
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0xnnnn`

  `CURL_TARGET_WINDOWS_VERSION` was introduced in early 2021, so it
  is also an option to delete it, if we have an agreement on that.

- When the user did no select a Windows target version manually, stop
  explicitly targeting Windows XP, and instead use whatever the
  toolchain default is.

  This may pose an issue with old toolchains defaulting to pre-XP
  targets. In such case you must manually target Windows XP via:
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501`

Closes #xxxx
vszakats added a commit to vszakats/curl that referenced this issue Jun 25, 2022
The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults or manual selection instead.
This gives back control to the user. This also brings CMake closer to
how autotools and `Makefile.m32` behaves in this regard.

- CMake had a setting `ENABLE_INET_PTON` defaulting to `ON`, which did
  nothing else than fixing the Windows build target to Vista. This also
  happened when the toolchain did not have Vista support (e.g. original
  MinGW), breaking such builds.

  In other environments it did not make a user-facing difference,
  because libcurl has its own pton() implementation, so it works well
  with or without Vista's inet_pton().

  This patch drops this setting. inet_pton() is now used whenever
  building for Vista or newer, either when requested manually or by
  default with modern toolchains (e.g. mingw-w64). Older envs will fall
  back to curl's pton().

  Ref: curl#9027 (comment)
  Ref: curl#8997 (comment)

- When the user did no select a Windows target version manually, stop
  explicitly targeting Windows XP, and instead use the toolchain default.

  This may pose an issue with old toolchains defaulting to pre-XP
  targets. In such case you must manually target Windows XP via:
    `-DCURL_TARGET_WINDOWS_VERSION=0x0501`
  or
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501`

Closes #xxxx
jay added a commit that referenced this issue Jul 1, 2022
- Define SRWLOCK symbols missing in some mingw environments.

Closes #8997
@jay jay force-pushed the bagder/easy-lock-win-include branch from fbbaee4 to 905ee02 Compare Jul 1, 2022
- Define SRWLOCK symbols missing in some mingw environments.

Closes #8997
@jay jay force-pushed the bagder/easy-lock-win-include branch from 905ee02 to 59b150a Compare Jul 1, 2022
@jay jay removed the on-hold label Jul 3, 2022
@jay jay closed this in bbffb8c Jul 3, 2022
@jay jay deleted the bagder/easy-lock-win-include branch Jul 3, 2022
vszakats added a commit that referenced this issue Jul 4, 2022
The goal of this patch is to avoid CMake forcing specific Windows
versions and rely on toolchain defaults or manual selection instead.
This gives back control to the user. This also brings CMake closer to
how autotools and `Makefile.m32` behaves in this regard.

- CMake had a setting `ENABLE_INET_PTON` defaulting to `ON`, which did
  nothing else than fixing the Windows build target to Vista. This also
  happened when the toolchain did not have Vista support (e.g. original
  MinGW), breaking such builds.

  In other environments it did not make a user-facing difference,
  because libcurl has its own pton() implementation, so it works well
  with or without Vista's inet_pton().

  This patch drops this setting. inet_pton() is now used whenever
  building for Vista or newer, either when requested manually or by
  default with modern toolchains (e.g. mingw-w64). Older envs will fall
  back to curl's pton().

  Ref: #9027 (comment)
  Ref: #8997 (comment)

- When the user did no select a Windows target version manually, stop
  explicitly targeting Windows XP, and instead use the toolchain default.

  This may pose an issue with old toolchains defaulting to pre-XP
  targets. In such case you must manually target Windows XP via:
    `-DCURL_TARGET_WINDOWS_VERSION=0x0501`
  or
    `-DCMAKE_C_FLAGS=-D_WIN32_WINNT=0x0501`

Reviewed-by: Jay Satiro
Reviewed-by: Marcel Raad
Closes #9046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants