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

Proposing CMake Build-System Updates #1

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

mabruzzo
Copy link

Following up on our discussion from grackle-project#193, I wanted to suggest some changes to help reconcile the difference between our implementations of the cmake build-system (as a reminder, my version is found in grackle-project#182).

As I've previously mentioned, I think the stuff that you are doing is fantastic, and I definitely don't want to get in the way of your progress (if it's more convenient for you, we can always put off dealing with these differences until a later date).

Overview

I'll now provide some basic details about the changes I am proposing here. There are 2 parts of this changeset that are worth highlighting:

  1. this proposed-changeset builds on changes that are proposed as part of my Factor out some file-generation machinery grackle-project/grackle#181 pull request
    • that PR (181) overhauled a bunch of the file generation machinery (using the old build-system). It drastically simplifies the process of auto-generating files with cmake. (this is where the config/*.py files come from.
    • that PR (181) is awaiting some final review before it gets merged into the main-branch. Based on my conversation with Britton, it sounds like his going to approve it (I've already made some changes he requested)
  2. this proposed-changeset essentially merged in the changes from my Introducing Experimental Supplementary CMake Build System grackle-project/grackle#182 pull-request
    • that PR (182) is where I introduced my version of the cmake build-system. It already includes the changes from (181)
    • as I've noted elsewhere, that PR was originally written with the intention of letting us embed Grackle inside of other CMake projects. A number of naming choices were made to help facilitate this goal, such as:
      • a bunch of public-facing variables are prefixed with GRACKLE_
      • the main library-target is called Grackle::Grackle (this is standard convention for names of imported targets)
      • machinery is put in place to disable some stuff when the Grackle-build is embedded inside another cmake project (i.e. grackle is not the top-level cmake project)
      • a lot of the design choices were influenced by this book
    • Overall, I placed a lot of emphasis on compatibility with existing tests and the results produced by the existing build-system
      • the python bindings can now efficiently be built again (CI tests have been adjusted to run with the python tests)
      • the build-system doesn't force grackle to be a static library. This is still the default behavior, but now users can explicitly opt into using shared libraries (e.g. for use with python bindings) with via the standardized cmake BUILD_SHARED_LIB variable
      • when compiling a shared library (for use with grackle tests), a pseudo-makefile is produced to let the python test run through the examples
      • (there is also the option to compile with openmp - primarily for testing versions)
    • a couple of other relevant changes:
      • hdf5 linking now works more effectively.
      • files are now auto-generated correctly
      • the testable_grackle target should now be used for building unit tests (this is functionally identical to the main Grackle::Grackle target -- it just exposes an extra macro definitions necessary for some unit test)

Let me know if you are unhappy with any of these changes. I'm happy to iterate.

Other stuff

The changes to website-documentation that I added here are somewhat outdated (i.e. it presents cmake as a secondary build-system that supplements the older classic build system). You should probably just ignore it for now. I need to update that more, but I didn't want that to delay me from proposing this changeset.

For better or worse, I also reintroduced files associated with the old build-system

  • the main purpose of these is to just let you merge your changes into the upstream repository more easily/frequently/quickly (I don't think the community is quite ready to part with the old build-system quite)
  • you definitely don't need to worry about supporting your new tests on the old build system (and once you start working on the GPU porting, you definitely don't need to worry about the old build-system)

Out of curiosity, how do you typically invoke the unit-tests and functional-tests? (While I've worked with Google Test, I've never used it with CMake before). I would love to add these tests to the continuous integration file.

Closing Thoughts

Honestly, the number of files changed may seem a little overwhelming. If I were you, I would just focus on the changes to the cmake files. Most of the remaining changes relate to reintroducing old build-system files.

Again, I want to re-emphasize, that I want to make this all as easy for you as possible.

Let me know if there's something that I can do to help with this process

  • For example, if you would prefer that I only propose changes to the smallest subset of files necessary (e.g. remove the old build-system files again), I can do that fairly easily.
  • If you would prefer that I propose these changes to your test_implementation instead I can do that
  • If you would prefer a cleaner commit history, I would be happy to accommodate that as well (now that I've done this reconciliation once, it will be easy to do it again).

mabruzzo added 25 commits March 6, 2024 11:35
It now also deletes the build directory.
In more detail, I added support for bundling the version of libgrackle
used while building the Cython extension module into the resulting
python package.
- Historically, when we would build the Cython extension module, the
  linking step would link against the copy of libgrackle.so that was
  found in the build-tree. Then at runtime, when python loaded the
  shared library (representing the extension module), the linker would
  search for libgrackle in `LD_LIBRARY_PATHS` and in the system library
  directories.
- As a consequence of this behavior, any time you wanted to rebuild the
  python module with a newer grackle version, you had to overwrite your
  "installation" of libgrackle. Given that the grackle test-suite can
  only be executed with pygrackle, this behavior can sometimes be
  undesirable
- This commit adds support for packaging the a copy of the libgrackle
  shared library from the build-tree directly into the resulting package
  (in this scenario, the extension module will always use that
  particular version of libgrackle).
- To specify which choice to use, assign the `PYGRACKLE_PACKAGE_LOCALLIB`
  environment variable a value of 0 or 1, before building pygrackle.
This commit modifies the `setup.py` script to be able to build the
pygrackle python package against versions of libgrackle that were
compiled with the CMake build system.
…s. Autogeneration should now be a lot more transparent.
…command rather than the config/configure_file.py script
This was achieved by tweaking `doc/source/conf.py`.

This triggered a few cascading changes:
1. I needed to rename `config/query-version.py` to `config/query_version.py` so that it's contents could be properly imported
2. I needed to slightly tweak the internals of `config/query_version.py` so that the function returned values (instead of directly printing the value). To reflect this change in behavior, I changed a function name from `show_version` to `query_version`
3. While doing this, I also fixed a bug in some functionality from `config/query_version.py` that was intended to strip off a trailing '\n' character from the result of a shell command. In some cases, a trailing line-break might not be present and the functionality accidentally striped off a different character instead.
4. I needed to adjust a path in `src/clib/Makefile` to reflect the new name of the `config/query_version.py` script
This commit adds capabilities to perform a basic installation with CMake. It also adopts a policy consistent with the older build-system for naming shared libs (and making symbolic links).

This policy is a little unidiomatic, but it is probably a better strategy than what idiomatic CMake could produce (given that we don't support ABI compatability)
Previously, we also included a bunch of extra improvements. I definitely think those would be useful. But this should make the cmake PR a lot easier to review
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.

1 participant