search

Macaulay2 / mathicgb

Compute (signature) Groebner bases using the fast datastructures from mathic.
2 stars 4 forks source link

Use googletest 1.10.0 #15

Closed d-torrance closed 4 years ago

d-torrance commented 4 years ago

As reported in #12 and #13, mathicgb's tests failed using googletest 1.8.1.

TODO: If googletest is installed, check the version to make sure that it's at least 1.10.0.

This may be a bit tricky, as the only place it appears to be hard-coded in the source code is in the root directory of the git repository containing both googletest and googlemock. The contents of this directory may or may not be available on the user's system. For example, the libgtest-dev package in Debian does not contain it. One possible idea: try to compile something using *_TEST_CASE and check for deprecation warnings?

DanGrayson commented 4 years ago

Under arch you get the version this way:

arch$ /usr/sbin/gtest-config --version
1.8.1
d-torrance commented 4 years ago

Unfortunately, that script isn't necessarily available. For example, it doesn't appeared to be built for the Debian/Ubuntu packages:

$ apt-file search gtest-config
googletest: /usr/src/googletest/googletest/scripts/gtest-config.in

That's just the original source file for the script, before the version information is substituted in during build.

I was able to figure it out though. My original idea of checking for deprecation warnings was pretty convoluted. It's much simpler to just try and build a little program that calls one of the new functions!

I just pushed a new commit to this branch with the check added.

d-torrance commented 4 years ago

The current check in Macaulay2 is okay with any version of googletest, so long as it's not 1.8.1 (see https://github.com/Macaulay2/M2/commit/9f7368a).

If this pull request were merged in its current state (after resolving the conflict), then we would need to require at least 1.10.0 because of https://github.com/Macaulay2/mathicgb/pull/15/commits/05bb211.

Which would be preferable?

mikestillman commented 4 years ago

@DanGrayson, @mahrud Which do you prefer? I am tempted to say let's require 1.10.0 or greater, so that we can make sure we compile cleanly. But I can go either way.

mahrud commented 4 years ago

On the cmake branch of M2 we use googletest 1.10 and don't get any deprecation warnings. I don't see any reason why we should require that, especially given that we compile with libgtest rather than link with it, so its presence on the computer is irrelevant. On that note, I also don't know why we're downloading it as a tarfile rather than submodule (the cmake branch uses a submodule).

d-torrance commented 4 years ago

Here's an example of the deprecation warnings I'm talking about. It probably depends on which flags are getting passed to the compiler.

From https://salsa.debian.org/science-team/mathicgb/-/jobs/740770:

In file included from /usr/src/gtest/include/gtest/gtest.h:71,
                  from src/test/MonoMonoid.cpp:8:
 /usr/src/gtest/include/gtest/gtest-typed-test.h:227:38: warning: 'constexpr bool testing::internal::TypedTestCaseIsDeprecated()' is deprecated: TYPED_TEST_CASE is deprecated, please use TYPED_TEST_SUITE [-Wdeprecated-declarations]
   227 |   static_assert(::testing::internal::TypedTestCaseIsDeprecated(), ""); \
       |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~
mahrud commented 4 years ago

Ah, I fixed that single instance of TYPED_TEST_CASE in the last PR to this repository and there are no instances of it in M2 itself.

DanGrayson commented 4 years ago

@DanGrayson, @mahrud Which do you prefer? I am tempted to say let's require 1.10.0 or greater, so that we can make sure we compile cleanly. But I can go either way.

I think compatibility with the latest version of gtest is the way to go.

DanGrayson commented 4 years ago

On the cmake branch of M2 we use googletest 1.10 and don't get any deprecation warnings. I don't see any reason why we should require that, especially given that we compile with libgtest rather than link with it, so its presence on the computer is irrelevant. On that note, I also don't know why we're downloading it as a tarfile rather than submodule (the cmake branch uses a submodule).

Because building libraries from tar files predates our use of submodules by many years.

d-torrance commented 4 years ago

Ok, I'll update this pull request so there's no conflicts with master. Then I'll work on a corresponding one for Macaulay2 to drop the "1.8.0 or less is okay" check.

d-torrance commented 4 years ago

Ok, I've rebased this branch on top of master. Ironically the only conflict was the cross-build patch I forwarded from Debian. :)

mikestillman commented 4 years ago

@d-torrance This pull request is still good to accept?

d-torrance commented 4 years ago

Yes -- just did a test build with it. It downloads googletest just fine and all the tests pass!