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

CMake support #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CMake support #15

wants to merge 1 commit into from

Conversation

kassane
Copy link

@kassane kassane commented Sep 10, 2023

Hi @boazsegev ,

Build library with options to build tests and examples.

Need improvement yet to build all examples and tests.

Msys2/clang64, has patchwork block.
see: https://github.com/kassane/cstl/actions/runs/6139216577/job/16656901085#step:5:75

Copy link
Member

@boazsegev boazsegev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, looks amazing. Thanks!

I know CMake is extremely popular and I am thankful for this contribution.

However, there is also a partial commitment here. I know nothing of CMake. I use it by necessity only but I am much more comfortable with the original GNU Make.

I am not sure I will be able to maintain the CMake support. I hope you will be able to help with this if this breaks in the future. Even if not, I am thankful.

Having said that, please see my comments and see what can be done.

.github/workflows/cmake.yml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cstl.pc.in Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@kassane
Copy link
Author

kassane commented Sep 12, 2023

Wow, looks amazing. Thanks!

I know CMake is extremely popular and I am thankful for this contribution.

Nice!
This change allows using the dependency manager from cmake as shown in the example below.

include(FetchContent)

find_package(facil.io 0.8.0)
if (NOT facil.io_FOUND)
    FetchContent_Declare(facil.io GIT_REPOSITORY https://github.com/facil-io/cstl.git
        GIT_TAG master)
    FetchContent_GetProperties(facil.io)
    FetchContent_MakeAvailable(facil.io)
endif()
    
add_executable(myserver src/main.c)
target_link_libraries(myserver PRIVATE facil.io::facil.io)

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@michaellenaghan
Copy link
Contributor

I left a bunch of drive-by comments. I don't know if that's annoying, or helpful, or perhaps a bit of both. :-)

I see some other potential improvements. If the comments are more helpful than annoying, let me know and I'll take a closer look.

Btw, just to be clear: I'm knowledgeable about CMake, but I'm certainly not an expert.

@kassane
Copy link
Author

kassane commented Sep 19, 2023

I left a bunch of drive-by comments. I don't know if that's annoying, or helpful, or perhaps a bit of both. :-)

I see some other potential improvements. If the comments are more helpful than annoying, let me know and I'll take a closer look.

Btw, just to be clear: I'm knowledgeable about CMake, but I'm certainly not an expert.

No worries!

Also, I'm no expert. But thank you for your suggestions.
I'll look at each one and then refactor my initial proposal.

@kassane kassane marked this pull request as draft September 19, 2023 15:58
@michaellenaghan
Copy link
Contributor

What I'd suggest, if you're interested, is that we talk some things through first. I was leaving comments as I was reading from top to bottom; I have more, and continuing to leave them one by one might really tip the scales from "helpful" to "annoying."

If that makes sense, email might be better, to reduce noise here? But I'm OK either way.

I really am only a day into looking at this, so a question for @boazsegev: my impression, given the compile-time configurability, is that this is really meant to be a compile-time library. Am I off the mark?

Also, what's the correct name for this project — cstlor facil or facil.io? More specifically: if people are linking against something, what should that something be called? (I think facil.io is not a good choice, given the ".". But I guess facilio could be…?)

@kassane
Copy link
Author

kassane commented Sep 19, 2023

For Windows need fix

@boazsegev
Copy link
Member

Hi @michaellenaghan ,

Thanks for the comments and the questions.

I really am only a day into looking at this, so a question for @boazsegev: my impression, given the compile-time configurability, is that this is really meant to be a compile-time library. Am I off the mark?

In general, the CSTL is a header library that has a lot of compile time options and a nice toolset for creating commonly used types and containers...

... but there's also a bunch of sane defaults that allow the use of the compiled FIO_EVERYTHING as a good linked / static library from many use cases.

Also, what's the correct name for this project — cstlor facil or facil.io?

The compiled library is the facil.io framework (I would call it facil in CMake, as the . can easily cause issues).

The facil.io is the website, that's where the v.0.8.x will go.

@kassane
Copy link
Author

kassane commented Sep 27, 2023

Hi @boazsegev ,
Enable build all examples and tests, gets some errors:
https://github.com/kassane/cstl/actions/runs/6331126625 - need refactoring or skip some examples and tests build?

@kassane kassane marked this pull request as ready for review September 28, 2023 14:07
@kassane
Copy link
Author

kassane commented Sep 28, 2023

The Windows issue will need to refactor the code before we can proceed.

@kassane
Copy link
Author

kassane commented Sep 29, 2023

@boazsegev, I have removed Windows tests on CI.
I'll need some more time to fix the Windows issue. If you'd like to evaluate my latest progress, please feel free to do so.

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.

3 participants