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

Jump to bottom

cmake: enable more detection on Windows #9687

Closed
wants to merge 2 commits into from
Closed

cmake: enable more detection on Windows #9687

wants to merge 2 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 10, 2022 •
edited

Enable HAVE_UNISTD_H, HAVE_STRTOK_R and HAVE_STRCASECMP detection on Windows, instead of having predefined values.

With these features detected correctly, CMake Windows builds get closer to the autotools and config-win32.h ones.

This also fixes detecting HAVE_FTRUNCATE correctly, which required unistd.h.

Fixing ftruncate() in turn causes a build warning/error with legacy MinGW/MSYS1 due to an offset type size mismatch. This env misses to detect HAVE_FILE_OFFSET_BITS, which may be a reason. This patch force-disables HAVE_FTRUNCATE for this platform.

Reviewed-by: Daniel Stenberg

Closes #9687

@vszakats vszakats added build cmake Windows Windows-specific labels Oct 10, 2022
@vszakats vszakats removed the build label Oct 10, 2022
@vszakats
Copy link
Member Author

vszakats commented Oct 10, 2022 •
edited

The good news is that this also fixed 1 to correctly detect HAVE_FTRUNCATE with modern MinGW.

The bad news is that this in turn breaks legacy MinGW/MSYS1 builds because of the enabled ftruncate() 2:

[ 86%] Building C object src/CMakeFiles/curl.dir/tool_operhlp.c.obj
C:/projects/curl/src/tool_operate.c: In function 'post_per_transfer':
C:/projects/curl/src/tool_operate.c:576:48: error: conversion from 'curl_off_t' {aka 'long long int'} to 'off_t' {aka 'long int'} may change value [-Werror=conversion]
  576 |         if(ftruncate(fileno(outs->stream), outs->init)) {
      |                                            ~~~~^~~~~~
[ 86%] Building C object src/CMakeFiles/curl.dir/tool_panykey.c.obj
cc1.exe: all warnings being treated as errors
make[2]: *** [src/CMakeFiles/curl.dir/build.make:456: src/CMakeFiles/curl.dir/tool_operate.c.obj] Error 1
[ 86%] Building C object src/CMakeFiles/curl.dir/tool_paramhlp.c.obj
[ 86%] Building C object src/CMakeFiles/curl.dir/tool_parsecfg.c.obj

Is there someone with access to this old env with an idea how to fix it?

Footnotes

  1. Detection of HAVE_UNISTD_H is required to include unistd.h, which is required to successfully detect ftruncate(). ↩

  2. https://ci.appveyor.com/project/curlorg/curl/builds/45020994/job/2udxojuyoxuuoi3n ↩

@vszakats
cmake: enable more detection on Windows
9965f7d 
Enable `HAVE_UNISTD_H`, `HAVE_STRTOK_R` and `HAVE_STRCASECMP` detection
on Windows, instead of having predefined values.

With these features detected correctly, CMake Windows builds get closer
to the autotools and `config-win32.h` ones.

This also fixes detecting `HAVE_FTRUNCATE` correctly, which required
`unistd.h`.

Fixing `ftruncate()` in turn causes a build warning/error with legacy
MinGW/MSYS1 due to an offset type size mismatch. This env misses to
detect `HAVE_FILE_OFFSET_BITS`, which may be a reason. This patch
force-disables `HAVE_FTRUNCATE` for this platform.
@vszakats
fix regression with legacy-MinGW/MSYS1
bd285e5
@vszakats
Copy link
Member Author

Updated to keep ftruncate() disabled for old MinGW.

@vszakats vszakats mentioned this pull request Oct 10, 2022
Closed
@vszakats vszakats closed this in 474a947 Oct 11, 2022
@vszakats vszakats deleted the cmakewin1 branch October 11, 2022 07:54
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 11, 2022
@vszakats
curl-cmake.sh: simplify after upstream curl fixes [ci skip]
8de1c92 
Upcoming curl release received fixes to correctly detect these build
flags, so drop manual settings for them:

- `HAVE_UNISTD_H` and with it: `HAVE_FTRUNCATE`
- `HAVE_STRTOK_R`
- `HAVE_STRCASECMP`
- `HAVE_INET_NTOP`

Ref: curl/curl#9687
Ref: curl/curl#9689
obonaventure pushed a commit to mptcp-apps/curl that referenced this pull request Oct 12, 2022
@vszakats @obonaventure
cmake: enable more detection on Windows
1b7810b 
Enable `HAVE_UNISTD_H`, `HAVE_STRTOK_R` and `HAVE_STRCASECMP` detection
on Windows, instead of having predefined values.

With these features detected correctly, CMake Windows builds get closer
to the autotools and `config-win32.h` ones.

This also fixes detecting `HAVE_FTRUNCATE` correctly, which required
`unistd.h`.

Fixing `ftruncate()` in turn causes a build warning/error with legacy
MinGW/MSYS1 due to an offset type size mismatch. This env misses to
detect `HAVE_FILE_OFFSET_BITS`, which may be a reason. This patch
force-disables `HAVE_FTRUNCATE` for this platform.

Reviewed-by: Daniel Stenberg

Closes curl#9687
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
cmake Windows Windows-specific
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vszakats @bagder