Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SuiteSparse] DONOTMERGE Update to v6 #6006

Closed
wants to merge 17 commits into from

Conversation

rayegun
Copy link
Contributor

@rayegun rayegun commented Dec 17, 2022

No description provided.

@rayegun
Copy link
Contributor Author

rayegun commented Dec 17, 2022

@gbaraldi any idea how to fix the sanitization error?

@imciner2
Copy link
Member

The error appears to be actually caused by BinaryBuilderBase. It appears that it is generating the same compiler/link flags for both the C compiler and the fortran compiler. This means it generates the sanitize options for Clang (which can support MSAN) and then uses those for gfortran as well - but gfortran (and GCC in general) doesn't support MSAN. It looks like the gfortran flags need to be modified to prevent the -fsanitize=memory from being appended to them during the environment setup.

@imciner2
Copy link
Member

@Wimmerer can you try unsetting the FC environment variable on the memory sanitizer platform? It looks like the SuiteSparse build system will ignore its internal Fortran files if there is no compiler set, and these files appear to not be necessary for its operation (it looks like the actual library is in C and those just provide an interface to the C library for Fortran code, but I hope @DrTimothyAldenDavis can confirm).

@DrTimothyAldenDavis
Copy link

The cmake script can work without a Fortran compiler.

I use the Fortran compiler in SuiteSparse_config/cmake_modules/SuiteSparseBLAS.cmake to determine how C calls Fortran (append an underscore, or not, etc). By default, for example, C calls the Fortran dgemm as dgemm_. If no Fortran compiler is available, then you can define -DBLAS_NO_UNDERSCORE, or -DBLAS_UNDERSCORE, to select how C calls the Fortran BLAS.

I also use the Fortran compiler to compile optional parts of the AMD library, and demo programs for UMFPACK, CHOLMOD, and AMD. Those are all optional.

@imciner2
Copy link
Member

Thanks for confirming @DrTimothyAldenDavis.

So, @Wimmerer I think then since FC= didn't work we have to get the hammer out and just set -DCMAKE_Fortran_COMPILER="" on the CMake command line to make it forget about the Fortran compiler altogether (I had hoped that nulling out the FC environment variable would cause CMake to then null out CMAKE_Fortran_COMPILER since the documentation claims one is set from the other, but that clearly didn't work). If that still doesn't work, it looks like the only way to unblock this is to actually modify BinaryBuilderBase to handle Fortran in the MSAN platform properly.

@DrTimothyAldenDavis
Copy link

If setting CMAKE_Fortran_Compiler to an empty string fails, I could add a option like -DNFORTRAN to SuiteSparse_config/cmake_modules/SuiteSparsePolicy.cmake.

Will Kimmerer and others added 2 commits December 29, 2022 10:01
Co-authored-by: Ian McInerney <ian.s.mcinerney@ieee.org>
@rayegun
Copy link
Contributor Author

rayegun commented Dec 29, 2022

@DrTimothyAldenDavis can you add a -DNFORTRAN option?

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned already on Slack, this should have a SuiteSparse@6 directory next to the existing one, instead of replacing it. For stdlibs we often have to come back and rebuild an old version.

@giordano
Copy link
Member

And in any case merging this should wait for JuliaLang/julia#47676 to be resolved, I don't want to throw multiple problems in the mix at the same time.

@giordano giordano marked this pull request as draft December 29, 2022 18:01
@DrTimothyAldenDavis
Copy link

I've added -DNFORTRAN. Try this version: https://github.com/DrTimothyAldenDavis/SuiteSparse/releases/tag/v6.0.4.beta1 just tagged. Also includes the v7.4.1.beta2.

These versions have passed my basic tests. I haven't yet tried them in my brutal tests but I'll do that before I tag them as stable releases.

@DrTimothyAldenDavis
Copy link

See https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER.html where it states that the CMAKE__COMPILER variable cannot be changed once set. So it looks like the NFORTRAN option is the best way to tell SuiteSparse not to even try using a Fortran compiler.

@imciner2
Copy link
Member

See cmake.org/cmake/help/latest/variable/CMAKE_LANG_COMPILER.html where it states that the CMAKE__COMPILER variable cannot be changed once set.

I had seen that, but I think we regenerate the build root every time we try a build, so I was thinking it should be set to an empty string the first time and then it would be fine. Apparently there is something else going on in the binary builder build root that is messing with the variable, though, so an explicit disable switch would seem to be the best option. Thanks for implementing that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants