Closed alexreinking closed 5 months ago
You need add Remove the dummy install rules.
back, you can't pass pytorch PreCI without it. @alexreinking
Dummy install will dirty pytorch output binaries.
@xuhancn -- I don't understand your comment. This PR removes the dummy install rules. Do you mean I should revert 592edaf?
@xuhancn -- I don't understand your comment. This PR removes the dummy install rules. Do you mean I should revert 592edaf?
Install dummy will always install sleef binaries into torch/dummy
directory. It will dirty pytorch output directory, and hit PreCI checkout rules.
I double checked it, your current code seems right. I will vaildate your PR later, and let you known the status. @alexreinking
I double checked it, your current code seems right. I will vaildate your PR later, and let you known the status.
Sounds good! Let me know how it goes.
@alexreinking your PR can pass pytorch Windows build.
Test in pytorch's PR: https://github.com/pytorch/pytorch/pull/122053
@xuhancn - Great! Thank you for testing.
Hopefully @blapie can review soon, but only during normal working hours of course 🙂
@alexreinking your PR can pass pytorch PreCI also.
Brilliant, thanks! It sounds like we set ourselves for some troubles by not keeping cmake tidy. To our defence the project has been hibernating for over 3 years, but we should probably have started with that.
Just left 1 nitty comment, otherwise happy to merge.
Brilliant, thanks! It sounds like we set ourselves for some troubles by not keeping cmake tidy. To our defence the project has been hibernating for over 3 years, but we should probably have started with that.
Just left 1 nitty comment, otherwise happy to merge.
Please merge it ASAP. Thanks.
@xuhancn - It's in! @alexreinking - Thanks that was super helpful!
@blapie - you're welcome! I'm always happy to help open source projects with their build systems 🙂
@xuhancn - It's in! @alexreinking - Thanks that was super helpful!
Cool, I will update pytorch to latest sleef master.
It seems that this have disabled the generation of libsleef.so and libsleefquad.so symlinks making it harder to find/link to the library. Is this intentional?
It seems that this have disabled the generation of libsleef.so and libsleefquad.so symlinks making it harder to find/link to the library. Is this intentional?
It was not intentional. The removal of the dummy targets was done in #510. I shouldn't have assumed this change was thoroughly tested before cherry picking it blindly.
Thank you for your follow up PR fixing it properly for CMake 3.18.
This PR contains three contributions in corresponding commits:
cmake_minimum_required
andproject
to maintain compatibility with future CMake versions.SLEEF_BUILD_SHARED_LIBS
variable which overrides the value ofBUILD_SHARED_LIBS
within SLEEF's project scope when used as a submodule.Parts (2-3) are meant to supersede #510.
ATTN @blapie @xuhancn