search

grackle-project / grackle

The Grackle chemistry and cooling library for astrophysical simulations and models.
Other
26 stars 50 forks source link

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

Open ChristopherBignamini opened 5 months ago

ChristopherBignamini commented 5 months ago

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 commented 5 months ago

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:

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 commented 4 months ago

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 commented 4 months ago

@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).

[^1]: The commit-history in #182 got a little messy because I made more improvements to #181 after initially creating #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)