bugGNU Octave - Bugs: bug #41027, config.h is too generic

 
 

bug #41027: config.h is too generic

Submitted by:  Orion Poplawski <opoplawski>
Submitted on:  Sun 29 Dec 2013 03:43:39 PM UTC  
 
Category: Configuration and Build SystemSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Installation Failure
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Fri 15 Jul 2016 10:33:46 PM UTC, comment #43:

Yes I think this could be closed. There are still some remaining HAVE_FOO symbols in our header files, but those header files were not including config.h anymore, so it's not really a problem.

I think we want to continue to clean all of the HAVE_FOO symbols out of the public header files so that the API is not conditional at all.

But the basic problem of this bug report has now been fixed.

Mike Miller <mtmiller>
Project Administrator
Fri 15 Jul 2016 10:31:37 PM UTC, comment #42:

I pushed a change to install a deprecated version of config.h since it is no longer included by any of our header files now, so our API is not dependent on it.

It may be that some Forge packages or other external code are relying on some of the HAVE_FOO symbols from config.h, but they shouldn't be. I think we actually do want to stop exposing those symbols unequivocally as part of this change, so those types of errors should just be fixed in the respective packages.

http://hg.savannah.gnu.org/hgweb/octave/rev/64f82a8b46e1

Mike Miller <mtmiller>
Project Administrator
Fri 15 Jul 2016 10:20:01 PM UTC, comment #41:

Once we have the help text, can we finally close this bug? It's been open since 2013.

Rik <rik5>
Project Administrator
Fri 15 Jul 2016 09:02:33 PM UTC, comment #40:

It may not always be appropriate to remove it. Some people may have been depending on it to provide configuration info. But it was really not appropriate for us to be exposing Octave's configuration environment and for them to be assuming that their configuration environment is the same as Octave's. So we should probably point to some documentation that explains that if they need configuration info, they need to obtain it using their own configure script. Alternatively, they might be able to use Octave features that already provide consistent interfaces to system dependent functionality.

But yes, it would be friendly to provide a config.h that gives a warning of some kind.

John W. Eaton <jwe>
Project Administrator
Fri 15 Jul 2016 08:07:26 PM UTC, comment #39:

There is a lot of legacy code that #includes octave/config.h. I think for 4.2 maybe we should install a stub config.h header with one line

Mike Miller <mtmiller>
Project Administrator
Fri 01 Jul 2016 02:29:52 AM UTC, comment #38:

It looks to me like none of the installed header files under $(includedir) actually include config.h anymore, they all include octave-config.h now. So we could probably stop installing config.h now with no ill effect.

Since it's no longer included already, that just means that classes like gzfilebuf won't be defined for API users outside of Octave, unless of course they also use autoconf and/or define HAVE_ZLIB.

Mike Miller <mtmiller>
Project Administrator
Tue 15 Mar 2016 07:54:35 PM UTC, comment #37:

I think the list of macros is now down to this:

I'll work on the ZLIB macros next.

The template source files are

I'm not sure whether all of these should be installed.

The JIT compiler doesn't work so I think we can put that off for now. The template source files probably don't see much use, and if someone does use them, they can come up with their own config.h if necessary.

Once I've eliminated the ZLIB macros from the public header files, I intend to stop installing config.h and oct-conf-post.h and close this report.

John W. Eaton <jwe>
Project Administrator
Tue 15 Mar 2016 07:52:37 PM UTC, comment #36:

I checked in the following changeset to eliminate using FFTW configuration macros in the public header files:

http://hg.savannah.gnu.org/hgweb/octave/rev/763e30285935

I also checked in this change to avoid installing headers from the libinterp/operators directory:

http://hg.savannah.gnu.org/hgweb/octave/rev/769f9a7c02ae

John W. Eaton <jwe>
Project Administrator
Wed 17 Feb 2016 11:46:58 PM UTC, comment #35:

Here is the list of remaining macros that I found are still unaccounted for if config.h goes away, maybe I missed some.

  • HAVE_FAST_INT_OPS
  • HAVE_FFTW
  • HAVE_LLVM
  • HAVE_LLVM_DATALAYOUT_H
  • HAVE_LLVM_IR_DATALAYOUT_H
  • HAVE_LONG_LONG_INT
  • HAVE_OPENGL
  • HAVE_OPENMP
  • HAVE_SUNMATH_H
  • HAVE_UNSIGNED_LONG_LONG_INT
  • HAVE_ZLIB
  • HAVE_ZLIB_H
  • OCTAVE_HAVE_SIG_JUMP
