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

experimental NTL support for cmake #543

Merged
merged 17 commits into from
Mar 1, 2019
Merged

Conversation

dimpase
Copy link
Contributor

@dimpase dimpase commented Feb 14, 2019

for more knowledgeable in cmake people to look at. It does build on FreeBSD.

@wbhart
Copy link
Collaborator

wbhart commented Feb 14, 2019

@peter-frentrup

/opt/local # DarwinPorts
/opt/csw # Blastwave
/opt
)
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to,

FIND_PATH(NTL_INCLUDE_DIR NTL/RR.h
  HINTS
  $ENV{NTLDIR}
)

/opt/local
/opt/csw
/opt
)
Copy link
Member

Choose a reason for hiding this comment

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

FIND_LIBRARY(NTL_LIBRARY
  NAMES ntl
  HINTS
  $ENV{NTLDIR}
)

CMakeLists.txt Outdated

list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/CMake")

find_package(GMP REQUIRED)
find_package(MPFR REQUIRED)
find_package(NTL REQUIRED)
Copy link
Member

Choose a reason for hiding this comment

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

I'd make this optional as NTL with MPIR on windows 64-bit assumes LP64 and doesn't work.
Something like,

option(WITH_NTL "Build with NTL interface" OFF)
if (WITH_NTL)
    find_package(NTL REQUIRED)
endif()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does one check this ? Is it cmake -D??? for some value of ???.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, cmake -DWITH_NTL=yes would enable it.

CMakeLists.txt Outdated
@@ -182,16 +184,17 @@ endforeach()
file(GLOB TEMP "${CMAKE_SOURCE_DIR}/*.h")
list(APPEND HEADERS ${TEMP})

add_library(flint ${SOURCES})
add_library(flint SHARED ${SOURCES})
Copy link
Member

Choose a reason for hiding this comment

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

This makes it impossible to build static libraries if needed. There's an option cmake -DBUILD_SHARED_LIBS=on which would build a shared library. (You could change the default value of off in BUILD_SHARED_LIBS to on if you like)

@dimpase
Copy link
Contributor Author

dimpase commented Feb 14, 2019

Thanks - today I touched cmake configs for the 1st time - no wonder this is very far from prefect.

@dimpase
Copy link
Contributor Author

dimpase commented Feb 15, 2019

By the way, how do you test flint's cmake build?

@wbhart
Copy link
Collaborator

wbhart commented Feb 15, 2019

We don't. We had CI for a while, but it wasn't passing anyway, so we switched it off.

@dimpase
Copy link
Contributor Author

dimpase commented Feb 15, 2019

cmake build does not seem to be working on Linux (Ubuntu 18) --- as well as on FreeBSD, in fact - I overlooked this) for me,
as libntl is not getting linked in:

$ ldd lib/libflint.so 
	linux-vdso.so.1 (0x00007ffe56374000)
	libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 (0x00007f0e59edf000)
	libmpfr.so.6 => /usr/lib/x86_64-linux-gnu/libmpfr.so.6 (0x00007f0e59c5f000)
	libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007f0e59ad5000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007f0e59948000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007f0e5992e000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f0e59744000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f0e5aad2000)

whereas on the system I install libflint using apt and see

$ ldd /usr/lib/libflint.so 
	linux-vdso.so.1 (0x00007ffdbb11b000)
	libmpfr.so.6 => /usr/lib/x86_64-linux-gnu/libmpfr.so.6 (0x00007fb464072000)
	libgmp.so.10 => /usr/lib/x86_64-linux-gnu/libgmp.so.10 (0x00007fb463df1000)
	libntl.so.35 => /usr/lib/libntl.so.35 (0x00007fb4639d7000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007fb4639b6000)
	libstdc++.so.6 => /usr/lib/x86_64-linux-gnu/libstdc++.so.6 (0x00007fb46382c000)
	libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fb46369f000)
	libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fb463683000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fb463499000)
	/lib64/ld-linux-x86-64.so.2 (0x00007fb464851000)
	libgf2x.so.1 => /usr/lib/x86_64-linux-gnu/libgf2x.so.1 (0x00007fb46328b000)

So there is a linking problem. What is this? A bug in Cmake? (As NTL is a C++ library, it's less obvious for cmake what to do). Same with cmake versions 3.12.1 and 3.13.4.

@isuruf
Copy link
Member

isuruf commented Feb 15, 2019

dimpase#1

@dimpase
Copy link
Contributor Author

dimpase commented Feb 17, 2019

We don't. We had CI for a while, but it wasn't passing anyway, so we switched it off.

After the update from @isuruf it seems to be possible to build and test with NTL on linux (latest Ubunti 18, and Gentoo) and FreeBSD, however tests get stuck on fmpz_poly_factor-test-t-factor (running for 40 minutes of CPU time). Is it a known problem with this particular test?

@isuruf
Copy link
Member

isuruf commented Feb 18, 2019

You are probably building in Debug mode. Did you try cmake -DCMAKE_BUILD_TYPE=Release ?

@wbhart
Copy link
Collaborator

wbhart commented Feb 18, 2019

It tries to factor some huge polynomials. I think @isuruf is right.

I must speed that code up some day.

@dimpase
Copy link
Contributor Author

dimpase commented Feb 18, 2019

You are probably building in Debug mode. Did you try cmake -DCMAKE_BUILD_TYPE=Release ?

OK, this works, thanks.

@dimpase
Copy link
Contributor Author

dimpase commented Feb 18, 2019

I've added 11a4f5d to allow building on Gentoo, where GMP headers have a different layout to what's programmed in FindGMP.cmake. This certainly is far from ideal, but seems to make sense (and work on an instance of Gentoo I tried it on).

@isuruf
Copy link
Member

isuruf commented Feb 18, 2019

dimpase#2 changes some default options and fixes some things

@dimpase
Copy link
Contributor Author

dimpase commented Feb 20, 2019

We don't. We had CI for a while, but it wasn't passing anyway, so we switched it off.

here is a passing with tests and NTL cmake Travis build: https://travis-ci.com/dimpase/flint2/builds/101585015 from https://github.com/dimpase/flint2/tree/travis_cmake

Would you like this merged into this PR?

@wbhart
Copy link
Collaborator

wbhart commented Feb 20, 2019

Yes, go ahead and put it into this PR. Thanks for your efforts.

@isuruf
Copy link
Member

isuruf commented Feb 20, 2019

@dimpase, do you mind adding a cmake job in addition to the other CI jobs?

@dimpase
Copy link
Contributor Author

dimpase commented Feb 20, 2019

@dimpase, do you mind adding a cmake job in addition to the other CI jobs?

yes, that's what I am going to do, add things in my experimental branch to the pull request (naturally, adding them to the other CI things)

@isuruf
Copy link
Member

isuruf commented Feb 20, 2019

Thanks. I've got a branch that fixes cmake builds on windows CI (except for 4 tests) on top of this branch. I'll send that when this PR is merged.

@dimpase
Copy link
Contributor Author

dimpase commented Feb 21, 2019

OK, I think I'm done with this, please review.

@dimpase
Copy link
Contributor Author

dimpase commented Feb 25, 2019

Probably Travis script may be made more clear by using their "matrix" stuff, but this can be done later.

@wbhart
Copy link
Collaborator

wbhart commented Feb 27, 2019

Thanks. I'll wait for @isuruf to sign off on it, then I'll merge it. I'm virtually ignorant about CMake.

@wbhart wbhart merged commit 4322020 into flintlib:trunk Mar 1, 2019
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.

3 participants