-
Notifications
You must be signed in to change notification settings - Fork 62
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
Cmake doesn't override glfw_FOUND when set manually + README update #84
Conversation
It's my first public pull request so I hope I didn't screw up anywhere. |
Thanks for your PR! Please refer to #83 on what I suggested :D |
Thanks for the suggestion, I'm still not too familiar with Cmake, but I'll try to create the change that way. |
That's alright, if you have any questions just go crazy!
That won't be necessary. You can just put all your changes into this one PR 👌 |
GLFW_FETCH is off by default and controlls whenevere we alwasys install the glfw or not
Okay I've set up a Variable |
With 0fe9f2e I added If you could combine this PR with the setting you proposed that would be great! That way you could first try and search for the library instead of instantly git cloning it the moment it fails. For example for us at Codam, glfw3 is already installed on the Macs yet for some reason it fails to find it even tho it links correctly. With this module it manages to suppress the warning. If not I can also deal with it if you'd like. |
Okay If you want to handle it from now on that's fine by me. But it you can explain how you want me to set it up I'm happy to do it. |
The Find module basically does all the searching for us. The main idea is to fetch as the last possible solution. Because if GLFW is already installed we can use it. So basically:
In order to make the module work that I added all you need to do is add: # Add this at the top or somewhere appropriate
list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake") I was thinking of then using the GLFW_FETCH as an option wether to fetch it at the last step or not. For your sakes then you don't need it to always fetch every-time, just use whats already installed. |
I added the line you wrote + removed a comment that I accidentally left in. |
Okay I see. On my system It can't find it as it's installed with brew, so I didn't see the problem. |
Add the paths used by 42Homebrew to detect the GLFW dependency
Now we first fetch, then try to find and finally clone if we really are unable to find the glfw dependency
I made the necessary changes, could you try and see if this works for you ? I made some tests myself but I just want to double check. Should now also recognize homebrew as a possible installation both normal and the 42 version used from: https://github.com/kube/42homebrew |
It seems to work really well, except that the option The difference between running
1c1
< cmake -DGLFW_FETCH=ON ../ -B ..//build && make -C ..//build
---
> cmake ../ -B ..//build && make -C ..//build So it's not building GLFW even with the option specified. |
If I understand it correctly this option is supposed to make sure that it builds glfw regardless of it's installation. |
Ah I see Ill fix it. Exactly 👍 |
I fixed a little mistake that it set the incorrect variable in the Find module. It should work now I hope. |
I won't go to school tomorrow but I'll check it on Wednesday morning it that works for you. And if you want I would be happy to update the docs as well it you tell me where to put the new option. |
Regarding the docs about this, I think stating under So like: Installation 🏗️Available OptionsYou can pass build options to cmake ... The following options for MLX42 are:
|
Thanks I'll update the docs today |
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.
I think this is ready to be merged 👌
Thank you so much for this PR! It actually fixes all the linkage problems that I've seen and found so far! I really appreciate your contribution :D
You're welcome happy to contribute |
Possible fix for #83 .
Added 2 lines in CMake list + 1 line in README to let people known about the new option.