bugGNU Octave - Bugs: bug #55937, building default with sundials 2.x...

 
 

bug #55937: building default with sundials 2.x fails in make, configure should disable sundials

Submitted by:  Mike Miller <mtmiller>
Submitted on:  Sat 16 Mar 2019 08:23:37 PM UTC  
 
Category:  Configuration and Build System Severity:  4 - Important
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  Mike Miller <mtmiller>
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)

Tue 02 Apr 2019 10:34:23 PM UTC, comment #24:

Ok, I have now tested again with

  • SUNDIALS 2.7, correctly warns only once that SUNDIALS is not compatible and will be disabled
  • SUNDIALS 3.1 without -I/usr/include/suitesparse, correctly warns that KLU isn't working, sparse functionality disabled
  • SUNDIALS 3.1 with -I/usr/include/suitesparse, no warnings, all features enabled, 'HAVE_SUNKLU' is defined
  • SUNDIALS 4.1, no warnings, all features enabled, 'HAVE_SUNKLU' and 'HAVE_SUNLINSOL_KLU' are both defined
Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 02 Apr 2019 10:04:40 PM UTC, comment #23:

Pushed on the default branch

https://hg.savannah.gnu.org/hgweb/octave/rev/b37c1656aa42

I have not exhaustively re-tested all possible combinations of SUNDIALS, but I will try to do that once more to see if it all still works.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Mon 01 Apr 2019 10:11:45 PM UTC, comment #22:

@Mike: Seems okay to push the second bit of your patch from comment #20, and then close this report.

Rik <rik5>
Project Administrator
Fri 29 Mar 2019 04:07:39 PM UTC, comment #21:

@Mike: I can confirm that your patch fixes the issue. With Sundials 2.7 installed, I get the following warning at configure time:

and the build completes successfully (without ode15i/s).

Pantxo Diribarne <pantxo>
Project Member
Thu 28 Mar 2019 05:13:29 AM UTC, comment #20:

Ok, can you test the attached patch? I think it works for me, tested again with all combinations. I think it's detecting the SUNKLU function when it should now, or the SUNLinSol_KLU function with version 4. And I don't get duplicate warning messages with sundials 2.7 now.

Does this work for you? Can we please be done with all of these sundials problems soon?

