OlivierLDff / asio.cmake

Small CMake wrapper to add asio with a simple FetchContent.
MIT License
17 stars 8 forks source link

feat: relax the minimum CMake version requirement #15

Closed windowsair closed 5 months ago

windowsair commented 10 months ago

Using a newer version like cmake 3.21 (2022-06) is too aggressive. Let's downgrade cmake to the original 3.11 version.

OlivierLDff commented 10 months ago

Thing is, FetchContent_Populate has been introduced in CMake 3.14. CPM is requiring cmake 3.14 too. So I guess we can go there. I don't think 3.21 is too new anyway, it's fairly easy to upgrade cmake and shouldn't break anything. What is your use case and why you need to downgrade?

OlivierLDff commented 10 months ago

Can you add in the CI a matrix to test CMake 3.14, latest (I don't think 3.11 will work), I recommend using https://github.com/lukka/get-cmake

windowsair commented 10 months ago

As easy as it is to upgrade, using a lower version of cmake in order to provide an out of the box experience is also important.

As an example, Ubuntu 20.04 provides a cmake of 3.16, while Visual Studio 2019 provides cmake as 3.20.

Maybe setting the minimum version to 3.14 is a good idea?

windowsair commented 10 months ago

Can you add in the CI a matrix to test CMake 3.14, latest (I don't think 3.11 will work), I recommend using https://github.com/lukka/get-cmake

Sure. Or we can just use the action app for that: https://github.com/marketplace/actions/actions-setup-cmake

Do I need to add it to this PR?

OlivierLDff commented 10 months ago

Yes we want the PR to pass the tests, so you can add everything here.

I think I prefer https://github.com/marketplace/actions/get-cmake (for sentimental reason, not very technical):

Again I don't know what are the difference with the other project exactly.

windowsair commented 10 months ago

Tested on CMake 3.14, 3.24, latestrc. Everything looks great except for a bunch of CI matrix.

      matrix:
        use_cpm: ['ON', 'OFF']
        build_type: ['Release']
        os: [ubuntu-latest, macos-latest, windows-latest]
        cmake_version: [3.14, 3.24, latestrc]

Do we need such kind of CI update?

windowsair commented 10 months ago

Okay, let's start by modifying the minimal version.

OlivierLDff commented 10 months ago

Yes we want to enforce in the CI that everything works. I guess CMake version can be tested only on ubuntu-latest, using include statement (https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategymatrixinclude)

windowsair commented 10 months ago

I found that cmake 3.14 on Windows needs to specify the PATH address manually, otherwise cmake can't find cl.exe.

If you only care about Ubuntu, I will upload a new modification

OlivierLDff commented 10 months ago

Yes because we want to test if the cmake code is working with 3.14, windows not finding cl.exe sound like a bug from an old version and we don't really care about testing that.

windowsair commented 10 months ago

It makes sense. The commit now is a possible implementation. CI looks like working fine. image

OlivierLDff commented 5 months ago

Sorry for taking so much time to merge, you never requested a review again so I lost track of the PR status. Thanks for the contribution