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

<sys/select.h> missing include on cygwin #1925

Closed
ifette opened this Issue Sep 26, 2017 · 13 comments

Comments

Projects
None yet
3 participants
@ifette

ifette commented Sep 26, 2017

I realize this was discussed in the past in #749 however the issue has re-surfaced and I'm hoping people will re-consider.

I tried compiling a very simple program with curl using -std=c++14 under 64-bit cygwin with gcc 6.4.0. When compiling with just g++ main.cpp -lcurl everything is fine, however if I try to use c++14 as the dialect (g++ main.cpp -lcurl -std=c++14) familiar problems creep up

In file included from /usr/include/curl/curl.h:2547:0,
from main.cpp:10:
/usr/include/curl/multi.h:155:40: error: ‘fd_set’ has not been declared
fd_set *read_fd_set,
^~~~~~
/usr/include/curl/multi.h:156:40: error: ‘fd_set’ has not been declared
fd_set *write_fd_set,
^~~~~~
/usr/include/curl/multi.h:157:40: error: ‘fd_set’ has not been declared
fd_set *exc_fd_set,
^~~~~~

This is resolved by manually including <sys/select.h> before including <curl/curl.h>

This was discussed in the curl project in the past (#749) where it was determined that it was caused by a cygwin bug which was addressed in https://cygwin.com/git/gitweb.cgi?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/sys/types.h;h=c9f0fc7f3a9ca420c2372c9af42ce2a0e63e3b1c;hb=ee97c4b22491b205fd3b7697e03c909e02b652d3

I brought this up on the cygwin mailing list, and the response there (which, for what it's worth I happen to agree with) is that if curl is using fd_set it should be explicitly including <sys/select.h>.

If you look at the spec (http://pubs.opengroup.org/onlinepubs/009696899/basedefs/sys/select.h.html) it is indeed sys/select.h that defines the fd_set type. The docs for sys/types.h (http://pubs.opengroup.org/onlinepubs/009696899/basedefs/sys/types.h.html) make no guarantee about fd_set being included.

Given that this is a one line change that should have zero adverse impact on other platforms, is it possible to have curl explicitly include sys/select.h? (I haven't done any exhaustive testing to see if other platforms are impacted by the current state of the world when trying to compile with -std=c++14).

Simple repro:

#include
#include <curl/curl.h>

using namespace std;

int main() {

CURL *curl = curl_easy_init();
if(curl) {
  CURLcode res;
  curl_easy_setopt(curl, CURLOPT_URL, "http://example.com");
  res = curl_easy_perform(curl);
  curl_easy_cleanup(curl);
}

}

@bagder

This comment has been minimized.

Member

bagder commented Sep 26, 2017

So you're using a version of cygwin without the fix?

@bagder

This comment has been minimized.

Member

bagder commented Sep 26, 2017

The problem for us is that not all systems that libcurl runs and builds on have a <sys/select.h> header file. We cannot include it unconditionally. We therefore have only included it on the platforms that need it (and thus have it). cygwin has never needed it until they changed their header files as mentioned in #749.

@ifette

This comment has been minimized.

ifette commented Sep 26, 2017

The version of cygwin I'm using compiles fine when using g++ with the default language profile, but when you specify -std=c++14 it does not work. It's not clear to me that the cygwin change from a year ago affected behavior when g++ is in standard c++14 mode.

Specifically, in the default mode, __BSD_VISIBLE is defined as 1 whereas in -std=c++14 it's defined as 0 - which in cygwin causes sys/select.h not to be included from sys/types.

Looking at Apple, they seem to include as a hack for a backward compatibility but still advise including sys/select explicitly. From their sys/types.h:

#if !defined(_POSIX_C_SOURCE) || defined(_DARWIN_C_SOURCE)
/*

  • This code is present here in order to maintain historical backward
  • compatability, and is intended to be removed at some point in the
  • future; please include <sys/select.h> instead.
    */
    #include <sys/_types/_fd_def.h>

Out of curiosity (not trying to doubt you, just trying to understand) what platforms have sys/types.h but not sys/select.h? The standard is pretty clear that fd_set is in select not types, so I'm trying to understand what kind of a change would be required to accommodate this and if such a patch would be welcome if it wouldn't break the other platforms you're mentioning.

@HBBroeker

This comment has been minimized.

HBBroeker commented Sep 26, 2017

The problem here is that the decision logic in <curl/curl.h> is backwards. You're effectively ignoring 20 years of standardization effort to pamper some hopelessly outdated platforms. The resulting construct

/* HP-UX systems version 9, 10 and 11 lack sys/select.h and so does oldish
   libc5-based Linux systems. Only include it on systems that are known to
   require it! */
#if defined(_AIX) || defined(__NOVELL_LIBC__) || defined(__NetBSD__) || \
    defined(__minix) || defined(__SYMBIAN32__) || defined(__INTEGRITY) || \
    defined(ANDROID) || defined(__ANDROID__) || defined(__OpenBSD__) || \
   (defined(__FreeBSD_version) && (__FreeBSD_version < 800000))
#include <sys/select.h>
#endif

is so clumsy to the point of being ridiculous. I mean, come on: libc5 Linux and HP-sUX? Seriously?

This basically has to treat every well-behaved platform in the world as an exception to the rule, because it has the rule the wrong way round. The rule has to be: include <sys/select.h>. Even just installing a fake <sys/select.h>, on platforms that have none would be preferrable to that construct.

@bagder

This comment has been minimized.

Member

bagder commented Sep 26, 2017

@ifette, so does it work for you with something like this?

diff --git a/include/curl/curl.h b/include/curl/curl.h
index 501e3d19b..7139a3311 100644
--- a/include/curl/curl.h
+++ b/include/curl/curl.h
@@ -72,10 +72,11 @@
    libc5-based Linux systems. Only include it on systems that are known to
    require it! */
 #if defined(_AIX) || defined(__NOVELL_LIBC__) || defined(__NetBSD__) || \
     defined(__minix) || defined(__SYMBIAN32__) || defined(__INTEGRITY) || \
     defined(ANDROID) || defined(__ANDROID__) || defined(__OpenBSD__) || \
+    defined(__CYGWIN__) || \
    (defined(__FreeBSD_version) && (__FreeBSD_version < 800000))
 #include <sys/select.h>
 #endif
 
 #if !defined(WIN32) && !defined(_WIN32_WCE)
@bagder

This comment has been minimized.

Member

bagder commented Sep 26, 2017

@HBBroeker wrote:

it has the rule the wrong way round

Yes it does, but it was made like that a very long time ago and then it slowly grew from there. Every little change since then has been done to fix the build for one platform a time where the bug reporter was present and could verify the fix. Reversing the check would instead risk breaking platforms without users around. This is true this time as well.

@bagder bagder added the build label Sep 27, 2017

@bagder bagder changed the title from <sys/select.h> missing include to <sys/select.h> missing include on cygwin Sep 27, 2017

@HBBroeker

This comment has been minimized.

HBBroeker commented Sep 27, 2017

Every little change since then has been done to fix the build for one platform a time

I feel your pain. I have a similar construct I'm not proud of, in one of my own projects.

But I maintain that you've been following the wrong logic about it. Even if the decision is to assume all platforms as broken until proven otherwise, at least the conditions for being added to that list of exceptions should not be "known to require it!", but rather "have it". In other words, if your autoconf found D_HAVE_SYS_SELECT_H, then you should #include it.

Cygwin is kept from doing the right thing by projects like yours that rely on incorrect assumptions.

@ifette

This comment has been minimized.

ifette commented Sep 27, 2017

@bagder As someone who to you is a probably random person from the internet, I'll stay out of the debate around what is the "correct" fix, and merely say that the change you posted does enable me to build correctly on cygwin using -std=c++14

@bagder

This comment has been minimized.

Member

bagder commented Sep 27, 2017

Thanks @ifette! And for the record: I listen, respect and try to cater to everyone's opinions and feedback. We're all just "random persons from the internet" in the first step. But I of course eventually make decisions and do changes based on what I think is the best way forward for the project and its users right now.

@bagder

This comment has been minimized.

Member

bagder commented Sep 27, 2017

@HBBroeker:

In other words, if your autoconf found D_HAVE_SYS_SELECT_H, then you should #include it.

No can do. The public header files need to remain read-only and cannot be updated based on configure or use #ifdefs based on its results. The installed set of header files can be used by different architectures/platforms (and we've worked hard to finally make them so).

Cygwin is kept from doing the right thing by projects like yours that rely on incorrect assumptions.

I strongly disagree. cygwin has provided a set of includes files for many years and there are a bazillion of programs out there in the world that can be built with it. If they then change how their headers work there will of course be compiler errors in programs all over the world. That's not our fault. Programmers make programs compile with cygwin and the assumption is then that it will keep compiling even when cygwin is upgraded. They can't be foolish enough to believe or expect the world of C programmers to write programs according to what the single unix spec or POSIX say.

So we may have done a slightly quirky solution once that we still live with, but it doesn't allow cygwin to cause breakage.

@bagder bagder closed this in 6aa86c4 Sep 27, 2017

@bagder

This comment has been minimized.

Member

bagder commented Sep 27, 2017

Thanks!

@HBBroeker

This comment has been minimized.

HBBroeker commented Sep 27, 2017

Programmers make programs compile with cygwin and the assumption is then that it will keep compiling even when cygwin is upgraded.

Programs written like the quoted fragment are not in the position to make such assumptions. The only assumption such code can reasonably make that is it will be broken anew every time any platform is changed or updated in any way. If that's how you want to work, OK. But don't go throwing blame at others.

They can't be foolish enough to believe or expect the world of C programmers to write programs according to what the single unix spec or POSIX say.

So what else do you suggest they might expect anyone to do? How is standardization ever going to make any kind of progress if programs actively refuse to cooperate with the effort?

What you're doing here is consciously deciding to be part of the problem. Speak of being foolish.

@bagder

This comment has been minimized.

Member

bagder commented Sep 27, 2017

If that's how you want to work, OK

  1. That's your words, I didn't say I or we work like that
  2. You have not even tried to come up with a working solution

What you're doing here is consciously deciding to be part of the problem. Speak of being foolish.

But lucky for you, you can just go somewhere else and use another library, nobody forces you to use the work I do here.

@curl curl locked and limited conversation to collaborators Sep 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.