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

build: use a single Makefile #155

Merged
merged 7 commits into from
Aug 21, 2023

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented Aug 9, 2023

This PR is currently WIP.

Finished it...

  • is a major step towards finishing ci: add release step #152.
  • simplifies the build process by using a single Makefile in the projects root that works dynamically based on the platform and also allows to specify a compiler via e.g. COMPILER=gcc.
  • allows to specify an ARCH_TARGET for mac, which I can use during github CI runs to add ARCH_TARGET=arm64-apple-darwin to cross-compile for m1 and add it to the releases
  • allows to remove the build/ directory with all its files

Either here - as it is related - or in a separate PR: I will also remove the need to hold static libray files in the example directories and rather update their Makefiles to handle the static files.

@ttytm ttytm mentioned this pull request Aug 10, 2023
@AlbertShown AlbertShown marked this pull request as ready for review August 10, 2023 17:08
@AlbertShown
Copy link
Contributor

@hassandraga Please invite @ttytm. Thanks.

@ttytm
Copy link
Member Author

ttytm commented Aug 10, 2023

Thanks for the work on the repo @AlbertShown! I got an invitation a few weeks ago and can merge things. But I feel a bit safer submitting such changes as PR so more then just two eyes can check the changes to prevent potential oversights. I would merge things myself when it has the approval of one other maintainer or small obvious things without review.

The PR is not finished, please don't merge yet.

@AlbertShown
Copy link
Contributor

👍

@ttytm ttytm force-pushed the build-single-makefile-in-root branch from 7deb6f3 to 9b28a35 Compare August 14, 2023 22:22
@ttytm ttytm marked this pull request as draft August 14, 2023 22:23
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ttytm
Copy link
Member Author

ttytm commented Aug 17, 2023

It's babysteps on the PR due to the time constraints but we'll get there soon 👍

@ttytm ttytm force-pushed the build-single-makefile-in-root branch 6 times, most recently from 907f86a to 0e9f79d Compare August 17, 2023 22:10
@hassandraga
Copy link
Member

Thank you @ttytm for this, yes, one makefile will be very appreciated. And This is the requirement:

WebUI Build

  • One makefile can deal with all compilers, MSVC, GCC, Clang, TCC
  • Should successfully build the WebUI from source webui/src, and put the output in webui/build
  • Should not deal with WebUI-Bridge at all to keep the possibility of compiling WebUI by using a simple C99 compiler

WebUI-Bridge Build

  • Should be completely separated from WebUI Build
  • Should be able to successfully build WebUI Bridge in webui/bridge and put the output in the same folder

@hassandraga
Copy link
Member

I suggest to modify/new PR as this:

  • Keep the one Makefile in the root
  • Update Readme to match the readme in webui/bridge
  • Move build.sh and build.ps1 into webui/bridge
  • If the one Makefile works, then we can celebrate and remove the entire folder webui/build

@ttytm
Copy link
Member Author

ttytm commented Aug 20, 2023

Thanks for throwing out all the work in the lasy days @hassandraga, it's motivating. The goal is to finish this PR here today as well.

@hassandraga
Copy link
Member

If you have any file conflicts because of the last commits, just do a fresh clone in a new folder, then add your makefile in webui/makefile. Then add webui/bridge/build.sh, and webui/bridge/build.ps1. Then push.

@ttytm
Copy link
Member Author

ttytm commented Aug 20, 2023

Thanks, yes I'll rebase onto the lastest main state and squash related commits to bring them in a comprehensible structure before marking it as ready. My visit just left, I'm on this now 🤓👍.

@ttytm ttytm force-pushed the build-single-makefile-in-root branch from a96e0e1 to 6987c6e Compare August 21, 2023 00:13
@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

This might be an engineers disease to underestimate the work ever so often - especially when it comes to cross platform compatibility with different compilers. Or maybe it's just me.

