nvim-telescope / telescope-fzf-native.nvim

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

add windows build support #50

Closed Leandros closed 2 years ago

Leandros commented 2 years ago

Added a Visual Studio solution. Confirmed working on Windows 11, with NeoVim Nightly.

Conni2461 commented 2 years ago

I had something else in mind. I am not interested to merge a configuration for a proprietary system specific build system. Especially when a build with gcc (mingw) works on windows. I need to look into how nvim-treesitter is handling windows builds and I honestly prefer a download script with a build in CI.

Edit: I would consider a build system generator like cmake or premake over a visual studio solution. Like how can you integrate it in vim. that it autobuilds on a PlugUpdate or PackerSync. If i push a commit and it requires a rebuild otherwise it the ffi glue code errors and i cant do this with a vs solution than its bad. With a download script i could just say run that script on Update. That script checks if building would be possible (with make and gcc) otherwise it downloads the latest ci build.

Btw we already have a windows job that runs perfectly: https://github.com/nvim-telescope/telescope-fzf-native.nvim/runs/4314879520?check_suite_focus=true

Leandros commented 2 years ago

Edit: I would consider a build system generator like cmake or premake over a visual studio solution. Like how can you integrate it in vim. that it autobuilds on a PlugUpdate or PackerSync. If i push a commit and it requires a rebuild otherwise it the ffi glue code errors and i cant do this with a vs solution than its bad

You can do this with a vs solution using msbuild. That was the idea behind that.

Conni2461 commented 2 years ago

Still requires msbuild. I thought about it some more and I dont want any windows only build files here. For compiler 1 c file.

You can just call the visual c compiler without build system configuration: https://docs.microsoft.com/en-us/cpp/build/reference/compiler-command-line-syntax?view=msvc-170

I just wrote a super quick and dirty downloader script with artifacts: https://github.com/nvim-telescope/telescope-fzf-native.nvim/tree/downloader

I would merge the DLLEXPORT stuff tho. That should be good

Leandros commented 2 years ago

Make sure to also prompt the user to install the required runtime libraries. I'm not sure whether Cygwin requires further runtime dlls, beyond the vcredist.

You can just call the visual c compiler without build system configuration: https://docs.microsoft.com/en-us/cpp/build/reference/compiler-command-line-syntax?view=msvc-170

Well of course you can do that. The issue with that is finding CL.EXE. You'd need something like vswhere, which is just another further dependency. Using VS directly is much more obvious. It's a pretty common pattern to have a Makefile for *nix and a VS solution for Windows (libpng, zlib, etc all do it that way). Make on Windows is, unfortunately, not a great solution in most cases.

Conni2461 commented 2 years ago

Fine. Remove the cpp file, fix formatting and ill merge ...

Note: I absolutely despise this solution. It physically hurts even thinking about it. So please fix the remaining things fast before i change my mind and just drop windows support.

Leandros commented 2 years ago

I'm sorry, that wasn't my intention.

The ideal solution is two-fold:

  1. Have a working Windows build (of which I provided a possible solution).

    Ideally, the Windows build is entirely done natively (i.e without Cygwin or MinGW) since the C code is already fully portable.

  2. Download the resulting binary in the post-setup script of the Vim plugin (of which you have provided the start of a possible solution).

    With the CI already setup, providing a binary via github releases shouldn't be a huge hassle anymore.

You need to be happy with both 1. and 2. and feel comfortable maintaining it. Your idea of having a CMake build is probably the best going forward into the future (unfortunately, my CMake experience is rather limiting). That would also make it much easier to maintain one single build for all platforms.

Leandros commented 2 years ago

I have updated the PR to remove the VS build and add cmake instead.

qgymib commented 2 years ago

Hi, I am working on windows port too and just found this article, it seems more completeness than mine.

After review the cmake script, there might be some improvement can be made:

  1. modern cmake should use target-based rules.
  2. cmake script should cover original Makefile-based functions.

Here is modified cmake script:

# CMakeLists.txt
cmake_minimum_required(VERSION 3.2)
project(fzf)

# add source files
add_library(${PROJECT_NAME}
    SHARED
        "src/fzf.c")

# define target include search paths
target_include_directories(${PROJECT_NAME}
    PUBLIC
        ${CMAKE_CURRENT_SOURCE_DIR}/src)

function(FZF_SETUP_COMPILE_OPTIONS name)
    if (CMAKE_C_COMPILER_ID STREQUAL "MSVC")
        target_compile_options(${name} PRIVATE /W4 /WX)
    else ()
        # Recommand to add -Wall -Wextra, should enable after fix compile error
        #target_compile_options(${name} PRIVATE -Wall -Wextra -Werror -std=gnu99)
        target_compile_options(${name} PRIVATE -std=gnu99)
    endif ()
endfunction()

# add warning check
FZF_SETUP_COMPILE_OPTIONS(${PROJECT_NAME})

# add test
if (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
    include(CTest)
endif()
if (CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME AND BUILD_TESTING)
    add_subdirectory(test)
endif()
# test/CMakeLists.txt
# test
add_executable(fzf_test
    "test.c")
FZF_SETUP_COMPILE_OPTIONS(fzf_test)
target_link_libraries(fzf_test PRIVATE fzf)
add_test(fzf_test $<TARGET_FILE:fzf_test>)

# benchmark
add_executable(fzf_benchmark
    "benchmark.c")
FZF_SETUP_COMPILE_OPTIONS(fzf_benchmark)
target_link_libraries(fzf_benchmark PRIVATE fzf)
add_custom_target(benchmark
    COMMAND $<TARGET_FILE:fzf_benchmark>)

# ntest
add_custom_target(ntest
    COMMAND nvim
            --headless
            --noplugin
            -u test/minrc.vim
            -c "PlenaryBustedDirectory test/ { minimal_init = './test/minrc.vim' }")

# clangdhappy
add_custom_target(clangdhappy
    COMMAND compiledb make)
Conni2461 commented 2 years ago

Thank you very much :)

For me it was enough to execute these steps

mkdir build
cmake -S. -Bbuild -DCMAKE_BUILD_TYPE=Release
cmake --build build

and we still need to check if it works with plug and packer. Can we do: `{ 'do': 'mkdir build && cmake -S. -Bbuild -DCMAKE_BUILD_TYPE=Release && cmake --build build' } or should we write small sh and bat scripts? Also we need to add some docs, most likely in the README. Another thing we could do is maybe add a wiki entry that helps windows users get started.

I dont think the cmake stuff should cover everything in make. Especially clangdhappy because that just calls a tool to generate compile_commands.json, cmake can just do that

Shatur commented 2 years ago

Especially clangdhappy because that just calls a tool to generate compile_commands.json, cmake can just do that

Agree, there is CMAKE_EXPORT_COMPILE_COMMANDS.

But I would migrate fully to CMake to make it more portable across different platforms.

Conni2461 commented 2 years ago

We just merged cmake support: https://github.com/nvim-telescope/telescope-fzf-native.nvim/commit/2330a7eac13f9147d6fe9ce955cb99b6c1a0face

Thanks for the initial work :)