STR #3013

GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 Home  |  Articles & FAQs  |  Bugs & Features  |  Documentation  |  Download  |  Screenshots  ]
 

Return to Bugs & Features | Post Text | Post File ]

STR #3013

Application:FLTK Library
Status:5 - New
Priority:3 - Moderate, e.g. unable to compile the software
Scope:3 - Applies to all machines and operating systems
Subsystem:Core Library
Summary:Fixes to use of config.h header
Version:1.4-feature
Created By:skunk
Assigned To:Unassigned
Fix Version:Unassigned
Update Notification:

Receive EMails Don't Receive EMails

Trouble Report Files:

Post File ]
Name/Time/Date Filename/Size top right image
 
#1 skunk
21:10 Dec 09, 2013
fltk-configh-fixes.patch
84k
 
 
#2 skunk
18:30 Feb 22, 2016
fltk-configh-use.patch
88k
 
bottom left image   bottom right image

Trouble Report Comments:

Post Text ]
Name/Time/Date Text top right image
 
#1 skunk
21:10 Dec 09, 2013
In the course of building FLTK, I've run into situations where some of the library source files do not #include the config.h configuration header, and thus [newly added] cpp conditionals that rely on symbols defined therein do not work.

I've also noticed that config.h is often #included after other headers, particularly system headers, which is contrary to convention and robs the configuration header of much of its power and usefulness. If config.h is seen after, say, stdio.h, then [user-added] cpp directives in the file like

    #define _POSIX_SOURCE

or

    #define _FILE_OFFSET_BITS 64

will have no effect. The user can add these as -D flags to CPPFLAGS, but the conventional understanding of config.h is that definitions in there are equivalent to those in CPPFLAGS. That's what the GNU Autotools do.

Given the above, I've prepared a comprehensive patch against r10024 that addresses both issues. Here is a walk-through of the changes:

* configh.in: Added a check to ensure that (1) config.h is #included only once, (2) config.h is #included before system header files (at least on Linux), and (3) config.h is not #included after Fl.H.

* Most files: Added a missing #include<config.h> directive, or moved it up so that it is the first file #included

* src/Fl_Font.H, src/Fl_XColor.H, src/flstring.h, test/CubeView.h: Removed the #include<config.h> directive, as this really shouldn't be in a header file. Having this #include in a header makes it harder to ensure that config.h is being used correctly.

* src/Fl_Gl_Window.cxx, src/glut_compatability.cxx: These files appear to #include"flstring.h" solely to pull in config.h

* src/Fl_PostScript.cxx: This file is #included from Fl_Printer.cxx, so only the latter needs the #include<config.h>

* src/Fl_get_key_mac.cxx: This file is #included from Fl_get_key.cxx, so only the latter needs the #include<config.h>

* src/fl_color_{mac,win32}.cxx: These files are #included from fl_color.cxx, so only the latter needs the #include<config.h>

* src/fl_draw_image_{mac,win32}.cxx: These files are #included from fl_draw_image.cxx, so only the latter needs the #include<config.h>

* src/fl_font_mac.cxx: This file is #included from fl_font.cxx, so only the latter needs the #include<config.h>

* src/fl_read_image_mac.cxx: This file is #included from fl_read_image.cxx, so only the latter needs the #include<config.h>

* src/fl_set_fonts_mac.cxx: This file is #included from fl_set_fonts.cxx, so only the latter needs the #include<config.h>


Correct usage of config.h is partly ensured by the check added to that same header, and can be further verified by [temporarily] adding a construct like the following to the top of Fl.H:

    #ifndef FLTK_CONFIG_H
    #  error "config.h was not #included"
    #endif

My patch addresses all cases of incorrect config.h usage that I could find on Linux/Win/Mac builds, with the notable exception of Fluid-generated source code (for which I was not able to find a good solution).
 
 
#2 AlbrechtS
04:50 Dec 28, 2013
Daniel (skunk), I can see a lot of good ideas and observations in your patch(es) and explanations, but there are some points I want to comment on.

(1) I'm not keen on adding config.h to ALL FLTK source files, as your patch does. Question to other devs: do we want to do this? My understanding is that config.h should only be included if it is needed in a particular file.