My goal for today was not fully met, but I got very close.

  • Linux and Macos works.
  • For windows GCC and TCC works. I will give MSVC a polish after getting some sleep.
  • I removed the build section update in the Readme for now as it was outdated for the latest changes and requirements.
  • I removed the bridge build scripts from this PR and submit them separately (Add bridge build scripts #164) as this is a change that can stand on it's own already.
  • The examples were updated to work with the new build output paths. Rebuilding the library was removed for now. Once this Makefile - for the library itself - is finished it will be easier to update the examples to also use a single Makefile based on the finished file. Then rebuilding the library can be re-added to the examples Makefile.

To complete this PR I'm aiming to finish MSVC compilation and the Readme tomorrow, I think this is realistic this time. Other changes like a single Makefile for the examples can still be done separately, they can work with multiple until then.

@hassandraga
Copy link
Member

Thank you @ttytm for supporting Linux, macOS, and Windows.
#164 is good to merge.
Yes, After this one is done, we'll be able to deal with examples later.

@ttytm ttytm force-pushed the build-single-makefile-in-root branch 3 times, most recently from 96d99f1 to d65b95e Compare August 21, 2023 15:00
@hassandraga
Copy link
Member

  • About the examples, I guess we need to remove all OS folder, so examples/C/call_c_from_js/Linux/Clang/Makefile, will become examples/C/call_c_from_js/Makefile as an example.
  • About the binaries output, I suggest to completely remove the build folder from the project, and only use dist folder. So example's makefile will use ../../../../../dist instead of build.

If you want, you can focus on the root makefile only, because it's the most important, when it's done, I will copy it and I will update all examples to use it.

@hassandraga
Copy link
Member

I'm fine with keeping build, but then we should remove dist. We should not have two folders both having compiled binaries.

@AlbertShown
Copy link
Contributor

In my opinion build folder means bins, logs, tests, scripts... while dist means only the final clean output ready for production. So if the makefile produces clean final lib, we should use dist. Otherwise, build.

@hassandraga
Copy link
Member

We should also add more sub-folders like x86-64, x86-32, ARM64, ARM32. I will do it after merging this PR.

@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

What is the use case for the subfolders if I may ask?

@ttytm ttytm force-pushed the build-single-makefile-in-root branch from d60f6b8 to f83727f Compare August 21, 2023 16:33
@hassandraga
Copy link
Member

To build for deferent platforms, so we can solve issues like this webui-dev/deno-webui#9.
I'm planning to up the makefile later after we merge this PR.

@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

Yes thought that it is to support differnt platforms, but I don't see yet why different directories are needed for this.

The output of the new makefile would always land in build or dist. The architecture would depend on the system it was build on. I added the possibility to cross-compile on mac for mac m1 arm. So the output can be used in the CI and for releases.

Wouldn't adding different directories be the same as it is for the compilers now with GCC CLANG MSVC TCC directories? One intention with the single makefile was to remove the need for this.

@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

I upated the build instructions to match the Readme in the bridge dir.

To check it github rendered: https://github.com/ttytm/webui/tree/build-single-makefile-in-root#build
As an alternative I would suggest: https://github.com/ttytm/webui/tree/build-readme-alt#build

Please let me know what you prefer.

To finish the PR I update the build output directory to dist than the PR should be finished both names are okay for me.

@ttytm ttytm marked this pull request as ready for review August 21, 2023 16:44
.github/workflows/build.yml Outdated Show resolved Hide resolved
@hassandraga
Copy link
Member

Yes, the readme section will stay as you suggested.
but we should add the target platform in the lib file name, for example webui-2-x64.dll, webui-2-i386.dll, webui-2-arm64.dll, webui-2-arm32.dll... the subfolders in the dist it's just to make things more organized.

@hassandraga
Copy link
Member

I will create a Wiki page later to explain everything. This PR right now is looking good to me.

@ttytm ttytm force-pushed the build-single-makefile-in-root branch from f83727f to f71f235 Compare August 21, 2023 17:00
@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

Thanks for the review! Readme was updated to the suggestion and out dirs to dist.
I'll merge it when it is green.

One thing that is left todo I see is that the msvc lib is not added to the artifacts:
https://github.com/webui-dev/webui/actions/runs/5929013692?pr=155
It only has the include dir.

@ttytm ttytm merged commit 900d967 into webui-dev:main Aug 21, 2023
8 checks passed
@hassandraga
Copy link
Member

First of all, Thank you very much for finishing this new build system, I appreciate it @ttytm 👍

One thing that is left todo I see is that the msvc lib is not added to the artifacts

Yes, windows-msvc-x64 has only include, and windows-gcc-x64 has non-needed *.o files. We will fix this separately.

@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

Thanks for your work as well @hassandraga.

The *.o is removed when the Windows make is run and won't remain as residue re-testing the build locally. Probably a Windows CI pediatric disease that we can hopefully easily solve as things progress.

@AlbertShown
Copy link
Contributor

Thank you @ttytm, finaly one makefile is here 🐱‍👤
but I can't build on Windows using GCC. Any reason why you removed -DMUST_IMPLEMENT_CLOCK_GETTIME ?
I fix it after this that flag to CIVETWEB_DEFINE_FLAGS.

@ttytm
Copy link
Member Author

ttytm commented Aug 21, 2023

I kept it for a while but removed it after testing it from several angles. I must have missed the need for this additional flag for Windows GCC. #167 Would re-add it for Windows GCC. Sorry about the inconvenience.

GCC is building in CI. Can you share what is failing on your machine?

Edit: Please also tell me about or share a reference to the CLOCK_GETTIME flag.

@ttytm ttytm deleted the build-single-makefile-in-root branch August 26, 2023 19:19
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

3 participants