Mike Miller <mtmiller>
Project Administrator
Wed 17 Feb 2016 06:23:46 PM UTC, comment #34:

It's fine with me if we change our sources to use "config.h" instead of <config.h>. We can't prevent all problems that might happen if there are stray files in random directories.

And yes, I'm getting closer to being able to close this bug report. The problem isn't so much the include <config.h> line in the public headers. I could stop installing config.h and oct-conf-post.h and remove those lines, but that wouldn't fix the problem of having some configuration macros that are defined in Octave's config.h file (HAVE_LLVM, HAVE_HDF5, etc.) and still used in the public header files. I count about 59 uses of 21 symbols remaining.

John W. Eaton <jwe>
Project Administrator
Wed 17 Feb 2016 06:12:17 PM UTC, comment #33:

I just did

It seems that Octave is pretty close, or maybe already at the point where this bug can be closed.

Rik <rik5>
Project Administrator
Wed 17 Feb 2016 06:06:26 PM UTC, comment #32:

Just verified that the problems with mkoctfile have been fixed.

Rik <rik5>
Project Administrator
Wed 17 Feb 2016 05:47:56 PM UTC, comment #31:

I also think that's a useful distinction, it helps serve both a documentation purpose and a maintenance purpose.

In the public header files, it also enables users to

if they like that style, and the local include directives should pick up all Octave headers that are also in the octave subdirectory without adding it as a -I option.

Mike Miller <mtmiller>
Project Administrator
Wed 17 Feb 2016 05:15:05 PM UTC, comment #30:

I suppose we could, in the sense that the tools support it, but it seems weird to use #include <...> when the headers are not from system libraries. I use the difference in syntax between <lib.h> and "lib.h" as a hint to the programmer about the structure of the code.

There is a whole Stack Overflow thread on this: http://stackoverflow.com/questions/21593/what-is-the-difference-between-include-filename-and-include-filename.

In short, it seems that for gcc the difference is that #include "..." will look first in the directory where the source code is being compiled. It seems very unlikely that a stray config.h is going to appear in, for example, libinterp/corefcn/. Certainly 'make dist' will not include it. Even at the top-level of the source tree where a config.h might accidentally get created, I don't think there is much danger. As far as I can see there is no actual C or C++ file at the top-level that--being compiled--would attempt to include config.h

Rik <rik5>
Project Administrator
Wed 17 Feb 2016 04:29:48 PM UTC, comment #29:

The autoconf manual recommends including <config.h> so that a stray config.h file is not accidentally picked up from the source tree when doing VPATH builds. I guess the same could be true of any generated header file. And since we always use -I options to specify where to search for files, including searching the build tree, I guess we could use <> for all header files?

John W. Eaton <jwe>
Project Administrator
Wed 17 Feb 2016 04:26:40 PM UTC, comment #28:

I checked in the following changeset to fix the problem with mkoctfile:

http://hg.savannah.gnu.org/hgweb/octave/rev/c4b9567f9405

John W. Eaton <jwe>
Project Administrator
Wed 10 Feb 2016 10:49:13 PM UTC, comment #27:

Just built and installed a local version of the development branch. When I try to build a .oct file using mkoctfile I get warnings about re-definitions of macros.

Apparently there is some overlap between octave-config.h and oct-conf-post.h

Rik <rik5>
Project Administrator
Wed 10 Feb 2016 10:33:32 PM UTC, comment #26:

Coming back to a more minor issue, shouldn't Octave code use

rather than

We want to pull a local copy of config.h, not what some other package may have accidentally installed in the global /usr/include directory.

Rik <rik5>
Project Administrator
Tue 09 Feb 2016 09:14:20 PM UTC, comment #25:

I also just added a #if conditional to oct-inttypes.h that depends on system configuration, it's in the stable branch but not yet merged to default, will need to be dealt with somehow :/

http://hg.savannah.gnu.org/hgweb/octave/rev/b48d65c5df5b

Mike Miller <mtmiller>
Project Administrator
Tue 09 Feb 2016 08:57:41 PM UTC, comment #24:

Regarding one point in comment #22, I cleaned up #ifdefs recently and made sure that every .h file had a guard against multiple inclusion. So I think it is safe, and quite clear from a coding perspective, to #include whatever the code actually uses.

Rik <rik5>
Project Administrator
Tue 09 Feb 2016 08:35:59 PM UTC, comment #23:

Looks like some great progress to me, thanks for working on this.

I haven't looked at our uses of the specific libraries you mention, but the way I've removed dependency libraries before is with opaque types in the header or a pimpl approach.

Wrt include files, my preference is to have header files always include everything they need to compile and not require the user to include anything additional. So I think every header should include octave-config.h, plus whatever other headers they need.

Mike Miller <mtmiller>
Project Administrator
Tue 09 Feb 2016 07:52:57 PM UTC, comment #22:

I've checked in a series of changesets that have removed many of the configuration macros from our public header files. For macros and typedefs that can't be removed, I created a new octave-config.h header. That file is installed now. I think I'm close to the point where it's no longer necessary to install config.h.

I'm not sure about the following:

Should we just require that anyone wishing to use Octave headers include octave-config.h? Or should all of our headers that need it include it? That would be almost all of them since nearly every header needs a typedef for octave_idx_type or OCTAVE_API (the dllimport/visibility attribute tag).

Similarly, should all header files include all the other headers they need? I guess so, since that's something that needs to happen anyway and we have multiple inclusion guards on almost every file now (and it wouldn't be too hard to make sure that we have them on all header files).

The big remaining issues are dealing with FFTW, LLVM, and libz, as the classes that we have that use these features expose details of the various library interfaces in our public interfaces. I've already cleaned up a lot of this kind of thing but I'm not sure yet how to deal with these remaining problems.

Even though a few things still need to be fixed, I think the Octave header files are significantly cleaner now. So, progress anyway.

John W. Eaton <jwe>
Project Administrator
Wed 27 Jan 2016 05:15:09 PM UTC, comment #21:

jwe is looking at redoing the build system to solve the "config.h" problem. He might chime in if he is making progress.

Rik <rik5>
Project Administrator
Wed 27 Jan 2016 05:09:30 PM UTC, comment #20:

I'm facing this issue again with the development version of the mapping package. The problem is that gdal has the same bug, it's installing gdal/cpl_config.h which is sourced on its public headers, and then conflicts with the ones from Octave. Yes, this is a gdal bug but is also an Octave bug.

Carnë Draug <carandraug>
Project Member
Mon 15 Jun 2015 11:59:45 AM UTC, comment #19:

I am facing this issue again with the image package. Symbols defined by the configure script (I'm defining them via command line and not by creating a config.h file) get shadowed by Octave core results:

At the moment I am not actually using these symbols so I can ignore the warnings. But so many warnings scare the users. Also, I may want to use them later (these or some other that core already defines).

Carnë Draug <carandraug>
Project Member
Sun 11 Jan 2015 08:26:10 PM UTC, comment #18:

I am not sure if this is the cause, but I cannot use the octave-swig binder/wrapper for openCV(http://octave-swig.sourceforge.net/octave-opencv.html). I am also using 3.8.1 on a beaglebone black. The printout I receive is:

Greg <mog123>
Fri 21 Nov 2014 06:48:37 PM UTC, comment #17:

This is also affecting the image package. Its configuration is more complicated than it should we need to undef a set of Octave macros.

http://hg.code.sf.net/p/octave/image/file/68db994686c5/src/undef_unordered_map.h

Carnë Draug <carandraug>
Project Member
Tue 29 Jul 2014 12:46:55 PM UTC, comment #16:

I am also experiencing this issue with Octave 3.8.1. I am the author of a module which is meant to be compilable by other people, so I can't just patch my own version of Octave.

IMHO the roadblock seems to be that users like myself, who are affected by this, don't know anything about Octave's internals and so couldn't attempt the "complete solution" that is being insisted upon (e.g. removing config.h completely). Whereas the developers who would know enough to fix this aren't motivated to do so - understandably, since presumably this problem doesn't affect them. It would be nice to find some way to more forward.

Karl Wette <kwwette>
Mon 19 May 2014 04:42:00 PM UTC, comment #15:

> I agree this would be temporary, but it has been 2 releases


These "temporary" things are hardly ever temporary. This is a bug that more or less goes back to the creation of Octave, more than 20 years ago. Let's actually fix it.

> Would you consider including it in the next release if a different fix is not done ?


I'd rather encourage you to write a real fix. Since you're the person most apparently affected by it, I think you're the one with the highest motivation to write a real fix or pay someone to do it for you.

If you don't want to write a real fix, you can always just patch your own version of Octave or hope for another champion to come by and write it for you.

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Mon 19 May 2014 09:22:35 AM UTC, comment #14:

I agree this would be temporary, but it has been 2 releases since we couldn't build octave modules altough this possible workaround is trivial has been reported.

Would you consider including it in the next release if a different fix is not done ?

MIchel <xantares>
Sun 18 May 2014 06:26:56 PM UTC, comment #13:

This patch is just a band-aid. Feel free to apply it to your own copy of Octave, but as has already been discussed, we need a different fix for Octave.

Jordi Gutiérrez Hermoso <jordigh>
Project Administrator
Sun 18 May 2014 04:15:48 PM UTC, comment #12:

Hi,

Here is the verified patch attached from the previous comment.
Anyone for a review ?

xan.

(file #31400)

MIchel <xantares>
Sat 19 Apr 2014 06:22:27 PM UTC, comment #11:

Hello!
Has anyone made progress on this issue ?
It is becoming frustrating to not be able to compile modules using octave headers.
The following patch against the current master at least removes the angled brackets to allow the compilation before a proper solution is found.
Could it be applied, please :]

diff -r 900b524d9072 libinterp/corefcn/comment-list.h
--- a/libinterp/corefcn/comment-list.h Sat Apr 19 09:28:20 2014 -0700
+++ b/libinterp/corefcn/comment-list.h Sat Apr 19 20:12:54 2014 +0200
@@ -25,7 +25,7 @@

#include <string>

-#include <base-list.h>
+#include "base-list.h"

extern std::string get_comment_text (void);

diff -r 900b524d9072 libinterp/corefcn/oct.h
--- a/libinterp/corefcn/oct.h Sat Apr 19 09:28:20 2014 -0700
+++ b/libinterp/corefcn/oct.h Sat Apr 19 20:12:54 2014 +0200
@@ -28,7 +28,7 @@
// config.h needs to be first because it includes #defines that can */
// affect other header files.

-#include <config.h>
+#include "config.h"

#include "Matrix.h"

Anonymous
Wed 15 Jan 2014 12:07:51 AM UTC, comment #10:

There is also:

--- a/libinterp/corefcn/comment-list.h Wed Jan 08 14:42:56 2014 +0100
+++ b/libinterp/corefcn/comment-list.h Tue Jan 14 17:06:41 2014 -0700
@@ -25,7 +25,7 @@

#include <string>

-#include <base-list.h>
+#include "base-list.h"

extern std::string get_comment_text (void);

It is also tricky because user code is expected to use <octave/header.h>, but the octave code does not use the octave/ path.

Orion Poplawski <opoplawski>
Tue 07 Jan 2014 09:45:31 PM UTC, comment #9:

I'd prefer to try to do it right, and actually see if we can limit or even eliminate any configure macros in our public header files.

John W. Eaton <jwe>
Project Administrator
Tue 07 Jan 2014 09:37:04 PM UTC, comment #8:

The autotools FAX mentions two possible solutions. The more braindead solution seems to be to give the config file and all #defines unique names so that the conflict with an existing config.h file or its #defines is removed.

There is a macro to help implement that: AX_PREFIX_CONFIG_H (http://www.gnu.org/software/autoconf-archive/ax_prefix_config_h.html).

It would involve a lot of search and replace operations (Perl!), but is otherwise easy to implement. The first alternative looks harder in that you have to think carefully about how to make exportable header files which don't rely on config.h

Rik <rik5>
Project Administrator
Tue 07 Jan 2014 04:54:45 AM UTC, comment #7:

It's not just the name config.h, it's that a header file that contains the results of feature tests should never be installed.

It's an autotools FAQ:

http://www.gnu.org/software/automake/faq/autotools-faq.html#Why-should-config_002eh-not-be-installed_003f

It's a bit embarrassing that Octave has been doing this for so long, but I've just never gotten around to fixing it. Maybe this bug report will provide the motivation.

John W. Eaton <jwe>
Project Administrator
Tue 07 Jan 2014 03:33:38 AM UTC, comment #6:

Well, plplot will also be renaming their "config.h" to "plplot_config.h" to work around this, so I'm good. But yes, changing <config.h> to "config.h" fixes this issue as well.

Orion Poplawski <opoplawski>
Sat 04 Jan 2014 11:36:08 PM UTC, comment #5:

Ping. Can we work around this temporarily by changing <config.h> to "config.h" everywhere in the source code?

Rik <rik5>
Project Administrator
Thu 02 Jan 2014 07:53:05 PM UTC, comment #4:

As a temporary workaround, is it correct to revert to #include "config.h" without the angle brackets?

Rik <rik5>
Project Administrator
Thu 02 Jan 2014 03:25:48 PM UTC, comment #3:

John, do you mean ideally you want to purge all HAVE_* options and other macros from config.h from all Octave public header files? Sounds good to me.

Yes, the real bug is that too many packages install their own config.h, the regression in this specific case is probably due to this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/307912900544

Note that similar errors would occur for projects including other Octave header files, it just happens that oct.h was including "config.h" as Orion pointed out so that "safely" included the right config.h.

Mike Miller <mtmiller>
Project Administrator
Mon 30 Dec 2013 06:11:40 AM UTC, comment #2:

The real bug is that Octave should not be installing config.h, even as octave/config.h. This problem has been around a long time but so far no one has taken the time to really fix it correctly. Changing Octave sources to use <octave/header.h> is not the right solution.

John W. Eaton <jwe>
Project Administrator
Sun 29 Dec 2013 06:15:06 PM UTC, comment #1:

Actually, perhaps the issue is changing from:

#include "config.h"

to

#include <config.h>

?

Orion Poplawski <opoplawski>
Sun 29 Dec 2013 03:43:39 PM UTC, original submission:

We're seeing build issues with plplot against octave 3.8.0:

/usr/bin/c++ -DHAVE_CONFIG_H -Dplplot_octave_EXPORTS -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -fPIC -I/builddir/build/BUILD/plplot-5.9.10/include -I/builddir/build/BUILD/plplot-5.9.10/lib/qsastime -I/builddir/build/BUILD/plplot-5.9.10/fedora -I/builddir/build/BUILD/plplot-5.9.10/fedora/include -I/builddir/build/BUILD/plplot-5.9.10/fedora/bindings/octave -I/usr/include/octave-3.8.0 -I/usr/include/octave-3.8.0/octave -I/builddir/build/BUILD/plplot-5.9.10/bindings/swig-support -o CMakeFiles/plplot_octave.dir/plplot_octaveOCTAVE_wrap.cxx.o -c /builddir/build/BUILD/plplot-5.9.10/fedora/bindings/octave/plplot_octaveOCTAVE_wrap.cxx
In file included from /usr/include/octave-3.8.0/octave/dim-vector.h:35:0,
from /usr/include/octave-3.8.0/octave/Array.h:35,
from /usr/include/octave-3.8.0/octave/boolMatrix.h:27,
from /usr/include/octave-3.8.0/octave/mx-base.h:32,
from /usr/include/octave-3.8.0/octave/Matrix.h:30,
from /usr/include/octave-3.8.0/octave/oct.h:33,
from /builddir/build/BUILD/plplot-5.9.10/fedora/bindings/octave/plplot_octaveOCTAVE_wrap.cxx:154:
/usr/include/octave-3.8.0/octave/oct-refcount.h:27:3: error: #error "The file <octave/config.h> must be included before oct-refcount.h."
# error "The file <octave/config.h> must be included before oct-refcount.h."

This appears to be because plplot's own config.h is included from octave/oct.h instead of octave's. octave/oct.h has:

#include <config.h>

I would argue that this should be <octave/config.h> (and same for other octave headers).

Orion Poplawski <opoplawski>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #31400:  octave-bug_41027.patch added by xantares (689B - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mog123 (Posted a comment)
  • -unavailable- added by kwwette (Posted a comment)
  • -unavailable- added by jordigh (Posted a comment)
  • -unavailable- added by xantares (Updated the item)
  • -unavailable- added by rik5 (Posted a comment)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by opoplawski (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 15 Jul 2016 11:07:27 PM UTCrik5StatusConfirmed=>Fixed
      Open/ClosedOpen=>Closed
    Wed 27 Jan 2016 05:09:30 PM UTCcarandraugRelease3.8.0=>dev
    Sun 18 May 2014 06:26:56 PM UTCjordighStatusNone=>Confirmed
    Sun 18 May 2014 04:15:48 PM UTCxantaresAttached File-=>Added octave-bug_41027.patch, #31400

    Back to the top


    Powered by Savane 3.1-cleanup