(2) I don't want to encourage users to change config.h (or configh.in) to add their own symbols (macros, #defines). I believe that config.h should be generated by autoconf/configure, and that it should not be changed by others. Adding other defines by using CPPFLAGS (or CFLAGS, CXXFLAGS) seems more appropriate to me.

(3) That said, I don't know much of the "conventions" you mentioned, and if other devs confirmed that these conventions are widely used and that we should adhere to these conventions... well, then points (1) and (2) would probably be moot.

(4) The 6 files fluid/*_panel.cxx are generated files, although they are included in the svn repository. We would need to change fluid/*_panel.fl and regenerate the .cxx/.h files instead (if we would want to add config.h to ALL files at all, see (1-3).

(5) You wrote: "My patch addresses all cases of incorrect config.h usage ... with the notable exception of Fluid-generated source code...". I don't think that Fluid-generated code should deal with and/or include config.h at all, since fluid is primarily used to generate user code (outside the FLTK lib) - hence config.h wouldn't be available at all. Or did I misunderstand what you wanted to say?
 
 
#3 skunk
09:02 Dec 28, 2013
Hi Albrecht,

(1) Questions of convention aside, it's easier to require that config.h be #included globally, and have mechanisms to ensure that, than to #include it on an as-needed basis and have to keep track of the as-needed-ness. What happens when a file sans config.h gains its first USE_X11 or FLTK_HAVE_CAIRO symbol? If you forget to add the #include, those conditionals will be stuck to false even if they are true in the header. (My initial motivation for filing this issue in the first place was running into that very situation as I was hacking on FLTK.)

(2) It may not be most users who add their own symbols to config.h, but that is a thing. It's easier than re-configuring a project to modify the value of CPPFLAGS, and people are used to the idea that definitions in config.h are interchangeable with those in CPPFLAGS anyway.

(3) About the only thing FLTK is doing differently from most Autotools-based projects is not using an Autoheader-generated config header (as well as not using Automake, though that is more common).

(4) Ah, I missed that those files were generated.

(5) Bear in mind that config.h is not just the header file used by FLTK; this is the default/standard name for the configuration header used by Autoconf in general. When users generate their own code via Fluid, if they are using their own config header at all, 99% of the time that header will be named config.h.

Of course, not all users will make use of a config header. It is conventional (from Autotools) to #include the header with

    #ifdef HAVE_CONFIG_H
    # include <config.h>
    #endif

so that the HAVE_CONFIG_H symbol can control whether or not the #include happens. It would be sensible for Fluid to add this construct instead of the #include by itself.

That said, Fluid need not have special awareness of this #include/construct. If it provided a mechanism for adding arbitrary code _before_ the #include of the interface header file, that would be enough. (Right now, you can only pull in arbitrary headers _after_ that #include, which for config.h purposes is useless because system headers have already been seen at that point.)
 
 
#4 michaelbaeuerle
03:38 Jan 07, 2014
To (1):
Include config.h everywhere may be harmless if it contains only definitions that are used locally. But we must remember that things like the example:
#define _POSIX_SOURCE
will change the behaviour of system header files and are not trivial to use in a project like FLTK that use mixed C and C++. Older versions of POSIX have no C++ binding, this means all source files using this definition must be compiled with a C compiler (and yes, I have seen g++/libstdc++ failing in real world if this is not respected).

To (2):
I agree with Albrecht that changing the generated file config.h manually is not a good idea.

To (3):
The other example:
#define _FILE_OFFSET_BITS 64
may also change the behaviour of system header files. If we really want to be able to use such things in config.h, then we must ensure that config.h is always included before all system header files as Daniel has mentioned.
My opinion: The idea to have such definitions in a central place is desirable in general, but it may not be as simple as it looks at first (see my comment to (1)) and we should think twice.
 
 
#5 skunk
11:10 Jan 07, 2014
Re (1): There are systems where _POSIX_SOURCE may be required in order to enable POSIX features used by the code, in which case changing the behavior of system header files is exactly what you want to do. (Moreover, while POSIX may not have a C++ binding, that doesn't mean that C++ source files can't call the C functions just as well. If you want to guard against buggy systems where this isn't possible, you could keep the POSIX calls in C files, and maybe put an "#ifndef __cplusplus" around the macro.)

Anyway, _POSIX_SOURCE is an arbitrary example, and the point is not the merits of that particular feature test macro but how config.h is handled. After all, a user could instead configure with CPPFLAGS="-D_POSIX_SOURCE", and that should be equivalent---but currently is not.

Re (2): Manually editing config.h is a "you know what you're doing" kind of thing. The way FLTK uses this header is not unusual or different from most other projects, so people are naturally going to bring to bear the knowledge they've gained from working with other projects.

Re (3): Aside from #including config.h before any system header file, you really only have to worry about #including it consistently from all source files (otherwise, some files may not see e.g. _FILE_OFFSET_BITS, and end up using types/structures incompatible with other files). That's about as far as the subtleties of this go, and as much as you need for config.h and CPPFLAGS to be interchangeable.

My patch adds a bit to config.h that will help ensure no system headers have already been seen, and if you're willing, we could add a bit to Fl.H [that is only used when building FLTK itself] that ensures config.h has been #included.
 
 
#6 AlbrechtS
18:48 Jan 20, 2016
Moved to 1.4-feature.  
 
#7 skunk
18:36 Feb 22, 2016
I've posted an updated patch, against r11211.

This does just three things:

* Adds #include<config.h> to all files that need it;

* Removes or relocates #include<config.h> to all files that are not using the header correctly;

* Adds to configh.in some code to check against three different scenarios where the config header is used incorrectly.


This patch makes the same rote change to a large number of files. As such, it has a short shelf life. Could this be committed soon, to avoid needing to regenerate the patch down the line, as well as find new source files that need the #include?
 
 
#8 AlbrechtS
04:14 Feb 23, 2016
Daniel, thanks for the updated patch. As you may have noticed I moved this STR to 1.4-feature. We're now working on the supposedly final release of 1.3.x (1.3.4).

We will consider your patch in FLTK 1.4.0. However, this new release will have many changed and new files and a new header file "src/config_lib.h" that

 (a) #include's <config.h>
 (b) will likely be #include'd in most or all library source files

This has two consequences:

 (1) unfortunately your patch will be very difficult if not impossible to apply (directly)
 (2) since config_lib.h will be #include'd (almost?) everywhere we can be sure that <config.h> will be #include'd as well.

You may take a look at the new development branch "branch-1.3.porting" which will likely be the base of FLTK 1.4.0.

PS: this is no decision pro or contra your patch, it's just a description of the status quo.
 
 
#9 skunk
13:09 Feb 23, 2016
Hi AlbrechtS,

Should I prepare a patch against 1.3-porting? I see a number of files still lack a config header of any kind, and many are using config.h instead of this new config_lib.h.

I think you're better off requiring that *all* library source files #include config_lib.h, if for no other reason than that'll be easier than trying to explain when to use it vs. when not to.
 
 
#10 AlbrechtS
16:41 Feb 23, 2016
Thanks for your offer. I believe it's not the right time (too early) now to create a patch for branch-1.3-porting. We're frequently adding and removing files and your patch would likely be worthless after a few more commits (wasted time).

We should discuss the issue in fltk.coredev to get to a decision whether we want to add src/config_lib.h to all source files (which appears more and more to be a good decision in my eyes). Whether we want or need to enforce this and the order of #include's is another question.

Please feel free to start a thread in fltk.coredev (fltkcoredev@googlegroups.com) and refer to this STR for more info.

Thanks for your efforts to improve FLTK.
 
 
#11 skunk
17:34 Feb 23, 2016
Argh, starting discussion threads is a little much for me at present. But if there's a thread going, I may be able to chime in.

This GNU binutils bugzilla page may be of interest:

    https://sourceware.org/bugzilla/show_bug.cgi?id=14072

The binutils project, at one point, was also a mess regarding how the config header was handled. This was causing me grief on some exotic platforms where missing/ineffective config definitions led to breakage (see bug #13558), so I did pretty much the same thing there that I've been doing here :)
 
bottom left image   bottom right image

Return to Bugs & Features | Post Text | Post File ]

 
 

Comments are owned by the poster. All other content is copyright 1998-2021 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.