bugGNU Octave - Bugs: bug #60016, libqhull was deprecated in favor...

 
 

bug #60016: libqhull was deprecated in favor of libqhull_r

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Sat 06 Feb 2021 03:18:31 PM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 26 Nov 2022 08:10:49 PM UTC, comment #37: 

Finally removed building the non-reentrant version of QHull in MXE Octave here:
https://hg.octave.org/mxe-octave/rev/5c46967d43cc

Markus Mützel <mmuetzel>
Group administrator
Mon 19 Jul 2021 08:16:34 PM UTC, comment #36: 


comment #35:

> Octave 6 is currently on the stable branch. The stable branch is mainly for fixing bugs or regressions found in the currently stable version of Octave.


qhull non-reentrant is deprecated, and I was not able to build octave 6.3.0 (stable) against current qhull version. The old patch available here doesn't work with octave 6.3.0 (worked with 6.2.0), this is why I uploaded the patches, so that distros or individuals can build octave stable with current qhull.

Andre da Costa Barros <andrecbarros>
Mon 19 Jul 2021 04:26:41 PM UTC, comment #35: 

Octave 6 is currently on the stable branch. The stable branch is mainly for fixing bugs or regressions found in the currently stable version of Octave.
Changes that add new features or new dependencies should be done on the default branch instead.
Of course, there are exceptions to that rule of thump.
One of those is e.g. if a new dependency fixes a bug.
Are there any particular bugs that that are present when using libqhull and that are fixed only when using libqhull_r?

Markus Mützel <mmuetzel>
Group administrator
Mon 19 Jul 2021 04:19:39 PM UTC, comment #34: 

I updated the patches to be applied to octave-6.3.0, also run the tests and no errors were found.


