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

Draft: Add CMake build system #310

Merged
merged 2 commits into from
May 31, 2024
Merged

Draft: Add CMake build system #310

merged 2 commits into from
May 31, 2024

Conversation

NicolasBFR
Copy link
Contributor

@NicolasBFR NicolasBFR commented Jan 20, 2024

Add CMake build system

New CMakeLists.txt to let users build the libraries using CMake
Build CivetWeb and then use it to build WebUI

@NicolasBFR NicolasBFR marked this pull request as draft January 20, 2024 19:18
@NicolasBFR
Copy link
Contributor Author

For some reason the linking works fine on all Linux compilers but fails on the _mg_websocket_write symbol linking. Anyone have any idea ?

@jinzhongjia
Copy link
Contributor

jinzhongjia commented Jan 21, 2024

Do you mean this problem with cmake ?
I am currently developing under arch linux, and the original makefile and zig builds are fine
And to be honest, I don't like cmake

@AlbertShown
Copy link
Contributor

I don't use cmake either, but if a PR provides a fully working cmake script, then it will be welcomed as for sure many people may prefer cmake over makefile/zig.

On the other hand, webui is simple, one .c file, so cmake may be overkill, in my opinion.

@AlbertShown
Copy link
Contributor

I suggest providing CMakeLists.txt only while keeping the original CI (GitHub Workflow) untouched as it's working fine now.
cmake is like the zig build, is an extra if anyone needs it.

Thank you @NicolasBFR for this PR by the way 👍

@NicolasBFR
Copy link
Contributor Author

I suggest providing CMakeLists.txt only while keeping the original CI (GitHub Workflow) untouched as it's working fine now. cmake is like the zig build, is an extra if anyone needs it.

@AlbertShown

I just wanted to make sure it would technically work for all platforms, unfortunately there is a linker error and I have no Mac or Windows installation to debug it.

CMake is obviously not necessary for a small project like this, but I needed it to import the webui target in a bigger project (AI inference application).

It builds fine on Arch Linux and Ubuntu, should I remove the CI changes, squash history and remove the draft status of the PR ?

@jinzhongjia
Copy link
Contributor

Don't worry, we need to wait @hassandraga for a decision

New CMakeLists.txt to let users build the libraries using CMake
Build CivetWeb and then use it to build WebUI
@eliemichel
Copy link

eliemichel commented Mar 19, 2024

On the other hand, webui is simple, one .c file, so cmake may be overkill, in my opinion.

This is really not transparent from reading the README tbh. If using WebUI is as simple as adding a single .c file in a project, I would suggest this to be advertised in the README instead of only showing an opinionated build process. That would likely refrain many people from thinking "ok, where is the CMakeFile" or whetever other build system they want to use.

@hassandraga
Copy link
Member

New CMakeLists.txt to let users build the libraries using CMake

Thank you @NicolasBFR for the PR. Yes, Adding CMake to WebUI is necessary for projects that use already CMake. This task was already in my todo list, so thank you for starting that.

to be honest, I don't like cmake

Me neither, but I use it daily to avoid being fired. What can we do?!.

webui is simple, one .c file

This is wrong, but it's one header file.

If using WebUI is as simple as adding a single .c file in a project

Readme says One header file, means integrate WebUI in a project is simple as adding one header file, then adding a linking reference to WebUI static library, or dynamic one.

really not transparent from reading the README tbh

Any suggestion is very welcome.

@hassandraga hassandraga merged commit 373c546 into webui-dev:main May 31, 2024
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.

None yet

5 participants