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

CMake: enable using external mpark.variant #1900

Merged
merged 11 commits into from
Mar 12, 2020
Merged

Conversation

ZedThree
Copy link
Member

Fixes #1899

The following now works for me without downloading the submodules:

cmake . -B_build_external -DEXTERNAL_MPARK_VARIANT=ON \
  -Dmpark_variant_ROOT=~/Tools/mpark_variant/_build/install \
  -DEXTERNAL_FMT=ON \
  -Dfmt_ROOT=~/Tools/fmt/build/install \
  -DPACKAGE_TESTS=OFF

note that this turns off the tests because googletest is still required for the unit tests. The recommended way to build googletest is at the same time as the main project, so there is not an option to use an external version of that.

GIT_SUBMODULE=OFF can still be used to turn off updating the submodules, but is now secondary to EXTERNAL_*.

@ZedThree ZedThree added the build-system issues with make/configure/... label Jan 27, 2020
@ZedThree ZedThree added this to the BOUT-5.0 milestone Jan 27, 2020
@dschwoerer
Copy link
Contributor

With configure I can build and test everything but the unit tests.
I assume there is no similar way for cmake?

With tests enabled, it fails, but a summary is printed, including instructions on how to continue:

[...]
   Make sure that the tools/pylib directory is in your PYTHONPATH
   e.g. by adding to your ~/.bashrc file

       export PYTHONPATH=$PWD/tools/pylib/:$PYTHONPATH

*** Now run `cmake --build .` to compile BOUT++ ***

-- Configuring incomplete, errors occurred!
[...]

Is there a way around this? The whole summary should probably be dropped, as this is confusing, and building does not work.

Also CHECK changed to 3, compared to 2 with configure.

Besides that, cmake .. -DPACKAGE_TESTS=OFF -DEXTERNAL_MPARK_VARIANT=ON -DEXTERNAL_FMT=ON fine.

However, if I provide googletest, it does not work anymore:
cmake .. -DPACKAGE_TESTS=ON -DEXTERNAL_MPARK_VARIANT=ON -DEXTERNAL_FMT=ON

The can be fixed by:
cmake .. -DPACKAGE_TESTS=ON -DEXTERNAL_MPARK_VARIANT=ON -DEXTERNAL_FMT=ON GIT_SUBMODULE=OFF

However, then most tests fail. The unit tests are very noise, it seems the awk script to reduce the noise is not used. This is very annoying as it fills the buffer of my terminal with stuff I don't care, thus hiding errors.

export PYTHONPATH=/BOUT-dev/tools/pylib fixes most of the errors, however this is not required with configure.

So this does fix the issues, but there are still some other regressions compared to configure. Should I open bugs for them?

dschwoerer
dschwoerer previously approved these changes Jan 27, 2020
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
`googletest <https://github.com/google/googletest>`_. If you wish to
use an existing installation of ``mpark.variant``, you can set
``-DEXTERNAL_MPARK_VARIANT=ON``, and supply the installation path
using ``mpark_variant_ROOT`` via the command line or environment
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is not required, if the path is already known.

@dschwoerer
Copy link
Contributor

Is it intentionally that the unit tests are not run if the integrated tests fail?
Just thought it is confusing, as with autotools the unit tests run first ...

@dschwoerer
Copy link
Contributor

There are several other things, that do not seem possible, like running all tests, as well as skipping tests if requirements are not available.
Is this something which is planned?

There still seem to be some things missing in the cmake interface. The main question is, do we want to transition to cmake? I do not feel strongly about this, but I do not like the current state, where we have 2 build systems.
This way we get most of the disadvantages of both ...

@ZedThree
Copy link
Member Author

With configure I can build and test everything but the unit tests.
I assume there is no similar way for cmake?

The build-* and check-* targets still exist, so you should be able to do:

cmake --build <build_dir> --target check-integrated-tests

(or make check-integrated-tests). This will build and run just the integrated tests and their dependencies, i.e. this will not build googletest. However, googletest will still need to be present.

With tests enabled, it fails, but a summary is printed, including instructions on how to continue:

[...]
   Make sure that the tools/pylib directory is in your PYTHONPATH
   e.g. by adding to your ~/.bashrc file

       export PYTHONPATH=$PWD/tools/pylib/:$PYTHONPATH

*** Now run `cmake --build .` to compile BOUT++ ***

-- Configuring incomplete, errors occurred!
[...]

Is there a way around this? The whole summary should probably be dropped, as this is confusing, and building does not work.

What actually fails here?

I possibly have a solution to the summary always getting printed, will push soon. We could bump it up to VERBOSE, it's just on by default because that's how we do it with configure.

However, if I provide googletest, it does not work anymore:
cmake .. -DPACKAGE_TESTS=ON -DEXTERNAL_MPARK_VARIANT=ON -DEXTERNAL_FMT=ON

If you're trying to use a system installation of googletest: that's not supported. Google recommend not doing that, and building googletest at the same time as your project. As it's only the unit tests that are linked against it, and we don't install those, this shouldn't be a problem. Therefore, we always use the bundled googletest.

However, then most tests fail. The unit tests are very noise, it seems the awk script to reduce the noise is not used. This is very annoying as it fills the buffer of my terminal with stuff I don't care, thus hiding errors.

What's failing? How are you running the tests? make check uses ctest --output-on-failure, so without failures they should be very quiet. I didn't add the awk script yet, but it's not very robust against segfaults, so we should investigate other ways to reduce the output on failure.

export PYTHONPATH=/BOUT-dev/tools/pylib fixes most of the errors, however this is not required with configure.

Which errors does this fix with CMake where this isn't required with configure? Only the integrated tests use the python tools, so they must need PYTHONPATH setting whichever build system is used. If there's something else failing that shouldn't need python, then that's a bug!

So this does fix the issues, but there are still some other regressions compared to configure. Should I open bugs for them?

Sure, the CMake interface is not 100% complete yet. I'm aware of some things, but I've almost certainly missed some things.

Is it intentionally that the unit tests are not run if the integrated tests fail?

Nope! How are you running the tests?

There are several other things, that do not seem possible, like running all tests, as well as skipping tests if requirements are not available.

There's a check target that should run all the tests. Currently, it's all or nothing though, for simplicity. At some point, I'll add a way to not run the slower tests.

There's a CMake function, bout_add_integrated_test that takes a REQUIRES argument that skips the test if the value is false, for example test-io_hdf5 requires BOUT_HAS_HDF5:

If there's some tests that are being run despite missing requirements, that's a bug!

There still seem to be some things missing in the cmake interface. The main question is, do we want to transition to cmake? I do not feel strongly about this, but I do not like the current state, where we have 2 build systems.

Yes, there are definitely things currently missing. The MMS tests, for instance. I don't think CMake is perfect, but I do think it has some distinct advantages over the autoconf build system, speaking with my maintainer hat on. I would like us to eventually switch over to just the CMake, but with a transition period while the wrinkles are ironed out.

@ZedThree
Copy link
Member Author

  • Exits earlier if add_subdirectory fails for the bundled dependencies
  • Following advice from the CMake community, rename the EXTERNAL_* options to BOUT_USE_SYSTEM_*: namespaced and more descriptive.

@ZedThree ZedThree merged commit 4b0bc9e into next Mar 12, 2020
@ZedThree ZedThree deleted the optional-external-mpark branch March 12, 2020 14:41
@ZedThree ZedThree mentioned this pull request Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build-system issues with make/configure/...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants