-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Cura 7924 sip cmake build #39
Conversation
This ports the CMake build logic. It won't compile yet because We also need to change some code in the sip definition files. Contributes to CURA-7924
A convenience function which outputs a compile_commands.json showing the used compile commands. Helpful for CMake debugging. Contributes to CURA-7924
Co-authored-by: rburema <r.burema@ultimaker.com> Sip 6.5.0 uses different methods and types. Contributes to CURA-7924
- Set SIP_BUILD_EXECUTABLE from Script - Set extension of module depending on OS Contributes to CURA-7924
message(STATUS "Setting Python version to ${Python_VERSION}. Set Python_VERSION if you want to compile against an other version.") | ||
endif() | ||
if(APPLE) | ||
set(Python_FIND_FRAMEWORK NEVER) |
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.
set(Python_FIND_FRAMEWORK NEVER) | |
set(Python_FIND_FRAMEWORK NEVER) |
This could use a bit of documentation as to why this should be set to NEVER in the case of osx builds (and yes, i know, it was also like this in the old code)
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.
Because MacOS Framework python is the root of all evil ;-) and we don't want to accidentally link against it. I think the variable name and value say's it all. btw I could add a one liner of doc in there. But I don't think it will add much additional information
super().__init__(project, **kwargs) | ||
|
||
def build(self): | ||
""" Only Generate the source files """ |
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.
""" Only Generate the source files """ | |
""" Only Generate the source files """ |
Because of the tons of issues that we've had from this, I think we should have a longer writeup here as to why we went for this route.
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.
True, Was prepping an update to to doc's and build documentation in global. But I decided to not put that in this PR due to time constraints.
I'm working on that documentation as we speak and I will update when ready.
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.
This will be updated across all three repo's: pynest2d, libsavitar and pyarcus
[tool.sip.bindings.pySavitar] | ||
exceptions = true | ||
release-gil = true | ||
concatenate = 8 |
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.
Why 8? Is there a reason to not leave this at whatever the default is?
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.
Ah, wait. Looking back it makes sense. THe CMAKE does mention something about touching the 8 files. Perhaps it's a good idea to add comments here and in the cmake to say that these two things are talking about the same 8 files? I can imagine that this might cause issues in the future.
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.
Yes will update that.
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.
Because those numbers in the SIPMacros and the pyproject.toml.in should be the same. in all cases.
This PR is part of: