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

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

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

bruxisma
Copy link
Contributor

@bruxisma bruxisma commented Jul 16, 2022

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 Building FZF on windows with clang and MSVC toolchain generates libfzf.so #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

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.
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@Conni2461
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

bruxisma commented Sep 5, 2022

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 :PackerUpdate command.

@Conni2461 Conni2461 merged commit 65c0ee3 into nvim-telescope:main Sep 6, 2022
@bruxisma bruxisma deleted the bruxisma/fixup-cmake branch September 6, 2022 05:41
@Conni2461
Copy link
Member

Conni2461 commented Sep 6, 2022

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
Copy link
Contributor Author

bruxisma commented Sep 6, 2022

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

@Conni2461
Copy link
Member

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

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.

Building FZF on windows with clang and MSVC toolchain generates libfzf.so
3 participants