(file #46653)

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 28 Mar 2019 04:48:28 AM UTC, comment #19:

Working on yet another fix now, will post a patch here for you to test.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 28 Mar 2019 04:37:32 AM UTC, comment #18:

Ok, I see the difference, on Debian with SUNDIALS 3.1.2 and using -I/usr/include/suitesparse, with my original ordering it does not detect the 'SUNKLU' function specifically, no error or warning is printed, it would just silently disable that feature at build time.

Somehow we need to do the optional tests for KLU features after testing whether SUNDIALS will work at all, but unfortunately the same macro that tests for version 3 vs version 4 API functions also tests for KLU functions. Maybe we can move just the tests for the 'SUNLinSol_KLU' and 'SUNKLU' functions into the 'OCTAVE_CHECK_SUNDIALS_SUNLINSOL_KLU' macro?

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 28 Mar 2019 04:11:02 AM UTC, comment #17:

> checking whether SUNDIALS API provides the necessary functions... no


And I just realized I wasn't really done coming up with a suitable message here, I am open to suggestions about how to describe this test for the version 3 or version 4 API functions.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 28 Mar 2019 04:03:54 AM UTC, comment #16:

Really? I specifically tested that, also Debian with SUNDIALS 3.1.2, and everything seemed to be working for me. Did you see a problem with configure or only when you run make?

The reason I put the checks in that order was specifically to test for required features first that may short-circuit the following tests. With your change, I now get this on a system with SUNDIALS 2.7:

And then again in the configure summary at the end

I would really like to avoid these duplicate warnings if we could.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Thu 28 Mar 2019 03:44:56 AM UTC, comment #15:

I needed the following additional change to make the build work again with Sundials 3.1 on a Debian system. Otherwise, the check for SUNKLU fails.

http://hg.savannah.gnu.org/hgweb/octave/rev/5bd4ffb55b1b

John W. Eaton <jwe>
Project Administrator
Wed 27 Mar 2019 10:52:38 PM UTC, comment #14:

This is fixed now on the default branch with this change

I also pushed a followup change to make the SUNDIALS detection fail fast and avoid printing 2 or 3 warning messages if several feature tests fail

On the default branch, I can now configure and build with SUNDIALS version 2.7.0 (configure disables automatically), version 3.1.2 without -I/usr/include/suitesparse (configure warns about KLU not found), version 3.1.2 with -I/usr/include/suitesparse (no warning, all features enabled), and version 4.1.0 (no warning, all features enabled). I also tested building with '--without-sundials_nvecserial', and it avoids unnecessary tests for optional features because it already knows that the library is missing.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 27 Mar 2019 08:55:24 PM UTC, comment #13:

I have a fix for this, testing with SUNDIALS 2.7, 3.1 without -I/usr/include/suitesparse, 3.1 with -I/usr/include/suitesparse, and 4.1.

Now I'm just trying to clean up redundant warning messages for various SUNDIALS failures.

This is all on default, Octave 5.1 is not affected by this task, nothing here is holding up 5.2.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 27 Mar 2019 07:07:32 PM UTC, comment #12:

No, this bug is still open, I'm looking at configure today, but may take a little while to reorganize.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Wed 27 Mar 2019 06:44:10 PM UTC, comment #11:

The overlinking fix for bug #55956 was applied. Can this bug also be closed?

Rik <rik5>
Project Administrator
Tue 19 Mar 2019 06:57:49 PM UTC, comment #10:

See bug #55956 for solution to the overlinking bug, made on stable and merged to default.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 19 Mar 2019 05:21:21 PM UTC, comment #9:

Fixing a configuration and build issue on stable seems OK to me.

John W. Eaton <jwe>
Project Administrator
Tue 19 Mar 2019 05:09:25 PM UTC, comment #8:

Yeah, agree with both of you. When I said "builds cleanly even if the user has sundials 2", I meant to fix the false positive that Carlo mentions, still a problem right now.

I have a fix for the overlinking problem that I can commit to stable and merge to default, it's very simple and should be safe for stable, but I will happily submit as a patch for review or avoid stable altogether if you think it's too dangerous.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 19 Mar 2019 05:02:13 PM UTC, comment #7:

AFAIK The current situation is:

Octave 4 and Octave 5 build correctly with any version of sundials

if sundials is 2.7 ode15 is activated, for any other version it is correctly detected as non compatible and deactivated

Octave 6 seems to detect correctly either sundials 3 or sundials 4 and seems to set all flags correctly

last time I tried to build Octave 6 with sundials 2.7, configure detected sundials as available but then the build failed, which is consistent with your observation below.

The only thing that definitely needs fixing is the false positive detection of sundials 2.7 in
Octave 6. Octave 6 should definitely disable ode15 if sundials is older than version 3.

My further suggestion would be to also drop support for sundials 3 to avoid extra complications
related to different APIs.

Carlo de Falco <cdf>
Project Member
Tue 19 Mar 2019 04:43:02 PM UTC, comment #6:

Currently, I think Octave 6 will work with either Sundials 3 or 4.

I don't personally care about earlier versions and would only recommend making Octave work with Sundials 2.7 if it is easily done and/or necessary because we are likely to find that many distributions currently in use don't have Sundials 3.

John W. Eaton <jwe>
Project Administrator
Tue 19 Mar 2019 04:38:28 PM UTC, comment #5:

Thank you. My intent is to make sure Octave 6 configures and builds cleanly even if the user has sundials 2 installed, which means we need to account for that case and properly disable everything, including overlinking the sundials libraries. I also plan on fixing that last part on stable (the '__ode15__.oct' file should not be linked with -lsundials libs if HAVE_SUNDIALS is not set).

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Tue 19 Mar 2019 04:28:37 PM UTC, comment #4:

@mtmiller,

Re comment #2: yes,

either IDASetJacFn or IDADlsSetJacFn
and either IDASetLinearSolver or IDADlsSetLinearSolver
and either SUNLinSol_Dense or SUNDenseLinearSolver

are required.

If any of these functions is missing sundials should be disabled altogether.

Re the note at the end of comment #3 :

as for the check for current and deprecated interfaces my intention was to
only accept the new interfaces (available in sundials > 4.0.1),
but I think jwe wants to also support some prior versions as that is what
is currently distributed with debian.

my personal opinion would be that it makes sense to spend as little as
possible effort in supporting sundials 3 because by the time Octave 6 is released
sundials 4 will probably be more widely available as a package.

Carlo de Falco <cdf>
Project Member
Mon 18 Mar 2019 05:46:48 PM UTC, comment #3:

Oops, forgot to add @cdf in Cc, done now.

Carlo, does this look right to you? It looks to me like we need to reorganize the checks in configure. Specifically, configure should not define HAVE_SUNDIALS until after it tests for the current and deprecated interfaces.

The section for "Current interface" and "Deprecated interface" seems like an afterthought, maybe it should be moved into a new OCTAVE_CHECK_SUNDIALS_SUNLINSOL_API macro and called with the other CHECKs? The possible outcomes are "current", "deprecated", and "neither", and for the latter sundials should not be used.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Sat 16 Mar 2019 08:42:20 PM UTC, comment #2:

From the configure output, these are the version 4 API functions

and these are the corresponding version 3 API functions

@cdf Does this look right to you? We have to detect one or the other of these sets of functions (except for KLU, which is optional). If they are all "no" then HAVE_SUNDIALS should not be set.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Sat 16 Mar 2019 08:38:04 PM UTC, comment #1:

I looked into '__ode15__.cc' a bit more, and I see how the conditionals are set up to work with the version 3 and version 4 APIs. So for example, sundials needs to provide either of the 'SUNLinSol_Dense' or 'SUNDenseLinearSolver' functions, both serve the same purpose. But if neither function is found, then sundials is not a compatible version, and we should detect that in configure.

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.
Sat 16 Mar 2019 08:23:37 PM UTC, original submission:

The default branch only supports the new sundials API in version 3.x and later.

Trying to build the default branch on a system with sundials 2.x now, configure thinks that it found a suitable version of sundials, the feature tests think that it is good enough, but make fails with an error. This should be detected in configure and sundials disabled completely.

Here is what configure puts out with sundials 2.7.0

With all of those feature tests, and all of those "no"s, is there enough information here already to disable sundials?
If not, it looks like it should be very easy to test for the SUNMatrix or SUNLinearSolver typedefs and if they don't exist disable sundials.

Here are the compilation errors

Mike Miller <mtmiller>
Project AdministratorIn charge of this item.

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46653:  sunklu.diff added by mtmiller (3KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by mtmiller
  •  

    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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-05 rik5 StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2019-04-02 mtmiller StatusIn Progress => Ready For Test
    2019-03-28 mtmiller Attached File- => Added sunklu.diff, #46653
    2019-03-28 mtmiller StatusReady For Test => In Progress
    2019-03-27 rik5 Carbon-CopyRemoved 72865 => -
    2019-03-27 mtmiller StatusIn Progress => Ready For Test
    2019-03-27 mtmiller StatusConfirmed => In Progress
        Assigned toNone => mtmiller
    2019-03-18 mtmiller Carbon-Copy- => Added cdf
    2019-03-16 mtmiller StatusNone => Confirmed
    2019-03-16 mtmiller Carbon-CopyRemoved 80942 => -

    Back to the top


    Powered by Savane 3.4