Skip to content

Conversation

LourensVeen
Copy link

Here are some improvements for the build system of the tests:

  • by adding to variables like CXXFLAGS instead of overwriting them, the user (or conda, or lmod) can set things like additional include and link directories, increasing the chances that we'll find the dependencies right away,
  • calling the linker directly is not recommended these days, the compiler is much better at doing it for us than we can do it ourselves,
  • make kept trying to recreate that CUDA symlink, which gave an error.

The whole symlink thing is awkward anyway, and having the kernels in separate files is a bit tricky to manage if sapporo2 is packaged separately. It seems that for OpenCL, the kernels are converted to headers and included. The conversion is also done for CUDA, but the .ptxh file is currently unused. I'm going to try to make that work again, it's much easier than putting separate files into ${PREFIX}/share/sapporo2/CUDA and then trying to figure out where that is at runtime.

Actually, I think it would be better if the user just specified which kind of integrator they wanted, rather than a path to a kernel file. Then the whole OpenCL-or-CUDA thing can be abstracted away, and the user just says "give me a 6th order integrator" with sapporo2 doing the rest. But I'm trying to do the minimum to get this packaged first, improvements and optimisations can come later if they seem urgent.

@LourensVeen
Copy link
Author

I just discovered that this is incomplete, the Makefile_ocl isn't updated. Please don't merge yet, I'll add those updates.

@jbedorf
Copy link
Collaborator

jbedorf commented Nov 2, 2023

I just discovered that this is incomplete, the Makefile_ocl isn't updated. Please don't merge yet, I'll add those updates.

Sure, just add me as reviewer when you're ready

@LourensVeen
Copy link
Author

There, that should do it.

@LourensVeen
Copy link
Author

I can't add you as a reviewer, I guess that can only be done by repository admins. I think this is ready to go though, as is #8.

Incidentally, #8 doesn't update lib/Makefile to pick up the environment, because there's another PR coming that completely replaces it, so I didn't bother.

Copy link
Collaborator

@jbedorf jbedorf left a comment

Choose a reason for hiding this comment

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

Changes look good. I can't remember what the reason was for the symlinks originally. So as long as the downstream codes are not affected (I guess they can use the older version which I'll be tagging in a bit then it should be fine).

@LourensVeen
Copy link
Author

I've been looking at the compiled-in kernels, and that seems to work fine with CUDA too once enabled. The users still pass a path, but that's just mapped to the right constant with the precompiled kernel data. If the user has a kernel of their own that they want to use, then they can still pass a path that will get used.

I haven't actually tried to build any of the codes that use Sapporo against this yet. The new build system is pretty much there, and I'm adding some headers for the public API and a make install target. Once I've got that I can make an initial Conda package definition for it, and then I'm going to leave that for a bit and try to package amuse-framework for Conda. Once we have that, I can try to package one of the codes that use Sapporo, and see if it all actually works. If it does, then I'll make another PR with the new build system. Possibly we'll need to coordinate some releases to make sure that any changes are implemented on all sides.

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