bugGNU Octave - Bugs: bug #44370, Octave compiler flags used at...

 
 

bug #44370: Octave compiler flags used at build time are not used by mkoctfile

Submitted by:  Carnë Draug <carandraug>
Submitted on:  Wed 25 Feb 2015 05:23:38 PM UTC  
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  dev Operating System:  GNU/Linux

Add a New Comment (Rich Markup)
   

You are not logged in

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

 

( Jump to the original submission)

Fri 27 Feb 2015 02:07:50 AM UTC, comment #16: 

Yes at least this bug looks fixed to me now. Ordinary oct-files that aren't doing anything with HDF5 shouldn't need to include hdf5.h now.

Mike Miller <mtmiller>
Project Administrator
Thu 26 Feb 2015 05:15:06 PM UTC, comment #15: 

I pushed this change:

http://hg.savannah.gnu.org/hgweb/octave/rev/09ed6f7538dd

I think it solves the immediate problem.  Ultimately, I'd like to isolate all the HAVE_HDF5 conditionals in just a few files.  That means making the load_hdf5 and save_hdf5 functions in the octave_value classes unconditionally call functions that deal with HDF5 being available (or not) and placing all the HDF5 interface glue in those functions instead of the octave_value classes.

John W. Eaton <jwe>
Project Administrator
Thu 26 Feb 2015 03:43:43 PM UTC, comment #14: 

Yes, the API has changed. True, it is still the same int type argument, but I do have to make changes to the communications package to build with this change. I can attach a diff later, I don't have it at the moment. But essentially I had to add #includes to get the hdf5 interface (because ov-base.h used to give me hdf5.h for free). And I didn't have to, but chose to, move the #ifdefs inside the load_hdf5 and save_hdf5 functions just like you did, so they are no longer conditional.

Mike Miller <mtmiller>
Project Administrator
Thu 26 Feb 2015 02:46:07 PM UTC, comment #13: 

Has anything really changed though?  We are just substituting one typedef for another, but the type is still the same (int).

John W. Eaton <jwe>
Project Administrator
Thu 26 Feb 2015 03:36:40 AM UTC, comment #12: 

And this API change should probably be mentioned in NEWS, since packages that derive from octave_base_value will need to be updated and depend on >= 4.0 if they have an hdf5 load/save interface.

Mike Miller <mtmiller>
Project Administrator
Thu 26 Feb 2015 03:34:12 AM UTC, comment #11: 

Oops, attached now.