(file #51672, file #51673)

Andre da Costa Barros <andrecbarros>
Thu 13 May 2021 09:45:10 AM UTC, comment #33: 

Thanks for the feedback. I pushed the patch with your proposed changes here:
https://hg.savannah.gnu.org/hgweb/octave/rev/1564f617da7e

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Sun 09 May 2021 11:37:08 PM UTC, comment #32: 

I think I would substitute "requires" for "uses".  The second sentence would then read "Octave now requires the re-entrant version of that library "libqhull_r" instead."

I also would put a hyphen (-) between the prefix "re" and "entrant".  It's not universal, but in English when there might be confusion about a prefix joining with the word that follows a hyphen, or sometimes an umlaut, is often used.

Rik <rik5>
Group administrator
Sun 09 May 2021 06:02:32 PM UTC, comment #31: 

The attached patch adds the following paragraph to the NEWS file:

- The non-reentrant version of the QHull library "libqhull" was
deprecated upstream.  Octave now uses the reentrant version of that
library "libqhull_r" instead.


Is the wording ok?


(file #51406)

Markus Mützel <mmuetzel>
Group administrator
Sun 09 May 2021 05:14:16 PM UTC, comment #30: 

Well, it took a while but I think we finally have a nice solution here.  One last suggestion would be to update the NEWS file to mention that the Qhull library requirement has now changed to be the re-entrant version of the library.

Rik <rik5>
Group administrator
Sat 08 May 2021 06:01:40 PM UTC, comment #29: 

I like that solution. It didn't cross my mind that we could just copy the values of the auto-generated variables to variables with the old names.

Markus Mützel <mmuetzel>
Group administrator
Sat 08 May 2021 05:11:28 PM UTC, comment #28: 

I pushed a fix that makes sense for me on the default branch:

https://hg.savannah.gnu.org/hgweb/octave/rev/75556567592e

I really think this isn't much of an ugly workaround and remains fairly simple and consistent with what we do with other library special cases. If y'all decide later that you don't care about the old name anymore, it's easy enough to remove and start using the new variable names instead.

Mike Miller <mtmiller>
Group Member
Sat 08 May 2021 04:14:58 PM UTC, comment #27: 

I agree--no special autoconf macros for unique cases.

I think your small patch which renames QHULL_R_XXX flags to QHULL_XXX flags is a reasonable compromise.

However, I'm also now leaning towards just changing the flag names to QHULL_R_XXX.  Octave now requires the re-entrant version of the library, correct?

I previously used the example of BLAS_LIBS as an argument for keeping the same flag names (QHULL_XXX) regardless of the underlying implementation.  That makes sense for BLAS where the implementation could be reference BLAS, OpenBLAS, ATLAS, or something else, but the API is exactly the same and there are no code changes required.

In this case, the re-entrant QHULL library required us to change our code.  That makes me think we should use the specific flag names that match the library we are building against.

Rik <rik5>
Group administrator
Sat 08 May 2021 09:12:29 AM UTC, comment #26: 

I pushed a change to the configure test here:
https://hg.savannah.gnu.org/hgweb/octave/rev/162abe58aaef

With that, QHull seems to be detected correctly on macOS. Also the respective tests in the test suite seem to run and pass on that platform now. (If we still needed proof that the structure needs to be initialized, that's it. The example on the QHull page might be incomplete.)


IIUC, those make variable names are generated automatically from the library name by our OCTAVE_CHECK_LIB macro.
It'd be possible to have a specialized macro just for QHull to have make variables that are not automatically derived from the library name. But I'm not sure if it is worth the additional maintenance.

The instances where the old make variable names are still used should be fixed though.
We could still keep the old names for the variables in build-env.h and build-env.cc. See the attached patch.

IIUC, the make variable names are internal to the build system and probably shouldn't concern users. I agree that that's different for the fields in the `__octave_config_info__` structure.

Is this a good compromise? Or will this cause more confusion than what it will help?
What do you think?


(file #51401)

Markus Mützel <mmuetzel>
Group administrator
Fri 07 May 2021 08:05:09 PM UTC, comment #25: 

I assume it would be reasonable to keep the base name of the library for the flags.  The BLAS library could be provided by various alternatives, but BLAS_LIBS variable is still used to say what the linker flag should be.

Even for a configure test, might as well write good code.  I would initialize qhT in the same way in the configure test that we do for the main code.

Rik <rik5>
Group administrator
Fri 07 May 2021 07:52:22 PM UTC, comment #24: 

The Qhull variables aren't substituted correctly in libinterp/build-env.cc now.


>> disp (__octave_config_info__ ('QHULL_LIBS'))
@QHULL_LIBS@


Would it be reasonable / possible to keep using the make variables QHULL_CPPFLAGS, QHULL_LDFLAGS, and QHULL_LIBS, even though the library is now libqhull_r?

It's fine with me if the include guard is something like HAVE_LIBQHULL_R_H, since that's derived directly from the file name. But it seems better to me that the FLAGS and LIBS variables remain named simply "QHULL".

Mike Miller <mtmiller>
Group Member
Fri 07 May 2021 06:12:21 PM UTC, comment #23: 

The macOS runner says the following during configure:

checking for libqhull_r/libqhull_r.h... yes
checking for qh_qhull in -lqhull_r... yes
checking for qh_version in -lqhull_r... yes
checking whether the qhull_r library works... no
configure: WARNING: Qhull library found, but does not seem to work properly.  This will result in loss of functionality for some geometry functions.  Please try recompiling the library with -fno-strict-aliasing.


It uses the Homebrew QHull libraries. Afaict, that is version 2020.2.
Is the warning correct? Does it mean the library is built in a way that is incompatible with how we use it?
IIUC, this is their build rule:
https://github.com/Homebrew/homebrew-core/blob/HEAD/Formula/qhull.rb

Could something be wrong in our configure test?
Afaict, the qhT structure isn't initialized in the test. Should we do that to be on the safe side?

dnl
dnl Check whether Qhull works (does not crash).
dnl
AC_DEFUN([OCTAVE_CHECK_LIB_QHULL_OK], [
  AC_CACHE_CHECK([whether the qhull_r library works],
    [octave_cv_lib_qhull_r_ok],
    [AC_RUN_IFELSE([AC_LANG_PROGRAM([[
        #include <stdio.h>
        #if defined (HAVE_LIBQHULL_R_LIBQHULL_R_H)
        # include <libqhull_r/libqhull_r.h>
        # include <libqhull_r/qset_r.h>
        # include <libqhull_r/geom_r.h>
        # include <libqhull_r/poly_r.h>
        # include <libqhull_r/io_r.h>
        #elif defined (HAVE_LIBQHULL_R_H)
        # include <libqhull_r.h>
        # include <qset_r.h>
        # include <geom_r.h>
        # include <poly_r.h>
        # include <io_r.h>
        #endif
        #if defined (NEED_QHULL_R_VERSION)
          char *qh_version = "version";
        #endif
        ]], [[
        int dim = 2;
        int n = 4;
        coordT points[8] = { -0.5, -0.5, -0.5, 0.5, 0.5, -0.5, 0.5, 0.5 };
        boolT ismalloc = 0;
        qhT context;
        qhT* qh = &context;
        return qh_new_qhull (qh, dim, n, points, ismalloc, "qhull ", 0, stderr);
      ]])],
      octave_cv_lib_qhull_r_ok=yes,
      octave_cv_lib_qhull_r_ok=no,
      octave_cv_lib_qhull_r_ok=yes)
  ])
  if test $octave_cv_lib_qhull_r_ok = yes; then
    $1
    :
  else
    $2
    :
  fi
])


Or is there something else we should do about that?

I don't have a mac and can't give any more information than what is available in the logs:
https://github.com/gnu-octave/octave/runs/2529459401

Markus Mützel <mmuetzel>
Group administrator
Fri 07 May 2021 05:07:29 PM UTC, comment #22: 

Thanks for your fast reply and for the link to that bug report. I misunderstood what that warning was about.
I changed the initialization to the following that should also 0 initialize all fields but avoids the gcc warnings:

  qhT context = { };


Also thank you for the hint to part of your patch that was still missing.

With those changes, I no longer see warnings when compiling the changed files, and the test suite is passing without any new fails or regressions.

I pushed the change here:
https://hg.savannah.gnu.org/hgweb/octave/rev/93c8df989ea0

Marking as ready for test.


Markus Mützel <mmuetzel>
Group administrator
Fri 07 May 2021 05:01:09 PM UTC, comment #21: 

I just lightly read the example from Qhull.  But I don't see that they initialize qh_qh in any way.  The entire main () function is


int main(int argc, char *argv[]) {
  int curlong, totlong; /* used !qh_NOmem */
  int exitcode, numpoints, dim;
  coordT *points;
  boolT ismalloc;
  qhT qh_qh;
  qhT *qh= &qh_qh;

  QHULL_LIB_CHECK /* Check for compatible library */

  if ((argc == 1) && isatty( 0 /*stdin*/)) {
    fprintf(stdout, qh_prompt2a, qh_version, qh_prompt2b);
    exit(qh_ERRnone);
  }
  if (argc > 1 && *argv[1] == '-' && (*(argv[1] + 1) == '?' || *(argv[1] + 1) == '-')) { /* -? or --help */
    fprintf(stdout, qh_prompt2a, qh_version, qh_prompt2b);
    exit(qh_ERRnone);
  }
  if (argc > 1 && *argv[1] == '-' && (*(argv[1] + 1) == '?' || *(argv[1] + 1) == '-')) { /* -? or --help */
    fprintf(stdout, qh_prompt2a, qh_version, qh_prompt2b);
    exit(qh_ERRnone);
  }
  if (argc > 1 && *argv[1] == '-' && !*(argv[1]+1)) {
    fprintf(stdout, qh_prompta, qh_version, qh_DEFAULTbox,
                qh_promptb, qh_promptc, qh_promptd, qh_prompte);
    exit(qh_ERRnone);
  }
  if (argc > 1 && *argv[1] == '.' && !*(argv[1]+1)) {
    fprintf(stdout, qh_prompt3a, qh_version, qh_prompt3b);
    exit(qh_ERRnone);
  }
  if (argc > 1 && *argv[1] == '-' && *(argv[1]+1)=='V') {
      fprintf(stdout, "%s\n", qh_version2);
      exit(qh_ERRnone);
  }
  qh_init_A(qh, stdin, stdout, stderr, argc, argv);  /* sets qh->qhull_command */
  exitcode= setjmp(qh->errexit); /* simple statement for CRAY J916 */
  if (!exitcode) {
    qh->NOerrexit= False;
    qh_initflags(qh, qh->qhull_command);
    points= qh_readpoints(qh, &numpoints, &dim, &ismalloc);
    qh_init_B(qh, points, numpoints, dim, ismalloc);
    qh_qhull(qh);
    qh_check_output(qh);
    qh_produce_output(qh);
    if (qh->VERIFYoutput && !qh->FORCEoutput && !qh->STOPadd && !qh->STOPcone && !qh->STOPpoint)
      qh_check_points(qh);
    exitcode= qh_ERRnone;
  }
  qh->NOerrexit= True;  /* no more setjmp */
#ifdef qh_NOmem
  qh_freeqhull(qh, qh_ALL);
#else
  qh_freeqhull(qh, !qh_ALL);
  qh_memfreeshort(qh, &curlong, &totlong);
  if (curlong || totlong)
    qh_fprintf_stderr(7079, "qhull internal warning (main): did not free %d bytes of long memory(%d pieces)\n",
       totlong, curlong);
#endif
  return exitcode;
} /* main */


It is also possible that Qhull authors provided incomplete example.

Rik <rik5>
Group administrator
Fri 07 May 2021 04:49:53 PM UTC, comment #20: 

comment #18:

> It's not clear that we need to initialize the variable "context" of type qhT.  The Qhull website has an example of using the re-entrant library.  The code is at http://www.qhull.org/src/qhull/unix_r.c.
>
> Quoting from that code,
>


> int main(int argc, char *argv[]) {
>   int curlong, totlong; /* used !qh_NOmem */
>   int exitcode, numpoints, dim;
>   coordT *points;
>   boolT ismalloc;
>   qhT qh_qh;
>   qhT *qh= &qh_qh;
>
>   QHULL_LIB_CHECK /* Check for compatible library */


You omit the relevant part of code later, which does initialize qh_qh.

> Our equivalent code is
>


> +      qhT context = { 0 };
> +      qhT *qh = &context;


>
> I'd just drop the initialization "{ 0 }".


Nope. Definitely breaks qhull when you are unlucky and some fields are not 0-initialized.

Run the following through valgrind:

#include <string>
#include <iostream>

#include <libqhull_r/libqhull_r.h>
#include <libqhull_r/geom_r.h>

int main(int argc, char* argv[])
{
    qhT context; // not initialized = { 0 };
    qhT* qh = &context;

    int numpoints = 4;
    coordT points[] = {0,0,0, 1,0,0, 0,1,0, 0,0,1};
    int dim = 3;
    bool ismalloc = false;

    std::string cmd = "qhull s FA";

    FILE* outfile = fopen("/dev/stdout", "w+");
    FILE* errfile = fopen("/dev/stderr", "w+");

    qh_new_qhull (qh, dim, numpoints, points,
                  ismalloc, &cmd[0], outfile, errfile);

    qh_getarea(qh, qh->facet_list);
    std::cout << qh->totvol << std::endl;
    std::cout << qh->totarea << std::endl;

    qh_freeqhull (qh, !qh_ALL);
}


Stefan Brüns <stefanbruens>
Fri 07 May 2021 04:43:15 PM UTC, comment #19: 

comment #17:

> I rebased the patch to the latest default branch. That needed quite a few manual changes. (I hope I got those right. See attachment.)
>
> When I compile with that patch with gcc 10 on Ubuntu 20.10 (libqhull-r8.0 2020.1-2), I see loads of warnings for each time the qhT structure is initialized:


> ../libinterp/dldfcn/convhulln.cc: In function ‘octave_value_list Fconvhulln(const octave_value_list&, int)’:
> ../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ALLOWshort’ [-Wmissing-field-initializers]
>   174 |   qhT context = { 0 };
>       |                     ^
> ../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ALLOWwarning’ [-Wmissing-field-initializers]
>...
> ../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::CDDinput’ [-Wmissing-field-initializers]
> [...]


>
> Is it ok to leave all of those member fields uninitialized? Or should we do something different?


These are not unitialized. The fields are initialized to 0, as not explicit value is given.

See e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61489

> The tests for convhulln.oct are failing:


> >>>>> processing /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.cc-tst
> ***** testif HAVE_QHULL
>  cube = [0 0 0;1 0 0;1 1 0;0 1 0;0 0 1;1 0 1;1 1 1;0 1 1];
>  [h, v] = convhulln (cube, "Qt");
>  assert (size (h), [12 3]);
>  h = sortrows (sort (h, 2), [1:3]);
>  assert (h, [1 2 4; 1 2 6; 1 4 8; 1 5 6; 1 5 8; 2 3 4; 2 3 7; 2 6 7; 3 4 7; 4 7 8; 5 6 7; 5 7 8]);
>  assert (v, 1, 10*eps);
>  [h2, v2] = convhulln (cube);  # Test default option = "Qt"
>  assert (size (h2), size (h));
>  h2 = sortrows (sort (h2, 2), [1:3]);
>  assert (h2, h);
>  assert (v2, v, 10*eps);
> !!!!! test failed
> /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: failed to load: /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: undefined symbol: qh_facet3vertex


>
> Similar errors about undefined symbols for "test convhull", "test delaunay", "test delaunayn", "test griddata", "test griddata3", "test griddatan", and "test voronoi".
> It looks like it might be a little more involved to switch to using libqhull_r. Some functions seem to no longer exist in the reentrant version of the library (or they have been renamed?).


Both versions export exactly the same symbols:


$> nm -DC /usr/lib64/libqhull_r.so.8.0 | grep qh_facet3vertex
000000000001f460 T qh_facet3vertex


Its only missing an update in module-files:
https://hg.savannah.gnu.org/hgweb/octave/file/b4906d3eeb63/libinterp/dldfcn/module-files
sed -ie 's/QHULL_/QHULL_R_/g' libinterp/dldfcn/module-files

> @Stefan: When those issues are fixed, would you like to be added to the list of contributors that appears at the beginning of the manual?


Sure

Stefan Brüns <stefanbruens>
Fri 07 May 2021 04:35:30 PM UTC, comment #18: 

It's not clear that we need to initialize the variable "context" of type qhT.  The Qhull website has an example of using the re-entrant library.  The code is at http://www.qhull.org/src/qhull/unix_r.c.

Quoting from that code,


int main(int argc, char *argv[]) {
  int curlong, totlong; /* used !qh_NOmem */
  int exitcode, numpoints, dim;
  coordT *points;
  boolT ismalloc;
  qhT qh_qh;
  qhT *qh= &qh_qh;

  QHULL_LIB_CHECK /* Check for compatible library */


Our equivalent code is


+      qhT context = { 0 };
+      qhT *qh = &context;


I'd just drop the initialization "{ 0 }".


Rik <rik5>
Group administrator
Fri 07 May 2021 03:58:32 PM UTC, comment #17: 

I rebased the patch to the latest default branch. That needed quite a few manual changes. (I hope I got those right. See attachment.)

When I compile with that patch with gcc 10 on Ubuntu 20.10 (libqhull-r8.0 2020.1-2), I see loads of warnings for each time the qhT structure is initialized:

../libinterp/dldfcn/convhulln.cc: In function ‘octave_value_list Fconvhulln(const octave_value_list&, int)’:
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ALLOWshort’ [-Wmissing-field-initializers]
  174 |   qhT context = { 0 };
      |                     ^
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ALLOWwarning’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ALLOWwide’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ANGLEmerge’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::APPROXhull’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::MINoutside’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ANNOTATEoutput’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::ATinfinity’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::AVOIDold’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::BESToutside’ [-Wmissing-field-initializers]
../libinterp/dldfcn/convhulln.cc:174:21: warning: missing initializer for member ‘qhT::CDDinput’ [-Wmissing-field-initializers]
[...]


Is it ok to leave all of those member fields uninitialized? Or should we do something different?


The tests for convhulln.oct are failing:

>>>>> processing /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.cc-tst
***** testif HAVE_QHULL
 cube = [0 0 0;1 0 0;1 1 0;0 1 0;0 0 1;1 0 1;1 1 1;0 1 1];
 [h, v] = convhulln (cube, "Qt");
 assert (size (h), [12 3]);
 h = sortrows (sort (h, 2), [1:3]);
 assert (h, [1 2 4; 1 2 6; 1 4 8; 1 5 6; 1 5 8; 2 3 4; 2 3 7; 2 6 7; 3 4 7; 4 7 8; 5 6 7; 5 7 8]);
 assert (v, 1, 10*eps);
 [h2, v2] = convhulln (cube);  # Test default option = "Qt"
 assert (size (h2), size (h));
 h2 = sortrows (sort (h2, 2), [1:3]);
 assert (h2, h);
 assert (v2, v, 10*eps);
!!!!! test failed
/home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: failed to load: /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: undefined symbol: qh_facet3vertex
***** testif HAVE_QHULL
 cube = [0 0 0;1 0 0;1 1 0;0 1 0;0 0 1;1 0 1;1 1 1;0 1 1];
 [h, v] = convhulln (cube, "QJ");
 assert (size (h), [12 3]);
 assert (sortrows (sort (h, 2), [1:3]), [1 2 4; 1 2 5; 1 4 5; 2 3 4; 2 3 6; 2 5 6; 3 4 8; 3 6 7; 3 7 8; 4 5 8; 5 6 8; 6 7 8]);
 assert (v, 1.0, 1e6*eps);
!!!!! test failed
/home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: failed to load: /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: undefined symbol: qh_facet3vertex
***** testif HAVE_QHULL
 tetrahedron = [1 1 1;-1 -1 1;-1 1 -1;1 -1 -1];
 [h, v] = convhulln (tetrahedron);
 h = sortrows (sort (h, 2), [1 2 3]);
 assert (h, [1 2 3;1 2 4; 1 3 4; 2 3 4]);
 assert (v, 8/3, 10*eps);
!!!!! test failed
/home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: failed to load: /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: undefined symbol: qh_facet3vertex
***** testif HAVE_QHULL
 triangle = [0 0; 1 1; 1 0; 1 2];
 h = convhulln (triangle);
 assert (size (h), [3 2]);
!!!!! test failed
/home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: failed to load: /home/osboxes/Documents/Repositories/Octave/octave-1/.build/libinterp/dldfcn/convhulln.oct: undefined symbol: qh_facet3vertex


Similar errors about undefined symbols for "test convhull", "test delaunay", "test delaunayn", "test griddata", "test griddata3", "test griddatan", and "test voronoi".
It looks like it might be a little more involved to switch to using libqhull_r. Some functions seem to no longer exist in the reentrant version of the library (or they have been renamed?).

@Stefan: When those issues are fixed, would you like to be added to the list of contributors that appears at the beginning of the manual?

(file #51399)

Markus Mützel <mmuetzel>
Group administrator
Fri 09 Apr 2021 01:43:49 PM UTC, comment #16: 

I just fixed bug #57727 which changed some of the code in the files that use QHull.  You will need to update to cset 29517:78ccd8bf439c before re-basing the patch.

Rik <rik5>
Group administrator
Fri 09 Apr 2021 01:21:56 PM UTC, comment #15: 

So, it seems that the standard post-C+11 does guarantee a null-byte ending which was my concern.

Could you refresh the patch then to eliminate the rejection files and then I can apply it.

Rik <rik5>
Group administrator
Fri 09 Apr 2021 12:02:24 PM UTC, comment #14: 

std::basic_string<char> (aka std::string) has been contiguous since C++03, and since C++11 is also guaranteed to be 0-terminated.

https://en.cppreference.com/w/cpp/string/basic_string

> The elements of a basic_string are stored contiguously, that is, for a basic_string s, &*(s.begin() + n) == &*s.begin() + n for any n in [0, s.size()), or,
> equivalently, a pointer to s[0] can be passed to functions that expect a pointer to the first element of a null-terminated (since C++11)CharT[] array.


The 0-terminated requirement change in C++11 was done exactly for this use case.

If you don't trust cppreference or my words, see Herb Sutters comment here:
https://herbsutter.com/2008/04/07/cringe-not-vectors-are-guaranteed-to-be-contiguous/#comment-483

Stefan Brüns <stefanbruens>
Fri 09 Apr 2021 03:41:56 AM UTC, comment #13: 

Besides refreshing the patch as mentioned in comment #12, I don't think the switch from OCTAVE_LOCAL_BUFFER to std::string is correct.


-      // Qhull flags argument is not const char*
-      OCTAVE_LOCAL_BUFFER (char, flags, 9 + options.length ());
-
-      sprintf (flags, "qhull d %s", options.c_str ());
+      std::string cmd = "qhull d " + options;

       octave::unwind_protect frame;

@@ -181,16 +178,19 @@ Undocumented internal function.

       frame.add_fcn (close_fcn, outfile);

-      int exitcode = qh_new_qhull (dim, n, pt_array,
-                                   ismalloc, flags, outfile, errfile);
+      qhT context;
+      qhT* qh = &context;
+
+      int exitcode = qh_new_qhull (qh, dim, n, pt_array,
+                                   ismalloc, &cmd[0], outfile, errfile);


The function qh_new_qhull is expecting a non-const C string which is thus of type


char *


If you want a C-delimited string (i.e., it ends with \0) then you need to use the c_str() function.  But that library function returns a const pointer to char.

The way you coded it you use the [] operator access which returns a reference to char and then you take the address of that reference to get a pointer to char.


&cmd[0]


But, there is no guarantee of any kind that the C++ standard library has organized the string internally to have a trailing null character (\0).  The C++ standard maintains the length separately from the storage so it may be that there are trailing null bytes, but I'm pretty sure it's not guaranteed.

Hence, the strategy that Octave used to create a block of memory of the correct size, put the C string in that memory block, and then use a pointer to that block in the call to qh_new_qhull.

If we're certain that qh_new_qhull will not change the string in any way then we could use the std::string c_str() function to get a properly formed C-language string and then throw away the 'const' attribute with a const_cast.  But we do need to be certain the memory won't be changed.

Rik <rik5>
Group administrator
Tue 06 Apr 2021 03:03:48 AM UTC, comment #12: 

@Stefan: Could you refresh your patch against the latest Mercurial sources?  I tried to apply your patch, but got 4 .rej rejection files.

Rik <rik5>
Group administrator
Mon 05 Apr 2021 04:20:14 AM UTC, comment #11: 

I'm confident that we could limit the conditionals to a single file by defining a wrapper class or functions.  We wouldn't necessarily need a new configure option, we could just prefer to use libqhull_r if it is available, otherwise fall back on using the old library interface.  In any case, if the consensus is that the new library interface is widely available or easily installed, then it's also fine with me to go ahead with simply switching to use it.

John W. Eaton <jwe>
Group administrator
Sun 04 Apr 2021 04:09:58 PM UTC, comment #10: 

I would also be okay in just moving ahead with this.  The change will happen on dev which will become Octave 7.0 only in 2022.  By that time, even 16.04 will be five years past introduction and past its normal End-of-Life.

The other thing to check would be whether, even if it isn't included by default, libqhull_r can be built from source on those operating systems.  I assume that organizations which have a need for those long-term releases employ competent system administrators.  As long as a sysadmin could still make Octave available on an older OS I see no issue in continuing.

I don't want the tail to wag the dog here.  I think user's of these very long-lived OSes are probably 10% or less of our base.

Rik <rik5>
Group administrator
Sat 03 Apr 2021 11:58:25 PM UTC, comment #9: 

I'm usually on the side of continuing support for ancient features for a while, or at least for one release cycle to announce the change and give users time to adapt. In this case the impact doesn't seem worth the effort to make it conditional, just to simplify it again a year later.

The big slow-moving distributions we target:

  • Ubuntu 18.04 (April 2018) and later have libqhull_r, Ubuntu 16.04 does not (even though it is Qhull version 2015.2).
  • CentOS / Red Hat 8 has libqhull_r, CentOS / Red Hat 7 does not (first release in 2014, includes Qhull version 2003.1).


if that helps to decide.

Mike Miller <mtmiller>
Group Member
Sat 03 Apr 2021 10:51:05 PM UTC, comment #8: 

libqhull_r is available since QHull 2015.1. Should Octave 6.3, released sometime in 2021 really worry about systems which are not able to provide remotely up-to-date dependencies?

Making this optional means tons of #ifdef'ery in the code, extra configure options, and confusion for any packager.

Stefan Brüns <stefanbruens>
Sat 03 Apr 2021 04:39:30 PM UTC, comment #7: 

Thanks for the patch.  Maybe we should make these changes conditional so Octave could still be built with Qhull functions on systems that only have the old library installed?

John W. Eaton <jwe>
Group administrator
Sat 03 Apr 2021 10:07:05 AM UTC, comment #6: 

The attached patch switches from the deprecated, non-reentrant libqhull to libqhull_r.

The patch compiles and passes the test suite.

(file #51179)

Stefan Brüns <stefanbruens>
Sat 03 Apr 2021 10:00:18 AM UTC, comment #5: 


comment #3:

> They write that reentrant Qhull is 1-2% slower:
> http://www.qhull.org/html/qh-code.htm#reentrant


I doubt there is even a measurable difference between reentrant and non-reentrant library versions. Rationale:

Both versions fetch any data from the qhT struct as loads relative to its base address. In case of a global struct (non-reentrant), this base address has to be loaded from memory (even the non-reentrant library is relocatable), for the reentrant version it is passed via register as first parameter. The first is done by the calee, the second by the caller. The amount of code stays the same, as can be verified by reading the assembly.

When using a non-reentrant, non-relocatable static version of libqhull this can be optimized away, but this is of course not relevant for octave.

Stefan Brüns <stefanbruens>
Sun 07 Feb 2021 11:09:01 PM UTC, comment #4: 

Octave isn't using the C++ interface so it is not mandatory to convert.  Probably should do this before the next release, but not a lot of urgency.

Rik <rik5>
Group administrator
Sun 07 Feb 2021 09:29:24 PM UTC, comment #3: 

They write that reentrant Qhull is 1-2% slower:
http://www.qhull.org/html/qh-code.htm#reentrant

They also write:

> C++ users need to convert to libqhull_r.


I'm not sure if this applies to Octave.

Markus Mützel <mmuetzel>
Group administrator
Sun 07 Feb 2021 07:41:37 PM UTC, comment #2: 

A re-entrant Qhull library is probably of minimal utility for Octave.  And it would be useful if they would quantify the speed difference between the two libraries.  The file DEPRECATED.txt says "minimal", but that means different things to different people.  Does it mean a few milliseconds or does it mean within 10% of libqhull runtime?

Regardless, we probably need to end up porting the code in case there are bug fixes which the maintainers of QHull won't bother porting back to the non-reentrant library.

I believe that Octave uses the 'C' language interface even though the surrounding code is written in C++.  It would be more of a pain if we also have to rewrite our code to use the 'C++' API.  It's not quite clear from DEPRECATED.txt.  We use qh functions, and sometimes the qh macro.

Rik <rik5>
Group administrator
Sun 07 Feb 2021 01:48:00 PM UTC, comment #1: 

As a workaround for MXE Octave, the following change builds the deprecated library and installs it:
https://hg.octave.org/mxe-octave/rev/d52c26fc0820

Markus Mützel <mmuetzel>
Group administrator
Sat 06 Feb 2021 03:18:31 PM UTC, original submission:  

The non-reentrant version of the Qhull library `libqhull` was deprecated upstream in favor of the reentrant version of the same library `libqhull_r`.

From the upstream repository:
https://github.com/qhull/qhull/blob/master/src/libqhull/DEPRECATED.txt

qhull/src/libqhull

    This directory contains the non-reentrant version of qhull, libqhull.

    New code should use the reentrant version of qhull (libqhull_r).
    It allows multiple instances of qhull to run at the same time. On
    modern architectures, it is nearly as fast as libqhull.

    Qhull programs may be built with either library.  Each program has a
    reentrant version (e.g., qconvex_r.c) and a non-reentrant
    version (qconvex.c).   The programs, rbox, qconvex, qdelaunay, qhalf,
    and qvoronoi, are built with libqhull.  The qhull program is built
    with libqhull_r.

    Qhull's C++ interface requires libqhull_r.  If you previously used the
    C++ interface with libqhull, you will need to update your code to libqhull_r.
    See qh-code.htm#convert and Changes.txt for suggestions.

    The C code in libqhull looks unusual because of the 'qh' macro.  The 'qh'
    macro controls access to Qhull's global data structure, qhT.  If
    'qh_QHpointer' is defined, 'qh' is 'qh_qh->' and 'qh_qh' is defined as
    'qhT *qh_qh', otherwise 'qh' is 'qh_qh.' and 'qh_qh' is defined as
    'qhT qh_qh'.  This allows dynamic allocation of qh_qh without modifying
    the code.  Reentrant Qhull is a better solution.

    libqhull will be supported indefinitely.  The qh_QHpointer variation
    of libqhull will be not be retested each release.  It is replaced by
    libqhull_r.


See this related note from the Debian repositories:
https://salsa.debian.org/science-team/qhull/-/blob/debian/debian/README.Debian

Deprecation Notice
------------------
The non-reentrant libqhull is no longer built by default in upstream. You
should port your software to the reentrant libqhull_r library as soon as
possible. libqhull is likely to be discontinued after the 8.0.x releases.


We should probably switch over to using libqhull_r in Octave.

This is already an issue for MXE Octave: The non-reentrant version that we use in Octave isn't built currently.
That should probably be fixed there, too (at least until we switch to libqhull_r in Octave).

Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51705:  octave-bug60016-qhull-reentrant-v6.patch added by andrecbarros (39KiB - text/x-patch - Revert to previous initialization syntax.)
file #51685:  octave-qhull_r-fixes.patch added by andrecbarros (10KiB - text/x-patch - Samll update to both patches, to cope with warnings about data initialization and default CPPFLAGS.)
file #51684:  octave-bug60016-qhull-reentrant-v5.patch added by andrecbarros (39KiB - text/x-patch - Samll update to both patches, to cope with warnings about data initialization and default CPPFLAGS.)
file #51406:  bug60016-qhull-reentrant-news.patch added by mmuetzel (827B - application/octet-stream)
file #51401:  bug60016-qhull-config-vals.patch added by mmuetzel (2KiB - application/octet-stream)
file #51399:  bug60016-qhull-reentrant-v3.patch added by mmuetzel (18KiB - application/octet-stream)
file #51181:  0001-Use-reentrant-libqhull_r.patch added by stefanbruens (18KiB - text/x-patch - Switch to libqhull_r (ver 2, fix missing 0-initialization of qh context))
file #51179:  0001-Use-reentrant-libqhull_r.patch added by stefanbruens (18KiB - text/x-patch - Switch to libqhull_r)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by andrecbarros (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by stefanbruens (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-07-28 andrecbarros Attached File- Added octave-bug60016-qhull-reentrant-v6.patch, #51705
    2021-07-23 andrecbarros Attached File- Added octave-bug60016-qhull-reentrant-v5.patch, #51684
        Attached File- Added octave-qhull_r-fixes.patch, #51685
    2021-07-19 andrecbarros Attached File- Added octave-bug60016-qhull-reentrant-v5.patch, #51672
        Attached File- Added octave-qhull_r-fixes.patch, #51673
    2021-05-13 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-05-09 mmuetzel Attached File- Added bug60016-qhull-reentrant-news.patch, #51406
    2021-05-08 mmuetzel Attached File- Added bug60016-qhull-config-vals.patch, #51401
    2021-05-07 mmuetzel StatusIn Progress Ready For Test
    2021-05-07 mmuetzel Attached File- Added bug60016-qhull-reentrant-v3.patch, #51399
        StatusPatch Reviewed In Progress
    2021-04-09 rik5 StatusPatch Submitted Patch Reviewed
    2021-04-05 rik5 StatusConfirmed Patch Submitted
    2021-04-03 stefanbruens Attached File- Added 0001-Use-reentrant-libqhull_r.patch, #51181
    2021-04-03 stefanbruens Attached File- Added 0001-Use-reentrant-libqhull_r.patch, #51179
    2021-02-07 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code