Closed barracuda156 closed 8 months ago
@szhorvat pointed to the problem: https://trac.macports.org/ticket/69025#comment:2
If it is possible to pass -D__NOEXTENSIONS__
conditional on macOS and powerpc, this would fix the issue.
This is the same issue that I pointed out in https://github.com/DrTimothyAldenDavis/SuiteSparse/issues/634#issuecomment-1869541158
math.h
seems to use Real
and Imag
in some old OS X versions on some platforms. You can see this here:
https://opensource.apple.com/source/Libm/Libm-213/ppc.subproj/math.h.auto.html
As @barracuda156 says, it seems that defining __NOEXTENSIONS__
prevents the use of these non-standard symbols. __NOEXTENSIONS__
is undocumented, but it seems that defining _POSIX_C_SOURCE
would also fix this. Does SuiteSparse make use of any extensions that defining these symbols would remove? If not, it would be great to include this workaround in SuiteSparse 7.5.0 directly.
@barracuda156 Do you know which specific OS X versions / platforms require the fix, to reduce the chance of breaking something on newer OS X?
It is there from archaic versions (Libm-40.2) through 10.6.8 (Libm-315), which is the last to support PowerPC: https://github.com/apple-oss-distributions/Libm/blob/845f432413a66ea992f4797c66157c9824421719/Source/PowerPC/math.h#L704-L707
This is in 10.5.8 (Libm-292.4): https://github.com/apple-oss-distributions/Libm/blob/d9d7b69507796c31e17089f3aaedde1716a6002b/Source/PowerPC/math.h#L713-L716
So affected platforms, AFAIU, will be every macOS until 10.6.8, but only when built for ppc
and ppc64
.
What would the fix look like if it were added to SuiteSparse?
I have a single file, SuiteSparse_config/cmake_modules/SuiteSparsePolicy.cmake, (and identical copies of that file inside GraphBLAS and LAGraph cmake_modules folders). There could be an option added there. What would the test look like?
I could add something like this, but it wouldn't simplify your solution in the Portfile
option ( SUITESPARSE_NO_MAC_LIBM_EXTENSIONS "...describe me here ... " OFF )
if ( SUITESPARSE_NO_MAC_LIBM_EXTENSIONS )
add_compile_definitions ( __NO_EXTENSIONS__ )
endif ( )
I'm very reluctant to add a hard coded #define __NO_EXTENSIONS__
for all cases on all platforms, because it can be dangerous to #define
tokens that start with __
. I might break something else on another system that happens to use that token. The token __NO_EXTENSIONS__
is too generic. If the token were __NO_MAC_LIBM_EXTENSIONS__
or something that would be safer but of course we can't change that.
Maybe something like the following could be added to my SuiteSparsePolicy.cmake:
if ( APPLE )
if ( CMAKE_MACOSX_DEPLOYMENT_TARGET ... is something ) ??
add_compile_definitions ( __NO_EXTENSIONS__ )
endif ( )
endif ( )
see https://cmake.org/cmake/help/latest/variable/APPLE.html and https://cmake.org/cmake/help/latest/envvar/MACOSX_DEPLOYMENT_TARGET.html https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_DEPLOYMENT_TARGET.html#variable:CMAKE_OSX_DEPLOYMENT_TARGET
What values of CMAKE_MACOSX_DEPLOYMENT_TARGET
would trigger the need for this __NO_EXTENSIONS__
flag?
Also -- in looking through the macports configuration for SuiteSparse, I found this:
CHOLMOD has modules with different licenses. In particular, the MatrixOps, Modify, Supernodal, MATLAB, and GPU modules are GPL-2.0+, not LGPL. See the Doc/License.txt file or you can grep for SPDX, or see:
Those licenses have been in place ever since CHOLMOD was created in 2005.
Can you update the macports Portfile with the correct license?
@DrTimothyAldenDavis Thank you very much for responding.
Of course it is not needed to use the flag unconditionally for all platforms, it will be at best redundant and potentially undesirable or breaking.
This is what will be disabled on macOS < 10.7 by __NO_EXTENSIONS__
(provided anything at all was used, likely not):
If we have no issues here, then the flag is to be used for 10.6 and below.
Yes, I did look at those extensions and I don't use any of them, so __NO_EXTENSIONS__
seems safe to add.
What does the CMAKE_MACOSX_DEPLOYMENT_TARGET
variable contain? I tried adding
message ( STATUS "macos: ${CMAKE_MACOSX_DEPLOYMENT_TARGET}" )
on my M1 Mac, but the variable was empty. Is there another way to test for the Mac OSX version in cmake?
or if you're fine with the fix you have here, then this looks safe to me:
https://github.com/macports/macports-ports/commit/e485b0a0c8125cb9d8bcc6241a9c2016514188c8
you can probably also remove the KLU patch, with __NO_EXTENSIONS__
defined, right?
CMAKE_OSX_ARCHITECTURES
will work fine, since we only need this on PowerPC really. The values are ppc
and ppc64
. (Checking for CMAKE_SYSTEM_PROCESSOR
will be less robust, since it will leave Rosetta case broken.)
I am not sure if deployment target is being set automatically. I can be set from the user side (or Macports side) though. Perhaps a check for the arch is a better choice here.
you can probably also remove the KLU patch, with
__NO_EXTENSIONS__
defined, right?
Yes, that has been done already.
OK. I am not sure if this is the right thing to add, but see this commit: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/3892732a35f863b3b031295fd3f90c97720fd7e9
How is the CMAKE_OSX_ARCHITECTURES set? The variables are empty on my M1 Mac. I suppose you'll need to use
cmake -DCMAKE_OSX_ARCHITECTURES="ppc" or something?
@DrTimothyAldenDavis I'm not sure if using CMAKE_OSX_ARCHITECTURES
is reliable in all scenarios, including cross-compilation and when multiple architectures are present (e.g. producing universal binaries). But I'm not very comfortable with CMake so maybe @mmuetzel can chime in.
How about doing it by OS X version instead?
if (APPLE AND CMAKE_SYSTEM_VERSION VERSION_LESS 11)
...
endif()
OS X 10.6 is Darwin version 10.x. Thus anything less than 11 will be 10.6 and lower.
This applies the workaround not only for PPC but for all very old Darwin systems. Based on this information I expect it wouldn't be a problem if the workaround is applied on old Intel platforms in addition to PPC.
@szhorvat: That sounds reasonable. But shouldn't it be CMAKE_SYSTEM_VERSION
(instead of CMAKE_HOST_SYSTEM_VERSION
)? I'd guess the target would matter for this - not the host.
Yes, you are absolutely right. I mixed up the two. I'll edit my previous comment to avoid confusion.
@DrTimothyAldenDavis Having it empty should not create a problem (maybe PowerPC users would need to specify it, but Macports at least will do that), however given consideration for exotic builds, perhaps a suggestion of @szhorvat makes better sense then.
OK. I just added this: https://github.com/DrTimothyAldenDavis/SuiteSparse/commit/12202fa01081b7ed691811673449cdfc09dcd3b5
My M1 Mac is Darwin 22.6.0, and if I change the 11 to 23, it adds the -D__NO_EXTENSIONS__
.
I opened #690 that limits the scope of the workaround and fixes the name of the preprocessor definition.
Oops. Well, Darwin's math.h is confusing: __WANT_EXTENSIONS__
has the extra underscore so I got confused. That is one totally mangled math.h file on the Mac...
I think this is fixed now, in both the default dev branch and the development dev2 branch. Can you give it a try in macports?
A similar workaround was applied for 7.4.0 in https://github.com/macports/macports-ports/pull/22029 and according to @barracuda156 it works. I don't have the possibility to test on PowerPC. I recommend you go ahead with 7.5.0 and don't wait for us.
Thanks for the fix!
Ok. I will close this now but reopen it if it turns out that we still need some more work
@DrTimothyAldenDavis Thank you!
We've made it a bit more general, for all packages in SuiteSparse, with this update from @mmuetzel :
https://github.com/DrTimothyAldenDavis/SuiteSparse/pull/694
which is probably a good idea since none of SuiteSparse needs those extensions, and since I'm likely to use #define Real float
in UMFPACK, when I extend it to handle single precision matrices.
@DrTimothyAldenDavis I don't want to open a new issue for this small question, so I'm asking here, even though it's unrelated. Is SPEX an API-compatible drop-in replacement for SLIP_LU?
The SPEX package has had a major overhaul in its API, and it will get another revision in an upcoming release, as well, once we revise it for SPEX v3. So unfortunately it's not a simple drop-in replacement.
Once we reach SPEX v3, the API should become stable. It's been an iterative process to design this package and its API.