(file #33192)

Mike Miller <mtmiller>
Project Administrator
Thu 26 Feb 2015 03:33:45 AM UTC, comment #10: 

This is awesome, thanks for coming up with that so quickly. It almost works for me. I've made a few more changes in the attached diff, feel free to merge it into yours. I think these go with the pattern you are going with.

So this looks really good, HAVE_HDF5 doesn't show up in any public header files now, except for the ones that explicitly have to do with hdf5, and the load_hdf5 and save_hdf5 functions are no longer conditional.

I've already updated the communications package to use this new interface and it at least builds, not yet tested. But this looks like the right direction to me.

Should we move this to bug #43180 since it directly fixes that one also?

Mike Miller <mtmiller>
Project Administrator
Wed 25 Feb 2015 10:42:52 PM UTC, comment #9: 

Oops, I left out a couple of new files.  New version of the patch is attached.

(file #33190)

John W. Eaton <jwe>
Project Administrator
Wed 25 Feb 2015 10:41:14 PM UTC, comment #8: 

Mike:

The attached patch avoids including hdf5.h in any Octave headers except ls-hdf5.h, and although that file is installed, it should only be included in .cc files that define octave-value subtypes.  So this should effectively avoid the problem of including or even needing hdf5 to build .oct files.

This patch isn't perfect, but I think it is better than the current situation.  If you agree, I'll write a commit log message and push it to savannah.

(file #33189)

John W. Eaton <jwe>
Project Administrator
Wed 25 Feb 2015 07:44:33 PM UTC, comment #7: 

OK, then I won't worry about that issue.

I do agree with you that we should fix the problem of including external package header files in Octave's headers if at all possible.

John W. Eaton <jwe>
Project Administrator
Wed 25 Feb 2015 07:24:00 PM UTC, comment #6: 

I can confirm that the new Debian build instructions solve this issue for me.

Oliver Heimlich <oheim>
Wed 25 Feb 2015 07:16:25 PM UTC, comment #5: 

Well, I then adjusted the Debian build instructions to use that

http://wiki.octave.org/wiki/index.php?title=Octave_for_Debian_systems&diff=5642&oldid=5628

Carnë Draug <carandraug>
Project Member
Wed 25 Feb 2015 06:58:38 PM UTC, comment #4: 

So in the grand scheme of things, this only really affects Debian users and only (demonstrated so far) with the hdf5 header files. And there is an easy workaround. And rest assured the Debian package will be patched so that mkoctfile will work when 4.0 is apt-gettable.

Mike Miller <mtmiller>
Project Administrator
Wed 25 Feb 2015 06:53:43 PM UTC, comment #3: 

It's worth noting that flags in the user's CPPFLAGS and LDFLAGS (as well as CFLAGS and CXXFLAGS) are represented in mkoctfile. So a workaround that I think should work would be to build Octave like this:

$ ../configure CPPFLAGS=-I/usr/include/hdf5/serial LDFLAGS=-L/usr/lib/x86_64-linux-gnu/hdf5/serial

instead of like this

$ ../configure --with-hdf5-includedir=/usr/include/hdf5/serial --with-hdf5-libdir=/usr/lib/x86_64-linux-gnu/hdf5/serial

And when bug #43180 is fixed this should no longer matter, because hdf5.h won't be part of the Octave public API anymore. I don't know if this same bug exists for other external libraries (fftw, suitesparse, etc). At some point I would like to clean up the Octave API and ensure that we're not including things that don't need to be part of the interface.

So it's only the library-specific options that are not passed on to build every oct-file that the user might want to build. I think this is correct, frankly, because I shouldn't have to build my oct-file against hdf5 and fftw and suitesparse and arpack and ... if I don't use any of their functions.

I'm not saying we shouldn't fix this, but we should be cautious that we don't force unnecessary include and link dependencies on a simple hello-world oct-file.

Mike Miller <mtmiller>
Project Administrator
Wed 25 Feb 2015 06:25:53 PM UTC, comment #2: 

I experience the very same problem on Debian Jessie and the development branch. The octave_config_info command knows the correct HDF5_CPPFLAGS and HDF5_LDFLAGS, which have been set during compilation of GNU Octave. The mkoctfile command always fails.

Oliver Heimlich <oheim>
Wed 25 Feb 2015 06:09:23 PM UTC, comment #1: 

Probably any flags that would normally be passed when building Octave libraries should be handled in mkoctfile the same way they are handled in Octave's Makefiles.

Flags that are just for individual .oct files should probably not be included in mkoctfile's default flags.

These things seem to diverge over time.  I can take a look at it.

John W. Eaton <jwe>
Project Administrator
Wed 25 Feb 2015 05:23:38 PM UTC, original submission:  

I may be wrong on this premise but I was under the impression that part of the job of mkoctfile was setting up all the flags required to build oct files. Part of this job would be replicating the compiler flags used to build Octave.

However, this does not seem to be the case. Because of the place where hdf5 libraries are installed in Debian systems (see bug #38928), one needs to set --with-hdf5-includedir=... and --with-hdf5-libdir=... during configure.  These are not passed on to mkoctfile. I checked all the flags available to print from mkoctfile and I could not find them anywhere.

The end result is that package installation fails because hdf5 headers are not in the CPP path even though the package has nothing to do with hdf5 (side effect of bug #43180)

Carnë Draug <carandraug>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #33192:  diff-more-hdf5.txt added by mtmiller (5KiB - text/plain)
file #33190:  diffs-with-new-files.txt added by jwe (65KiB - text/plain)
file #33189:  diffs.txt added by jwe (63KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by oheim (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by carandraug (Submitted the item)
  •  

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

    Only project members can vote.

     

     

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-02-27 mtmiller StatusNone => Fixed
        Open/ClosedOpen => Closed
    2015-02-26 mtmiller Attached File- => Added diff-more-hdf5.txt, #33192
    2015-02-25 jwe Attached File- => Added diffs-with-new-files.txt, #33190
    2015-02-25 jwe Attached File- => Added diffs.txt, #33189

    Back to the top


    Powered by Savane 3.5