build: Ensure config.h does not undefine `sqrtf`

Submitted by Peter TB Brett on Sept. 26, 2016, 9:46 a.m.

Details

Message ID 0918260c-52db-c2e6-9b1f-8662d942f4e9@peter-b.co.uk
State New
Series "build: Ensure config.h does not undefine `sqrtf`"
Headers show

Commit Message

Peter TB Brett Sept. 26, 2016, 9:46 a.m.
When compiling for Windows using MSVC, `sqrtf` is a macro.
Previously, `configure` was detecting `sqrtf`'s availability, but then
undefining it in `config.h`, resulting in link failures.

This patch modifies `configure` to use `HAVE_SQRTF` as a feature macro
for the `sqrtf()` function, and adds an additional stanza to
`config.h` which defines a suitable alternative when `HAVE_SQRTF` is
not defined.

Signed-off-by: Peter TB Brett <peter.brett@livecode.com>

   dnl Thread local storage

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index e833e45..b70e32d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -932,9 +932,16 @@  fi
   dnl =====================================
   dnl Check for missing sqrtf() as, e.g., for Solaris 9

-AC_SEARCH_LIBS([sqrtf], [m], [],
-               [AC_DEFINE([sqrtf], [sqrt],
-                          [Define to sqrt if you do not have the 
`sqrtf' function.])])
+AC_SEARCH_LIBS([sqrtf], [m],
+               [AC_DEFINE([HAVE_SQRTF], [1],
+                          [Whether we have sqrtf()])],
+               [])
+
+AH_VERBATIM([HAVE_SQRTF_MISSING],
+            [/* If sqrtf() is missing, use sqrt(). */
+#ifndef HAVE_SQRTF
+#define sqrtf sqrt
+#endif /* HAVE_SQRTF */])

   dnl =====================================

Comments

Siarhei Siamashka Oct. 6, 2016, 4:25 a.m.
On Mon, 26 Sep 2016 10:46:50 +0100
Peter TB Brett <peter@peter-b.co.uk> wrote:

> When compiling for Windows using MSVC, `sqrtf` is a macro.
> Previously, `configure` was detecting `sqrtf`'s availability, but then
> undefining it in `config.h`, resulting in link failures.
> 
> This patch modifies `configure` to use `HAVE_SQRTF` as a feature macro
> for the `sqrtf()` function, and adds an additional stanza to
> `config.h` which defines a suitable alternative when `HAVE_SQRTF` is
> not defined.
> 
> Signed-off-by: Peter TB Brett <peter.brett@livecode.com>
> 
> diff --git a/configure.ac b/configure.ac
> index e833e45..b70e32d 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -932,9 +932,16 @@ fi
>    dnl =====================================
>    dnl Check for missing sqrtf() as, e.g., for Solaris 9
> 
> -AC_SEARCH_LIBS([sqrtf], [m], [],
> -               [AC_DEFINE([sqrtf], [sqrt],
> -                          [Define to sqrt if you do not have the 
> `sqrtf' function.])])
> +AC_SEARCH_LIBS([sqrtf], [m],
> +               [AC_DEFINE([HAVE_SQRTF], [1],
> +                          [Whether we have sqrtf()])],
> +               [])
> +
> +AH_VERBATIM([HAVE_SQRTF_MISSING],
> +            [/* If sqrtf() is missing, use sqrt(). */
> +#ifndef HAVE_SQRTF
> +#define sqrtf sqrt
> +#endif /* HAVE_SQRTF */])
> 
>    dnl =====================================
>    dnl Thread local storage

Thanks for posting the patch here.

As I asked in https://bugs.freedesktop.org/show_bug.cgi?id=97898
earlier, a bit more details would be very much welcome. Such as a
complete configure error report (then a relevant part of it may be
also included in the commit message).

There is just one thing that I don't quite understand. I'm not a
Windows person, but I believe that the MSVC compiler is not exactly
compatible with GCC command line options. And autotools generally
expect a GCC compatible compiler. So I'm quite curious about how
you managed to use autotools with the MSVC compiler for compiling
pixman.

Also if some other Windows user can confirm the bug and test your
fix, that would be perfect.
Peter TB Brett Oct. 6, 2016, 10:32 a.m.
On 06/10/2016 05:25, Siarhei Siamashka wrote:
> On Mon, 26 Sep 2016 10:46:50 +0100
> Peter TB Brett <peter@peter-b.co.uk> wrote:
>
>> When compiling for Windows using MSVC, `sqrtf` is a macro.
>> Previously, `configure` was detecting `sqrtf`'s availability, but then
>> undefining it in `config.h`, resulting in link failures.
>>
>> This patch modifies `configure` to use `HAVE_SQRTF` as a feature macro
>> for the `sqrtf()` function, and adds an additional stanza to
>> `config.h` which defines a suitable alternative when `HAVE_SQRTF` is
>> not defined.
  >>
  >> [snip]
>
> Thanks for posting the patch here.
>
> As I asked in https://bugs.freedesktop.org/show_bug.cgi?id=97898
> earlier, a bit more details would be very much welcome. Such as a
> complete configure error report (then a relevant part of it may be
> also included in the commit message).

Hi Sarhei,

There's nothing to see in the output of the configure script -- it succeeds.

> There is just one thing that I don't quite understand. I'm not a
> Windows person, but I believe that the MSVC compiler is not exactly
> compatible with GCC command line options. And autotools generally
> expect a GCC compatible compiler. So I'm quite curious about how
> you managed to use autotools with the MSVC compiler for compiling
> pixman.

We build pixman for a large number of platform/architecture 
combinations, and need to have as similar a behaviour as possible on 
each.  In order to achieve this, we:

1. Run autoconf on a suitable reference host [1] with our desired 
configuration options.  It is impossible to run ./configure for several 
of our targets.

2. Make (very few) modifications to the config.h file, and check this 
into our configuration management system. [2]

3. Ignore pixman's generated Makefiles and use other tools to actually 
compile.  On Windows we use Visual Studio. [3]

On a recent pixman update we did not initially make _any_ modifications 
to the generated config.h (this is ideal!)  Unfortunately, the Windows 
build link failed in Visual Studio because "sqrtf" was an unresolved 
symbol.  In the Windows x86 library, "sqrtf" is a macro, not a function.

The only reason this occurred because AC_DEFINE generates "#undef 
<macro>" in config.h by default (in this case, on AC_SEARCH_LIBS 
success).  In this case, the clear intent of the person who originally 
wrote the test was that if AC_SEARCH_LIBS succeeds, "sqrtf" is left 
_unchanged_.

The patch I provided makes it considerably easier for other users in our 
position to get pixman to build correctly, by making it clearer what's 
going on when reading "config.h" in order to make appropriate edits.

Do you have any specific objections to merging it?

                                        Peter

[1] I believe the config.h we're currently using was generated on an OS 
X 10.9 machine (but that's not really relevant).

[2] 
https://github.com/livecode/livecode-thirdparty/commits/develop/libcairo/src/pixman-config.h

[3] We generate all build control files with gyp.
Siarhei Siamashka Oct. 6, 2016, 11:29 a.m.
On Thu, 6 Oct 2016 11:32:00 +0100
Peter TB Brett <peter@peter-b.co.uk> wrote:

> On 06/10/2016 05:25, Siarhei Siamashka wrote:
> > On Mon, 26 Sep 2016 10:46:50 +0100
> > Peter TB Brett <peter@peter-b.co.uk> wrote:
> >  
> >> When compiling for Windows using MSVC, `sqrtf` is a macro.
> >> Previously, `configure` was detecting `sqrtf`'s availability, but then
> >> undefining it in `config.h`, resulting in link failures.
> >>
> >> This patch modifies `configure` to use `HAVE_SQRTF` as a feature macro
> >> for the `sqrtf()` function, and adds an additional stanza to
> >> `config.h` which defines a suitable alternative when `HAVE_SQRTF` is
> >> not defined.
>   >>
>   >> [snip]  
> >
> > Thanks for posting the patch here.
> >
> > As I asked in https://bugs.freedesktop.org/show_bug.cgi?id=97898
> > earlier, a bit more details would be very much welcome. Such as a
> > complete configure error report (then a relevant part of it may be
> > also included in the commit message).  
> 
> Hi Sarhei,
> 
> There's nothing to see in the output of the configure script -- it succeeds.
> 
> > There is just one thing that I don't quite understand. I'm not a
> > Windows person, but I believe that the MSVC compiler is not exactly
> > compatible with GCC command line options. And autotools generally
> > expect a GCC compatible compiler. So I'm quite curious about how
> > you managed to use autotools with the MSVC compiler for compiling
> > pixman.  
> 
> We build pixman for a large number of platform/architecture 
> combinations, and need to have as similar a behaviour as possible on 
> each.  In order to achieve this, we:
> 
> 1. Run autoconf on a suitable reference host [1] with our desired 
> configuration options.  It is impossible to run ./configure for several 
> of our targets.
> 
> 2. Make (very few) modifications to the config.h file, and check this 
> into our configuration management system. [2]
> 
> 3. Ignore pixman's generated Makefiles and use other tools to actually 
> compile.  On Windows we use Visual Studio. [3]
> 
> On a recent pixman update we did not initially make _any_ modifications 
> to the generated config.h (this is ideal!)  Unfortunately, the Windows 
> build link failed in Visual Studio because "sqrtf" was an unresolved 
> symbol.  In the Windows x86 library, "sqrtf" is a macro, not a function.

OK, basically the configure script runs on one system and then the
generated config.h is used on another system. I'm not sure if I
approve this particular approach, but the downstream maintainers
and users are surely free to experiment and use whatever works for
them. Thanks for sharing this information anyway.

And there is always more than one way to skin a cat. For example,
the others have also tried CMake:

    https://lists.freedesktop.org/archives/pixman/2016-May/004610.html

> The only reason this occurred because AC_DEFINE generates "#undef 
> <macro>" in config.h by default (in this case, on AC_SEARCH_LIBS 
> success).  In this case, the clear intent of the person who originally 
> wrote the test was that if AC_SEARCH_LIBS succeeds, "sqrtf" is left 
> _unchanged_.

Yes, this is some wacky code in the configure.ac and the function
availability detection is normally done via defining a HAVE_SQRTF
macro, just like you did.

Somebody apparently tried to be NIH-savvy with the sqrtf function
detection, and this backfired :-)

> The patch I provided makes it considerably easier for other users in our 
> position to get pixman to build correctly, by making it clearer what's 
> going on when reading "config.h" in order to make appropriate edits.

Just a side note. We still can't make any guarantees about using
the pixman build system in a non-intended way.

> Do you have any specific objections to merging it?

I'm fine with fixing this. But would prefer to get rid of the
AH_VERBATIM stuff and just directly add the

    #ifndef HAVE_SQRTF
    #define sqrtf sqrt
    #endif

chunk into the pixman-compiler.h header file. If this adjustment is
done, then the patch will be IMHO good enough to be pushed into
pixman git (unless other people have objections).

You can wait maybe a day (so that more people have a chance to
comment) and then send a v2 version of the patch. Thanks!

> 
>                                         Peter
> 
> [1] I believe the config.h we're currently using was generated on an OS 
> X 10.9 machine (but that's not really relevant).
> 
> [2] 
> https://github.com/livecode/livecode-thirdparty/commits/develop/libcairo/src/pixman-config.h
>
> 
> [3] We generate all build control files with gyp.
>
Peter TB Brett Oct. 6, 2016, 11:30 a.m.
On 06/10/2016 12:29, Siarhei Siamashka wrote:
>> The patch I provided makes it considerably easier for other users in our
>> position to get pixman to build correctly, by making it clearer what's
>> going on when reading "config.h" in order to make appropriate edits.
>
> Just a side note. We still can't make any guarantees about using
> the pixman build system in a non-intended way.

Understood. :-) I saw this as a good opportunity to help us with our 
non-supported hackery _and_ make pixman's official build system "more 
correct"!

>> Do you have any specific objections to merging it?
>
> I'm fine with fixing this. But would prefer to get rid of the
> AH_VERBATIM stuff and just directly add the
>
>     #ifndef HAVE_SQRTF
>     #define sqrtf sqrt
>     #endif
>
> chunk into the pixman-compiler.h header file. If this adjustment is
> done, then the patch will be IMHO good enough to be pushed into
> pixman git (unless other people have objections).
>
> You can wait maybe a day (so that more people have a chance to
> comment) and then send a v2 version of the patch. Thanks!

Will do!  Thanks for the feedback.

                                         Peter