GNOME Bugzilla – Bug 781598
gstdio.h should #include what it needs to work
Last modified: 2017-09-12 11:38:43 UTC
This is related to bug 566873. The conclusion reached in that report is that one should have to include extra header files to use gstdio.h. If that is the view of the glib maintainers, then it should be documented. At the moment, the "Includes" section of the "File Utilities" documentation only mentions glib.h and glib/gstdio.h. However, I suggest that it would be better to include the relevant headers in gstdio.h. There are three reasons for this: 1. In C code, compilation still works, but with warnings. Many projects require compilation with zero warnings, so such projects will still have to work out the problem and do extra work. 2. In C++ code, compilation can fail. For example, in libenchant's tests, I get: ./EnchantTestFixture.h:108:60: error: ‘creat’ was not declared in this scope 3. It is easy to fix this in a non-portable way that only builds on UNIX. I myself did this at first. I believe a portable fix is: #ifdef G_OS_UNIX #include <fcntl.h> #endif (In particular, on Win32 platforms, fcntl.h is included by glib.) I suggest that it would be better to avoid these problems by having the fix in glib itself. There is another, related oddity. The simple macro wrappers like #define g_creat creat are defined according to a test: #if defined(G_OS_UNIX) && !defined(STDIO_NO_WRAP_ON_UNIX) I don't understand the name "STDIO_NO_WRAP_ON_UNIX", because surely this should be true if the functions are to be wrapped, and false otherwise. Yet if the macro is true, then the wrapper functions are not used, and if it is false, the wrapper functions are used. It would also seem simpler to use the wrapper functions by default. This would avoid all the above compilation problems too, and the source fix to glib would be much simpler, and also solve the above-mentioned problem of the name of STDIO_NO_WRAP_ON_UNIX. Simply change the sense in which it is interpreted to match its name, change the guard line above to #if defined(G_OS_UNIX) && defined(STDIO_NO_WRAP_ON_UNIX) and correspondingly remove the line from gstdio.c that #defines it to 1. Of course, this would break any code that intentionally sets the macro, but it is undocumented, so it might be preferable to change the supported name, e.g. to STDIO_FUNCTION_WRAPPERS_ON_UNIX, and add #ifdef STDIO_NO_WRAP_ON_UNIX #error No longer supported; see STDIO_FUNCTION_WRAPPERS_ON_UNIX #endif to gstdio.h. To summarise, my suggestions, in increasing order of desirability, are: 1. Document the problem. 2. Add includes to gstdio.h to fix the problem. 3. Default to using the function wrappers to fix the problem. 4. Default to using the function wrappers to fix the problem, and remove the confusion around STDIO_NO_WRAP_ON_UNIX by replacing it with another macro which is used in the "correct" logical sense, and add an #error to help anyone unwise/unlucky enough to use the old, undocumented STDIO_FUNCTION_WRAPPERS_ON_UNIX.
*** Bug 742201 has been marked as a duplicate of this bug. ***
I think it makes sense to #include the necessary headers in gstdio.h; it should do no harm in any case, and makes the library easier to use. Patches welcome. I don’t understand the reasoning behind G_STDIO_NO_WRAP_ON_UNIX, and it strikes me as the kind of macro hackery which is in there to support some odd platform. I don’t have the time or inclination to dig into its history or purpose at the moment, so I would prefer to leave that alone. If you really care about changing it, please open a new bug report about it — let’s leave this one focused on the #include issues. Thanks.
(In reply to Reuben Thomas from comment #0) > The conclusion reached in that report is that one should have to include > extra header files to use gstdio.h. That was 2009... I hope we all agree that nowadays headers should #include what they need.
Created attachment 359593 [details] [review] Patch to include fcntl.h in gstdio.h
(In reply to Michael Catanzaro from comment #3) > (In reply to Reuben Thomas from comment #0) > > The conclusion reached in that report is that one should have to include > > extra header files to use gstdio.h. > > That was 2009... I hope we all agree that nowadays headers should #include > what they need. I think that was already agreed in 2009! What has perhaps changed is the scope of what gstdio.h is expected to do. Tor Lillqvist wrote in comment #3 on bug #566873: “Remember that gstdio's purpose is to work around the file name encoding issues on Windows. The intent is not that including <glib/gstdio.h> would remove the need to include the platform-specific header with a declaration of close(), for instance.” Now it seems reasonable to broaden the scope of gstdio to more general portability, and I certainly agree that one should be able to use all the g_ prefixed macros portably (i.e. without having to #include platform-specific headers in one’s glib-using code).
(In reply to Reuben Thomas from comment #5) > (In reply to Michael Catanzaro from comment #3) > > (In reply to Reuben Thomas from comment #0) > > > The conclusion reached in that report is that one should have to include > > > extra header files to use gstdio.h. > > > > That was 2009... I hope we all agree that nowadays headers should #include > > what they need. > > I think that was already agreed in 2009! What has perhaps changed is the > scope of what gstdio.h is expected to do. Tor Lillqvist wrote in comment #3 > on bug #566873: > > “Remember that gstdio's purpose is to work around the file name encoding > issues on Windows. The intent is not that including <glib/gstdio.h> would > remove the need to include the platform-specific header with a declaration > of close(), for instance.” > > Now it seems reasonable to broaden the scope of gstdio to more general > portability I don't really agree. gstdio.h is a "lesser evil" approach to deal with weird Windows-only issues with non-ASCII paths. I'd be entirely happy if it became `glib/gwin32-stdio.h` instead, and we figured out a way to replace the standard C library functions with it, without using a `g_*` namespace, and forcing people writing code to use those weird entry points. Alternatively, having a way to automatically turn paths from one form to the other. Sadly, that ship has long since left the harbour, made it halfway through the circumnavigation of the African continent, only to sink in proximity of the Cape of Good Hope. We cannot use G_OS_UNIX to include fctnl.h, because what we use to determine if we're using a Unix-like platform does not automatically enforce the presence of fnctl.h; a platform *may* have a usable fnctl.h header. That's why we use per-header and per-function checks; those checks exist in the internal config.h, and are not preserved outside of the project because that would break essentially everything that depends on GLib. Additionally, if you find yourself using gstdio.h you may want to reconsider your priorities and choices, and switch to GIO, which is *the* portable abstraction layer over I/O that we provide as part of GLib. I honestly don't understand why you can't just check for headers and functions in your project, and just use gstdio.h as the crutch for porting older projects from Linux to Windows it was always intended to be.
Review of attachment 359593 [details] [review]: ++
Thanks.
(In reply to Emmanuele Bassi (:ebassi) from comment #6) > We cannot use G_OS_UNIX to include fctnl.h, because what we use to determine > if we're using a Unix-like platform does not automatically enforce the > presence of fnctl.h; a platform *may* have a usable fnctl.h header. That's > why we use per-header and per-function checks; those checks exist in the > internal config.h, and are not preserved outside of the project because that > would break essentially everything that depends on GLib. Speaking of ships, it looks like we had a mid-sea collision. This is a good review point, and I suspect means we’re going to end up reverting this patch and changing the documentation for gstdio.h to mention fcntl.h instead. (In reply to Emmanuele Bassi (:ebassi) from comment #6) > I honestly don't understand why you can't just check for headers and > functions in your project, and just use gstdio.h as the crutch for porting > older projects from Linux to Windows it was always intended to be. It’s hard for people to work out the intention behind gstdio.h when it’s not mentioned in the documentation at all.
(In reply to Philip Withnall from comment #9) > (In reply to Emmanuele Bassi (:ebassi) from comment #6) > > I honestly don't understand why you can't just check for headers and > > functions in your project, and just use gstdio.h as the crutch for porting > > older projects from Linux to Windows it was always intended to be. > > It’s hard for people to work out the intention behind gstdio.h when it’s not > mentioned in the documentation at all. Indeed, the documentation should be amended to literally say: DO NOT USE THIS API Unless you're porting old Linux apps to Windows. Use GIO instead.
Indeed, it would be good to know what the intention is, and “DO NOT USE THIS API” is fine by me. My use case was using gstdio as a portable veneer for cross-platform code in a code base that already uses glib (the spell-checker veneer library Enchant). If that’s not how it should be used, then fine, I can use gnulib.
(In reply to Reuben Thomas from comment #11) > My use case was using gstdio as a portable veneer for cross-platform code in > a code base that already uses glib (the spell-checker veneer library > Enchant). If that’s not how it should be used, then fine, I can use gnulib. Out of interest, why not GIO? You're already using GLib, and GIO is part of that.
(In reply to Daniel Boles from comment #12) > > Out of interest, why not GIO? You're already using GLib, and GIO is part of > that. The code is basically POSIX; it uses glib mainly for data structures and character encodings, as sparingly as possible. Some modules do not require glib at all. Using GIO would require a reorientation to use glib more thoroughly, not to mention considerable work to apply that consistently. In short, a combination of design choices and historical reasons.
(In reply to Reuben Thomas from comment #13) > (In reply to Daniel Boles from comment #12) > > > > Out of interest, why not GIO? You're already using GLib, and GIO is part of > > that. > > The code is basically POSIX; it uses glib mainly for data structures and > character encodings, as sparingly as possible. Some modules do not require > glib at all. Using GIO would require a reorientation to use glib more > thoroughly, not to mention considerable work to apply that consistently.⋰ I’d be inclined on the whole to increase the use of gnulib, using iconv directly for character set conversion, gnulib’s data structure implementations, and its various POSIX and GNU API veneers. gnulib being a source-only library, it doesn’t add a build or run dependency to what is intended to be very simple, portable code. However, at present there are a few cross-platform abstractions that gnulib lacks which glib provides, so as it’s infeasible to stop using glib completely, there’s little incentive to rewrite the code that already uses its features.
I’ve reverted the #include change. Will attach a documentation patch shortly. commit 39469aa7bbccdd7b0526785612295a1534767807 (HEAD -> master, origin/master, origin/HEAD) Author: Philip Withnall <withnall@endlessm.com> Date: Tue Sep 12 12:12:27 2017 +0100 Revert "gstdio: #include fcntl.h on UNIX in gstdio.h" This reverts commit 6f8073d44ab02e9d641ccbe8c2640796ca1456ca. As per further discussion on bug #781598, we can’t do this in GLib, since fcntl.h is not guaranteed to be present on all Unix systems. Users of GLib *must* do a header check (for example, using AC_CHECK_HEADERS) and #include fcntl.h themselves.
(In reply to Philip Withnall from comment #15) > I’ve reverted the #include change. Will attach a documentation patch shortly. > > commit 39469aa7bbccdd7b0526785612295a1534767807 (HEAD -> master, > origin/master, origin/HEAD) > Author: Philip Withnall <withnall@endlessm.com> > Date: Tue Sep 12 12:12:27 2017 +0100 > > Revert "gstdio: #include fcntl.h on UNIX in gstdio.h" > > This reverts commit 6f8073d44ab02e9d641ccbe8c2640796ca1456ca. > > As per further discussion on bug #781598, we can’t do this in GLib, > since fcntl.h is not guaranteed to be present on all Unix systems. Users > of GLib *must* do a header check (for example, using AC_CHECK_HEADERS) > and #include fcntl.h themselves. I don’t contest the direction this is taking, but since fcntl.h is POSIX-mandated (since at least POSIX.1-2001) what are these Unix-like systems we’re thinking of that don’t have fcntl.h?
Created attachment 359614 [details] [review] docs: Discourage use of gstdio.h and clarify its header requirements Signed-off-by: Philip Withnall <withnall@endlessm.com>
(In reply to Reuben Thomas from comment #16) > > As per further discussion on bug #781598, we can’t do this in GLib, > > since fcntl.h is not guaranteed to be present on all Unix systems. Users > > of GLib *must* do a header check (for example, using AC_CHECK_HEADERS) > > and #include fcntl.h themselves. > > I don’t contest the direction this is taking, but since fcntl.h is > POSIX-mandated (since at least POSIX.1-2001) what are these Unix-like > systems we’re thinking of that don’t have fcntl.h? The issue is that there is no guarantee — implied or otherwise — that G_OS_UNIX means "POSIX.1-2001". We only tell you that "the G_OS_UNIX symbol is defined on UNIX systems" without any qualifier, and internally we use it as a catch-all fallback for "this is not a native Windows build". If you want to start defining G_OS_UNIX as "a specific minimum POSIX version" then we can open a new bug about that, but remember that Windows also supports a subset of POSIX, so using G_OS_UNIX would not help you for this issue. Additionally, the guarantees of what fcntl.h contains have been revised over the years, up to until POSIX.1-2008, whereas the meaning of G_OS_UNIX symbol hasn't really changed over the past 15 years.
Review of attachment 359614 [details] [review]: :thumbsup:
OK, take #2 at closing this bug. As Emmanuele said, if we want to think about any kind of minimum POSIX dependency, that should be a separate bug. Attachment 359614 [details] pushed as 1a5cebe - docs: Discourage use of gstdio.h and clarify its header requirements
(In reply to Emmanuele Bassi (:ebassi) from comment #18) > > The issue is that there is no guarantee — implied or otherwise — that > G_OS_UNIX means "POSIX.1-2001". Sure. I was wondering, rather, what non-native-Windows systems glib runs on that do not support POSIX.1-2001. But as you say, this isn’t really what G_OS_UNIX is for. > Additionally, the guarantees of what fcntl.h contains have been revised over > the years, up to until POSIX.1-2008, whereas the meaning of G_OS_UNIX symbol > hasn't really changed over the past 15 years. A simpler suggestion: deprecate G_OS_UNIX. From what you say, it could be replaced with !G_OS_WIN32, for external use. As glib is meant to enable the writing of portable code, it doesn’t make much sense for it to expose detailed platform compatibility information. Rather, it should restrict such information to the minimum, which consists mostly of being able to tell when one needs a platform-specific glib API (such as the Win32 APIs). Are there in fact any other platforms that get this sort of special treatment in glib? At least, there seems to be no other G_OS_* macro currently defined.