-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
🔨 Cleanup CMake to remove warnings + fix windows builds + add presets #78
Conversation
This change brings in several things 1. The `CMakeLists.txt` file has been reorganized and brought up to more "modern" CMake code (while still working with CMake 3.2 at a minimum) 2. A fix has been added for nvim-telescope#77, to always set the suffix to `.dll`, regardless of toolchain 3. Warnings for _CRT_SECURE and _CRT_NONSTDC are now disabled. 4. This also adds the most minimal version of a cmake "preset" for CMake 3.19 and later. This adds generators for unix makefiles and ninja, allowing users to simply execute their post-install step as: ```console $ cmake --preset <ninja|make> $ cmake --build build --target install ``` Sadly, this couldn't be simplified further unless a hard minimum of CMake 3.20 is set, as that would allow for an install build preset, allowing the steps to turn into `cmake --preset <preset> && cmake --build --preset <preset>`. I've not changed the readme as I'm probably the only one who will end up using the preset approach anyhow.
Sorry for the late response. Appearantly i missed this PR :) I happy with merging but without Presets, I don't see the benefit and, especially because the minimum CMake version doesn't even support it. |
Presets are there for folks who use newer versions of CMake, so it's just there as an option. Not trying to be a pedant, but technically speaking, the directions you have for configuring don't work on the minimum version of CMake either, as I'm fine with removing them but do want to point out that them being present won't affect anyone who can't use them and are just another option for folks to configure. |
Thats fair. Although us suggesting this command in the README doesnt add an additional file. I am currently evaluating who actually benefits from it. Its not like we compile a hugely complex project. Its just a single c file. Thats also why i originally only went with a makefile, keep it simple and stupid, and it works for most usecases. Sadly it doesn't work great for windows, which is currently the only purpose for the cmake file. Thats why i am evaluating who is actually benefiting from that additional file (one person, being you, doesnt cut it for me). For me right now its just something additional, that will never be used from me (and ci) that potentially needs maintenance. Thats why i am not thrilled. Hope that makes sense :) |
Sorry for the delay in a response, the work week and weekend ended up being very busy for me, so I didn't have time to respond properly 😔 I'm willing to remove the README instructions for presets, I simply recommended it because it's the standard approach for modern CMake projects (e.g, @kylo252 is working on adding presets to neovim, which is commendable). I don't personally think the presets will require maintenance unless you move to a higher CMake minimum where suddenly a build preset or test preset might be needed but even then you're free to ping me if you want me to maintain it full time (My job requires me to use Windows as my daily driver so this plugin gets a lot of daily usage, and I use windows on a fairly regular basis in my personal work too 🙂) and I would be the first to know if it broke once I run a |
The thing is i dont want this to be a modern cmake project. So i dont see a need for adding test, ... I continue to do this with good old Makefiles. This project is almost complete anyway. (couple of bugs and case support for utf8 charset) |
Understandable. I appreciate the merge. If anyone runs into stuff re: windows + CMake, feel free to tag me 🙂 |
Me having mixed feeling about presets doesnt change the fact that i really appreciate the refactor and the fixes. So thank you very much :) |
This change brings in several things
CMakeLists.txt
file has been reorganized and brought up to more "modern" CMake code (while still working with CMake 3.2 at a minimum).dll
, regardless of toolchainSadly, this couldn't be simplified further unless a hard minimum of CMake 3.20 is set, as that would allow for an install build preset, allowing the steps to turn into
cmake --preset <preset> && cmake --build --preset <preset>
. I've not changed the readme as I'm probably the only one who will end up using the preset approach anyhow.Closes #77