nvim-telescope / telescope-fzf-native.nvim

FZF sorter for telescope written in c
1.36k stars 45 forks source link

🔨 Cleanup CMake to remove warnings + fix windows builds + add presets #78

Closed bruxisma closed 1 year ago

bruxisma commented 2 years ago

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 #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:
    $ 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.

Closes #77

Conni2461 commented 1 year ago

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.

bruxisma commented 1 year ago

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 -S was added in 3.13, and folks had to use an undocumented -H flag prior to that, which was later removed in later versions, and only worked if there was no space between -H and the source path (whereas -S allows spaces).

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.

Conni2461 commented 1 year ago

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 :)

bruxisma commented 1 year ago

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 :pensive:

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 :slightly_smiling_face:) and I would be the first to know if it broke once I run a :PackerUpdate command.

Conni2461 commented 1 year ago

modern CMake projects

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)

bruxisma commented 1 year ago

Understandable. I appreciate the merge. If anyone runs into stuff re: windows + CMake, feel free to tag me 🙂

Conni2461 commented 1 year ago

Me having mixed feeling about presets doesnt change the fact that i really appreciate the refactor and the fixes. So thank you very much :)