Skip to content
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

Add SLEEF_BUILD_SHARED_LIBS to override BUILD_SHARED_LIBS from including projects #531

Merged
merged 4 commits into from
Mar 18, 2024

Conversation

alexreinking
Copy link
Contributor

This PR contains three contributions in corresponding commits:

  1. Correct the usage of cmake_minimum_required and project to maintain compatibility with future CMake versions.
  2. Add a new SLEEF_BUILD_SHARED_LIBS variable which overrides the value of BUILD_SHARED_LIBS within SLEEF's project scope when used as a submodule.
  3. Remove the dummy install rules.

Parts (2-3) are meant to supersede #510.

ATTN @blapie @xuhancn

Also uses the standard version variables. Replaced PATCHLEVEL by
PATCH except in #define'd macros (to preserve API).

Replaced languages list with enable_language.

Removed policy options that do not affect runtime (because they
are enabled by the minimum version, anyway)
@xuhancn
Copy link
Contributor

xuhancn commented Mar 17, 2024

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.

@alexreinking
Copy link
Contributor Author

@xuhancn -- I don't understand your comment. This PR removes the dummy install rules. Do you mean I should revert 592edaf?

@xuhancn
Copy link
Contributor

xuhancn commented Mar 17, 2024

@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

@alexreinking
Copy link
Contributor Author

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.

@xuhancn
Copy link
Contributor

xuhancn commented Mar 17, 2024

image
@alexreinking your PR can pass pytorch Windows build.
Test in pytorch's PR: pytorch/pytorch#122053

@alexreinking
Copy link
Contributor Author

@xuhancn - Great! Thank you for testing.

Hopefully @blapie can review soon, but only during normal working hours of course 🙂

@xuhancn
Copy link
Contributor

xuhancn commented Mar 18, 2024

@alexreinking your PR can pass pytorch PreCI also.

@xuhancn xuhancn mentioned this pull request Mar 18, 2024
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@blapie blapie left a comment

Choose a reason for hiding this comment

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

Thanks, really happy with this solution. Just left 1 nitty comment.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@blapie
Copy link
Collaborator

blapie commented Mar 18, 2024

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.

@xuhancn
Copy link
Contributor

xuhancn commented Mar 18, 2024

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.

@blapie blapie merged commit 29391cc into shibatch:master Mar 18, 2024
31 checks passed
@blapie
Copy link
Collaborator

blapie commented Mar 18, 2024

@xuhancn - It's in!
@alexreinking - Thanks that was super helpful!

@alexreinking
Copy link
Contributor Author

@blapie - you're welcome! I'm always happy to help open source projects with their build systems 🙂

@xuhancn
Copy link
Contributor

xuhancn commented Mar 18, 2024

@xuhancn - It's in! @alexreinking - Thanks that was super helpful!

Cool, I will update pytorch to latest sleef master.

@yuyichao
Copy link
Contributor

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?

@alexreinking
Copy link
Contributor Author

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.

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.

4 participants