-
Notifications
You must be signed in to change notification settings - Fork 21
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
Make GLM include directory portable. #1074
Conversation
1b07ff0
to
3daa46a
Compare
All tests ran using C, Without using environment var
C, after deleting
C, after
|
Can't find any GLM tests in python test suite (would explain Doesn't appear SWIG has GLM mappings setup either (did this get left on hold because of the Jitify slow compilation?). Regardless, the current changes should be appropriate, as it can still be used within RTC without being directly used in Python. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs atleast one python test which uses GLM (if GLM is enabled. Call pytest.skip(message)
in the test(s) if pyflamegpu.GLM
is not enabled)
Env var should be prefixed with FLAMEGPU_GLM_INC_DIR
(unless GLM_INC_DIR
is an env var set by GLM if centrally installed, which I doubt, and even then it needs to match the version used to build flamegpu, so should be a flamegpu specific one).
Not sure the warning on module import is a good addition either. importing a python module should be quiet IMO.
Line 6 of the same file, there's already a warning for the FLAMEGPU inc dir version, I just copied similar pattern. Otherwise good suggestions, will apply later in the week when I'm back on proper FGPU stuff. GLM + py is still a bit experimental. |
That's fair (and I'd prolly lean towards getting rid of that too to be honest, but can be a future thing) |
GLM include files and license are now embedded into Python wheels when enabled. pyflamegpu.GLM now returns whether GLM support is enabled within the wheel. Added GLM_INC_DIR as portable way to locate GLM include dir
GLM is not currently covered by CI, so warning was previously unnoticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed a linux warning from ~1 year ago in a glm-only test, but otherwise looks good and tests pass for me.
pyflamegpu.GLM
now reports GLM statusFLAMEGPU_USE_GLM
is the pre-existing C macroGLM_INC_DIR
environment variableGLM_INC_DIR
is set to the location of GLM include dir within the module's installGLM_INC_DIR
environment variable to be usedGLM_INC_DIR
environment variableGLM
flag to SWIGGLM_INC_DIR
inside wheel when GLM enabled in buildCloses #1001