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

[WIP] Introduced CMake based build procedure and first unit/funcitonal tests created #193

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

ChristopherBignamini
Copy link

According to what we discussed during the call we had on March 13, I created this PR as first step for the activity focused on Grackle performance analysis and porting on GPU. I will proceed with the creation of all the necessary unit/functional tests needed to keep under control the calculation performed by the code, before any attempt of porting on GPU. Together with a couple of unit/functional tests, I have introduced in this draft PR CMake as build tool instead of the existing configure and makefiles: the CMake based build is almost complete except for the automated generation of some source files which are currently in the src/clib/generated/ folder.

@mabruzzo
Copy link
Collaborator

Hi @ChristopherBignamini, thanks for working on this PR! This looks great so far!

I just wanted to mention that I actually have an open PR (#182) that introduces a CMake build-system. In that particular regard, it duplicates a lot of the work that you have already done here (and some work you may still need to do). Would you potentially be interested in collaborating on this particular topic? I'm very happy to help resolve differences in the implementations (on my end or on your end) -- I can definitely appreciate that you may find my implementation to be less desirable for one reason or another. I definitely don't want to hinder your progress, so definitely feel free to say "no" and ignore the rest of this message (we can always sort out merge conflicts later).

Maybe it would help to have a larger conversation about this? Are you planning to participate in the user/developer meeting next week? I'm also happy to talk more about it here.

Some Background About my PR: The stated goal of my PR was to add CMake in addition to the existing build-system. At the time that I started my PR, I thought it would be hard to sell the broader community on entirely replacing the existing build-system. With that said, I would personally be very happy to see us entirely shift to CMake and I think that providing support for GPUs makes a strong case for doing this.

I bring this up to explain why my version is currently less idiomatic:

  • currently, my version uses glob commands. I choose to do that to minimize the apparent maintenance burden of supporting a CMake-based build system. But, I totally agree that explicitly listing source files is better
  • the unit tests in the src/examples directory are still driven by a Makefile. But that could definitely be changed
  • currently, my version uses some bash scripts to produce the auto-generated files. But that's about to change. PR Factor out some file-generation machinery #181 proposes changes to the existing build-system to make use of more typical "template" files like the ones that CMake & autotools use. (more on this below).

PS: I would actually encourage you to wait to look at my PR (#182) until tomorrow. I am planning to merge in my changes from PR #181 (since it seems likely that #181 will be merged into the main branch relatively soon) later today and that should definitely simplify some things.

@ChristopherBignamini
Copy link
Author

Hi @mabruzzo , thank you very much for having a look to my PR and for your comment! I'm more than happy to collaborate on the CMake build topic, I'm not an expert at all and I'm quite sure I'm not doing things in the right/best way: for example I'm not sure how to proceed with the auto-generated source files that you mentioned (I'm temporarily including it "by hand" in the source tree by skipping the generation step) and I'm having some issues with the identification of HDF5 headers/libraries. My tasks will be more focused on unit/functional tests development, performance analysis, GPU porting, etc... the transition to CMake was justified by the necessity of having a less custom build system wrt to the original one, with the automatic generation of test with GTests, etc...
Unfortunately I won't be at the user/developer meeting (is there the possibility to connect remotely?) but I guess we can discuss this topic in other ways. I will have a look to the PRs that you mentioned so that I will be aware of the work you are doing, let's keep in touch so we can plan a call or whatever you prefer for next developments. Thank you again!

@mabruzzo
Copy link
Collaborator

mabruzzo commented Apr 19, 2024

@ChristopherBignamini that sounds great!

the transition to CMake was justified by the necessity of having a less custom build system wrt to the original one, with the automatic generation of test with GTests, etc...

That definitely makes sense to me! I recently started working with GTest for another project and it's really nice!

the transition to CMake was justified by the necessity of having a less custom build system wrt to the original one, with the automatic generation of test with GTests, etc... (I'm temporarily including it "by hand" in the source tree by skipping the generation step) and I'm having some issues with the identification of HDF5 headers/libraries

Yup, I can definitely help with those areas. The HDF5 headers/libraries are pretty easy to deal with. I definitely think that handling the generation "by hand" right now might make sense. Automatically doing it takes much more work (To deal with it, I made #181 to clean up that logic in the existing build system so that #182 could make use of those changes to implement the cmake build system.1).

Unfortunately I won't be at the user/developer meeting (is there the possibility to connect remotely?) but I guess we can discuss this topic in other ways. I will have a look to the PRs that you mentioned so that I will be aware of the work you are doing, let's keep in touch so we can plan a call or whatever you prefer for next developments. Thank you again!

I should have been more clear that the "meeting" is just a zoom call. I'll get you that information shortly (the information was previously sent out on the mailing list).

Footnotes

  1. The commit-history in Introducing Experimental Supplementary CMake Build System #182 got a little messy because I made more improvements to Factor out some file-generation machinery #181 after initially creating Introducing Experimental Supplementary CMake Build System #182 and I choose to merge the changes between the branches (I didn't quite appreciate how much more messy a merge would appear to be compared to rebasing)

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.

2 participants