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

Bump factory to 4.4.1 and FLINT to 3.2.1 #3698

Draft
wants to merge 9 commits into
base: development
Choose a base branch
from

Conversation

d-torrance
Copy link
Member

@d-torrance d-torrance commented Mar 15, 2025

Since there are no longer any factory-specific tarballs after version 4.4.0, we switch to downloading the entire Singular source tarball, but just building factory. Edit: There is now a factory 4.4.1 tarball on the Macaulay2 website.

We also drop the redundant gftables "library" from the autotools build since it's included with factory and fix the broken links pointed out in #3697 (comment).

This is a draft for now -- I'll remove the final commit if/when the GitHub builds succeed.

This partially addresses #3697. Edit: This PR was combined with #3699 and also bumps FLINT to 3.2.1.

@d-torrance d-torrance added build issue platform specific issues involving compiling M2, generating examples, or running tests dependencies Pull requests that update a dependency file labels Mar 15, 2025
@d-torrance d-torrance mentioned this pull request Mar 15, 2025
@d-torrance
Copy link
Member Author

This has been combined with #3699, and we bump factory and FLINT together.

@d-torrance d-torrance changed the title Bump factory to 4.4.1 Bump factory to 4.4.1 and FLINT to 3.2.1 Mar 15, 2025
@mahrud
Copy link
Member

mahrud commented Mar 16, 2025

I think Dan would ask the Singular developers and they would create a tarball for factory alone. I could be wrong though.

@d-torrance
Copy link
Member Author

@mahrud: Any idea about this error in the macOS cmake build?

CMake Error at /Users/runner/work/M2/M2/M2/BUILD/build/libraries/flint/build/CMakeFiles/CMakeScratch/TryCompile-smWOJZ/CMakeLists.txt:28 (target_link_libraries):
  Target "cmTC_e9203" links to:

    gmp::gmp

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.



CMake Error at /usr/local/share/cmake/Modules/Internal/CheckSourceCompiles.cmake:108 (try_compile):
  Failed to generate test project build system.
Call Stack (most recent call first):
  /usr/local/share/cmake/Modules/CheckCSourceCompiles.cmake:58 (cmake_check_source_compiles)
  CMakeLists.txt:156 (check_c_source_compiles)

@mahrud
Copy link
Member

mahrud commented Mar 17, 2025

Seems like Flint's cmake build can't find gmp, but no clue why.

@d-torrance
Copy link
Member Author

It looks like FLINT has stopped supporting their CMake build outside of Windows (flintlib/flint#2256). I just pushed a commit switching our CMake build to use their autotools build.

-DIPO_SUPPORTED=OFF # TODO: because of clang; see https://github.com/wbhart/flint2/issues/644
-DWITH_NTL=ON
INSTALL_COMMAND ${CMAKE_COMMAND} --install . ${strip_setting}
CONFIGURE_COMMAND cd ${CMAKE_SOURCE_DIR}/submodules/flint &&
Copy link
Member

Choose a reason for hiding this comment

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

I think the right way is to change the BINARY_DIR above.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see what you're doing ... I think this is non-idiomatic, but I don't know what's the right way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm just going to switch it to an in-tree build. It turns out that the out-of-tree builds are broken and aren't installing all the header files, which is why the cmake-ubuntu build was failing (factory couldn't find flint.h). I reported it upstream, so it should be fixed next release. I'll mention something about bootstrap.sh, too.

@d-torrance d-torrance force-pushed the factory branch 2 times, most recently from ff6e099 to 5471b49 Compare March 17, 2025 20:44
@@ -11,6 +11,7 @@
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wconversion"
#include <flint/flint.h> // for flint_free, flint_rand_t, fmpz_t
#include <gmp.h> // required for FLINT functions that use GMP
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we include gmp.h directly, instead you should probably include math-include.h.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do in a few places:

$ grep -r "<gmp.h>"
NCAlgebras/FreeAlgebra.hpp:#include <gmp.h>                      // for mpz_srcptr, mpq_srcptr
NCAlgebras/FreeAlgebraQuotient.hpp:#include <gmp.h>                      // for mpz_srcptr, mpq_srcptr
schreyer-resolution/res-f4-m2-interface.cpp:#include <gmp.h>                                          // for mpz_clear

I don't see the advantage of including another header that includes even more headers we won't be using.

Copy link
Member

Choose a reason for hiding this comment

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

Not really: the first two include math-include.h via ringelem.hpp, the third includes it via gbring.hpp, so in all cases including gmp.h is redundant and should be removed.

The point is to make sure every time certain libraries are loaded, the same macros.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated that commit with your suggestion, and also removed the redundant gmp.h includes from those other files.

@d-torrance
Copy link
Member Author

d-torrance commented Mar 18, 2025

There's some kind of floating point exception in the ARingQQFlint.arithmetic test in macOS that will be a pain to debug...

@d-torrance d-torrance force-pushed the factory branch 6 times, most recently from b414d29 to d914ad3 Compare March 18, 2025 22:33
Note that no factory-specific tarball is available any more, so we add
a "make dist" target to create one.
Use the gftables shipped with factory rather than relying on an old
tarball on our website.
license file changed from LICENSE -> COPYING
Required for certain FLINT functions that use GMP since FLINT 3.2.0

We actually include M2/math-include.h, which includes gmp.h.

Also remove some redundant includes of gmp.h from other files in the e
directory.
The order matters when building with static libraries -- LDFLAGS was
too early.
Their cmake build is deprecated outside Windows.
@d-torrance d-torrance force-pushed the factory branch 3 times, most recently from 59741af to 45a2560 Compare March 19, 2025 02:36
They might be there if we've built them but haven't installed them
yet.  We need topBuilddir to be a global variable for this to work.

Also, use "nonnull splice" instead of "join" to slightly simplify
generating the list of paths to try.
@mahrud
Copy link
Member

mahrud commented Mar 20, 2025

The msolve error could be unrelated.

Could you fix these warnings, though?

In file included from /home/linuxbrew/.linuxbrew/opt/flint/include/flint/flint.h:197,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/aring-qq-flint.hpp:13,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/aring-qq.hpp:6,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/aring-translate.hpp:18,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/aring-glue.hpp:7,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/gbring.cpp:9:
/home/linuxbrew/.linuxbrew/opt/flint/include/flint/longlong.h:74:9: warning: "__ll_B" redefined
   74 | #define __ll_B ((ulong) 1 << (FLINT_BITS / 2))
      |         ^~~~~~
In file included from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/recint/recdefine.h:145,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/recint/ruruint.h:43,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/recint/rumanip.h:42,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/recint/ruconvert.h:21,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/gmp++/gmp++_int.h:26,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/gmp++/gmp++.h:57,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/givaro/givinteger.h:19,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/givaro/givpoly1dense.h:22,
                 from /home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/givaro/givpoly1.h:38,
                 from /home/mahrud/Projects/M2/quickfix/M2/BUILD/build/usr-host/include/fflas-ffpack/ffpack/ffpack.h:38,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/aring-zzp-ffpack.hpp:17,
                 from /home/mahrud/Projects/M2/quickfix/M2/Macaulay2/e/aring-translate.hpp:17:
/home/linuxbrew/.linuxbrew/Cellar/givaro/4.2.0_1/include/recint/reclonglong.h:46:9: note: this is the location of the previous definition
   46 | #define __ll_B ((UWtype) 1 << (W_TYPE_SIZE / 2))
      |         ^~~~~~

@d-torrance
Copy link
Member Author

So for the msolve errors, we first get the following:

sh: line 1: 55919 Abort trap: 6           (core dumped) /usr/local/bin/msolve --help > /dev/null 2>&1
 -- warning: could not find msolve with version at least v0.7.0

This looks like it's happening during:

msolveProgram = findProgram("msolve", "msolve --help",
MinimumVersion => (msolveMinimumVersion, "msolve -V"),
RaiseError => false,
Verbose => debugLevel > 0)

This hasn't been a problem until the last day or so, so it seems likely to be related to the FLINT transition.

But the actual error is coming later:

stdio:3:30:(3): error: msolve returned an error (32512):
sh: /usr/bin/msolve: No such file or directory

Why is it trying to call /usr/bin/msolve, when brew installed it in /usr/local/bin? I think it's because of the next few lines:

if msolveProgram === null then (
printerr("warning: could not find msolve with version at least v" | msolveMinimumVersion);
-- note: msolve -h returns status code 1 :/
msolveProgram = findProgram("msolve", "true", Verbose => debugLevel > 0))

true is located in /usr/bin/, so that's where findProgram thinks it found msolve. I don't think we want to use true here. Was the goal of this piece of code to support older versions of msolve? (IIRC, they only added the -V option to query the version number in a pretty recent version.)


Regarding the compiler warnings, those should be fixed by using the latest givaro (see #3697 (comment)).

@mahrud
Copy link
Member

mahrud commented Mar 20, 2025

true is located in /usr/bin/, so that's where findProgram thinks it found msolve.

LOL I didn't realize this was the behavior. Shouldn't it check the directory where the first argument is found?

@d-torrance
Copy link
Member Author

Shouldn't it check the directory where the first argument is found?

This first argument may not be an actual program -- it's just a string identifier. This is to deal with things like 4ti2 and TOPCOM that are more like suites of programs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build issue platform specific issues involving compiling M2, generating examples, or running tests dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants