-
Notifications
You must be signed in to change notification settings - Fork 49
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
Fixes to CMake scripts #950
Conversation
Also remove LD_LIBRARY and DYLY_LIBRARY search paths from TBB library search path.
Fixes on this pull request might help a future cibuildwheel macos-arm64 build. However, such a build will require significant additional work, such as providing a cross-compiled TBB. |
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.
Just a couple small things.
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 all looks good. Are we able to start deploying macos-arm builds for conda-forge and pypi in the next release after these changes are merged?
I'll leave pypi support up to you. I'm not sure how much effort that will take to enable. I'm happy to test any build artifacts you create when testing. I would like at least to see macos-arm64 conda-forge builds in the next release. The MoSDeF tools are currently unable to support macos-arm64 due to upstream dependencies, one of which is freud. |
@joaander if this is change isn't extremely time-sensitive, I'd appreciate keeping this open for a few days to let me review it. I have been doing some work on scikit-build, and if the issues that you are fixing are more broadly applicable I would like to see if at least some of them can/should be upstreamed. If the MoSDeF timeline is tight I can always review the changes later. |
No, this is not extremely time sensitive. Feel free to reach out on Slack if you have questions that are not answered in the description. I didn't consider pushing these upstream as they are a big change from the behavior of |
Vyas, have you had a chase to look at these changes? |
I started looking at this last night. The
Now that I've read this a little more in detail I'm fine pushing it forward in the near term. My only concern is whether architecture-specific linking logic is actually correct on all architectures. Could someone try installing the new conda-forge packages on an Intel Mac and a Linux box (and Windows if that's easy, although we don't need to be blocked on that) and make sure that the new packages work? If they do then this PR gets a 👍 from me. |
I think so, but I don't understand how dynamic library loading works on windows as well as I do for Linux. But this is the behavior that pybind11 uses in
I'm less sure on this. I don't know why a
It definitely produces binaries that link to
It should be correct, as it is the same logic used by |
Fair enough, nothing here seems particularly concerning so if you are OK with keeping an eye on conda installations once we cut the next release then we don't need to test the conda packages too extensively right now. |
@tommy-waltmann I'll let you finalize and merge this when you're happy. |
Vyas, I added tests to the conda recipe itself: conda-forge/freud-feedstock#48 . When the build platform is the target platform, the conda build will now run all freud's tests with pytest to ensure that the built package functions. |
If I understand #961 correctly, there are some faililng tests that are due to changes introduced on this branch. With that in mind, let's fix them on this branch before merging. The failing tests appear to be floating point precision on the aarch64 architecture. |
The tests are not failing due to changes introduced on this branch. Previous freud-feedstock builds did NOT run aarch64 compatibility in the tests is a different problem than fixing the CMake scripts to generate proper builds, so it should be handled in a different pull request. |
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 looks good! Let's make another PR to fix the floating point issues on aarch64
Awesome, thanks for adding those tests @joaander! I agree that fixing the architecture-specific floating point errors is out of scope for this PR, so disabling those tests on those specific architectures is fine with me. The tests that you added are sufficient to ensure that we're generating functional builds on all platforms, which was my main concern following these changes to the build. |
Description
Make a number of fixes to the CMake scripts.
Motivation and Context
My goal is to provide a working
macos-arm64
build of freud onconda-forge
. The crucial fix is to change from thescikit-build
providedpython_extension_module
to a custom function. This is needed because:libpython3.X.dylib
causes segmentation faults on import:libpython3.X.dylib
is not necessary as the functions it provides are present in the already loadedpython3
image.I also had to change from
MODULE
toSHARED
library builds, as theconda-forge
installation script automatically links modules tolibpython3.X.dylib
on installation.For reasons I don't understand, these changes are not necessary when building from source in a native
macos-arm64
environment. The problems only occur up in cross-compiled builds such as those made byconda-forge
.Additionally, I removed
LD_LIBRARY_PATH
from the TBB search path.LD_LIBRARY_PATH
is a runtime search path. Users that wish to use a specific TBB at compile time should use the provided cmake variablesCMAKE_PREFIX_PATH
orTBBROOT
. In particular, this caused problems with cross-compilation onconda-forge
.How Has This Been Tested?
I built
conda-forge
packages on conda-forge/freud-feedstock#48, downloaded the build artifacts, and testedimport freud
on amacos-arm64
system. Once I had a working import,python -m pytest .
in freud'stests
directory passes.Types of changes
Checklist: