Skip to content

Algebraic kernel d examples fixes akobel #1374

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

akobel
Copy link
Member

@akobel akobel commented Aug 29, 2016

Verbatim copy from my mail to Laurent:

The CMakeLists didn't include the USE_MPFI, and in the testsuite the CGAL main lib itself is configured without MPFI.

I fixed that in cgal-dev/Algebraic_kernel_d_examples-fixes-akobel, and also added equivalent examples to AK_d/examples that use the RS AK. Shouldn't affect the testsuite yet, because AFAICS RS is currently not available in any configuration; but see below.
It's been a while since I got something to integration, and you need to refresh my mind about the procedure: do you take care of that or should I push it there if I get your ack?

On the other front, I also made a pull request for the dockerfiles where I add NTL, RS and LEDA in the Arch Dockerfiles. I don't think RS and LEDA are easily available for any other platform. If you think it's worth to do so, I can try to find out the package names of NTL for other distros; I assume it's available as a mainstream package everywhere.

Note that, for the time being and due to the problem we are investigating, I expect the new RS AK examples in cgal-dev/Algebraic_kernel_d_examples-fixes-akobel to fail to execute properly.

Added second commit for enabling MPFI / LEDA / RS in the actual AK_d testsuite (not just the examples) as well.
Note: The LEDA_USE_FILE claims to add -lX11 in the linking stage but doesn't; obviously, I'm not CMan enough to fix it in a reasonable time.

akobel added 2 commits August 29, 2016 10:39
Note: LEDA_USE_FILE claims to add -lX11 in the linking stage but doesn't;
obviously, I'm not CMan enough to fix it in a reasonable time.
@lrineau
Copy link
Member

lrineau commented Aug 29, 2016

The Travis CI build failed because the CMake run must not error out when MPFI cannot be found.


message(STATUS "This program requires the CGAL library, and will not be compiled.")

find_package( MPFI REQUIRED )
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be REQUIRED. It should be QUIET instead, so that the CMake gracefully continue if MPFI is not found. The sentence "This project requires the MPFI library, and will not be compiled." should produce a blue cell with an r in the testsuite (for a missing requirement) if MPFI is not found.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, I'll fix that. Does the same rationale apply to Boost, or is it that dependency special? I ask because lines 21-25 are generated exactly in that fashion by cgal_create_CMakeLists.

Copy link
Member

Choose a reason for hiding this comment

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

Boost libraries is a hard dependency: if they are not found, at least the basic header-only libraries we use, then the build is sure to fail. That is why the find_package for it must use REQUIRED: no need to even try the compilation, because without Boost libraries the build is sure to fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I see. In that case, I can revert line 22 to REQUIRED again. I git-grepped over some of the CMakeLists.txt in the entire library and found that this is highly inconsistent...

if ( NOT MPFI_FOUND )
message(STATUS "This project requires the MPFI library, and will not be compiled.")
return()
find_package( MPFI QUIET )
Copy link
Member Author

Choose a reason for hiding this comment

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

@lrineau Is it expected that CMake behaves differently when I leave out line 28 (separate find_package call for MPFI) and add MPFI in line 10? In that case, at least on my setup, the MPFI_USE_FILE is not loaded AFAICS.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, nobody in the CGAL project now understands what should happen when non-CGAL components are listed in the find_package(CGAL .. ) command. It was a feature added in our CMake scripts by Eric and Philipp, and they both left, without documenting that feature correctly.

It was the "small" feature Features/Small_Features/CMake-Installation-Manual (link for CGAL developers only).

I plan to remove the CMake code about those pre-configured third party libraries pretty soon (next week).

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 see. It partially works - e.g., it is certainly useful if paths to 3rd-party libs in non-standard places can be preconfigured once and for all, and the generic CMakeLists.txt with a default CMake invocation just works.
It is certainly incomplete, e.g., not all environment variables in the build process are translated to the proper CMake variables (e.g., RS{,3}_{INC,LIB}_DIR are not acknowledged for some reason, and I already bugged Eric about that while he was still around).

But if noone understands the scripts anymore, that's more or less academic - if noone can maintain it, it's doomed anyway.

@lrineau
Copy link
Member

lrineau commented Sep 22, 2016

@akobel
Copy link
Member Author

akobel commented Oct 7, 2016

The compilation errors are strange; I do not see them locally (outside of the Docker image). Will have a look asap.
On the other hand, the execution errors are expected for the time being, and exactly the motivation for adding the tests; it's the interface bug between MPFR/MPFI and CGAL that Luis, Fabrice and I hunted. There was some progress on the MPFR side in the past weeks, AFAIK, but I'm not sure when we can expect to see a proper and definitive fix upstream.

@lrineau
Copy link
Member

lrineau commented Oct 7, 2016

Thanks. I am glad you can handle that, because that is completely out of the scope, and competences, of GeometryFactory.

@lrineau
Copy link
Member

lrineau commented Oct 7, 2016

(The branch also has merging conflicts with master. You might want to rebase and push--force it.)

@akobel
Copy link
Member Author

akobel commented Oct 7, 2016

Okay, sure. BTW, I just found out that there was a new release of MPFR during my absence. The Arch repo has the updated version since Oct 1st; could be part of the problem. Not sure if it solves the initial issue, though.

If it doesn't: What is the rationale for tests that are expected to fail for the time being? Add them, not add them, or add them with a warning message (if so, where?)?

@lrineau
Copy link
Member

lrineau commented Oct 7, 2016

I do not know. I like test suite results lines that are fully green (but with real tests anyway). But I must admit that I treat lines starting by Algebraic_ like those with Kinetic_: indifference.

What bother me is that, if there are headers/classes in CGAL that are known to fail, and no one bother, then that means they are not used at all...

As I am rather indifferent (before that does not seem to impact any other parts of CGAL), I let you decide. Now that most of people from Algebraic_kernel_d/package_info/Algebraic_kernel_d/maintainer are almost gone, I consider that you are the maintainer of that package anyway.

@akobel
Copy link
Member Author

akobel commented Oct 11, 2016

Honestly, I'm very much surprised that it never affected other parts of CGAL; AFAIK, at least the core functionality of the AK (univariate solving) is still fundamental for many higher-level algorithms. I could imagine no-one using the bivariate AK, and I can believe that hardly anyone uses the RS-based kernel (despite the fact that it offers significant speedups).

Can you give me some insight what AK instantiations your clients typically use? LEDA, anyone? RS, I doubt. GMP? Probably many, but I remember Andreas telling me that GMP is a showstopper for some customers, so I wonder what they use instead...

W.r.t. me maintaining, don't expect too much - in particular if, to the best of your knowledge, nobody else really cares about those parts. Because I don't, and I don't have a lot of time to spend on it.

I understand your stance to green test suites. If no one plans to fix the issues (and I, personally, don't), I will refrain from adding tests for the RS_Ak.

@lrineau
Copy link
Member

lrineau commented Oct 17, 2016

I do not know any client using the AK. Is not it used only by specific algebraic traits classes in the arrangements?

@akobel
Copy link
Member Author

akobel commented Oct 17, 2016

Well, if none of the clients use inputs that have algebraic curves or surfaces, I guess so. Frankly, I never checked where it might be used; I can imagine that Voronoi / Delaunay do not rely on the AK_d, but use custom predicates. And FieldWithRoot is probably hardly used, too.
If you've never heard complaints about it, I assume that noone uses it...

@sloriot sloriot modified the milestones: 4.10-beta, 4-11-beta Jan 3, 2017
@lrineau lrineau modified the milestones: 4.11-beta, 4.12-beta Jun 27, 2017
@lrineau lrineau modified the milestones: 4.12-beta, 4.13-beta Jan 24, 2018
@lrineau lrineau removed this from the 4.13-beta milestone Jun 26, 2018
@lrineau lrineau added this to the Trash / Attic milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants