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 system usability and security concerns #114

Closed
GiuseppeCesarano opened this issue May 27, 2023 · 25 comments
Closed

Build system usability and security concerns #114

GiuseppeCesarano opened this issue May 27, 2023 · 25 comments

Comments

@GiuseppeCesarano
Copy link
Contributor

The current build system is difficult to use and maintain, it is too decentralized and makes it difficult to apply changes.

Running the ./linux_build script applies many changes to the repository and it is difficult to figure out which ones should be tracked properly; by the way, with the current build system and the fact that library binaries are included in the repository, it is particularly easy for an attacker to push a build with added malware.

I strongly recommend removing previously compiled versions from git tracking because it is unusual and not a safe practice.

As for the build system itself, it should be rewritten to use fewer files and remove duplication, i.e. all clang and gcc files are completely interchangeable except for how the compiler is called.

Redundancy creates problems if, for example, a single flag needs to be changed or added. I experimented with LTO and found that enabling it didn't increase compilation time by much, and the binaries produced were 15% smaller; but setting the flag in each and every file is painful.

Finally, this system is also slow because the files are compiled sequentially.

The solution proposed in #94 is good, but I agree that zig is not that widely used yet, then I don't think there is a good integration for Visual Studio.

A single makefile would be nice but it would not be possible to support windows, I think the only solution would be to use cmake as it is a de facto standard and visual studio has good integration.

@neroist
Copy link
Member

neroist commented May 27, 2023

I agree!

@hassandraga
Copy link
Member

Understandable. Honesty, WebUI has changed a lot since the first version v1.0 in November 2020.

ASIO to C
WebUI was completely written in C++ and used CMake as the build system. Later we find that people are struggling to install Boost ASIO libraries to be able to build WebUI v1.0. So, we decided to rewrite WebUI entirely in C.

CMake to Makefile
Later, I thought it would be fun to build WebUI easily as git clone and then make. That's it. Instead of installing CMake, explain to the user how to add CMake arguments to select their preferred compiler and other settings. But I'm open to bright back CMake or any other build system as an additional option.

Adding pre-built binaries
For a while, the Deno and Python wrappers have existed in the same main repo, so we find that Python and Deno's developers are struggling to build WebUI from C to test it... was painful, so we added the pre-built binaries to let those developers be able to test WebUI easily. Now, as we moved those wrappers to separate repos, I guess we should remove all the pre-built binaries, except the .zip as it is used in the website and GitHub releases.

@GiuseppeCesarano
Copy link
Contributor Author

The fact is, if you do any kind of C or C++ development, you already have cmake; make files are really neat, but as of now, the build system gets in the way of development more than it solves problems.

For ease of use, we could just leave the cmake defaults as the best for users, and then have some options for developers.

For the binaries, they should live in the release section, with a script to get them if you really want to automate the process, but having them in git really opens it up to malicious intentions.

As a proof of concept, I made a very opinionated branch in my fork, the intent being to show how clean the repo could be without significant sacrifices in usability.

https://github.com/GiuseppeCesarano/webui/tree/build_system

@hassandraga
Copy link
Member

I already tested making CMake the only build script, and many people skip trying the WebUI because of it. I strongly recommend adding CMake and more build systems in the future, but we will keep the simple legacy makefiles. Let's make WebUI very flexible for different needs.

About the pre-built binaries, you are right. We can altogether remove them. We keep the existing script so anyone can generate the same pre-built binaries format (Organized and compressed) to add them to the GitHub releases section.

Thank you for the proposed branch, and yes, you can implement the changes to the main repo after updating the branch to apply those points we just discussed:

  1. Adding CMake as an additional build option and keeping the existing old makefiles
  2. Removing all the pre-built binaries and keeping the existing generator scripts

@hassandraga
Copy link
Member

I which if GitHub had a vote option!

@neroist, @AlbertShown, @malisipi

@malisipi
Copy link
Collaborator

I prefer basic Makefiles instead of cmake personally. However adding cmake support will be good for persons who uses it. Also i support removing the prebuilt libraries. So i vote for both.

@GiuseppeCesarano
Copy link
Contributor Author

I have a lot of sympathy for people's frustration with cmake; the reason for this issue is not that I want cmake to be my only build system, so I'm all for keeping the makefiles; but in their current state, the makefiles are too many, redundant, and hard to maintain.

How about replacing them with a single makefile?

For example, the clang and gcc makefiles could be equal, defaulting to gcc (or clang) if the $CC flag isn't set, and using the $CC flag if it is.

The call to the makefile with the non-default compiler would be something like CC=clang; make.

@AlbertShown
Copy link
Contributor

How about replacing them with a single makefile?

I agree, but it's hard to do. Multi-compilers in the makefile are possible, but multi-os support is a nightmare.
I love to, but how can we create one makefile that does all this?

IF Windows --> IF $CC not set --> use MSVC
ELSE IF Windows --> IF $CC == GCC --> use MinGW
ELSE IF Windows --> IF $CC == TCC --> use TCC

ELSE IF Linux --> IF $CC not set --> use GCC
ELSE IF Linux --> IF $CC == CLANG --> use Clang
ELSE IF Linux --> IF $CC == TCC --> use TCC (In future)

ELSE IF macOS --> IF $CC not set --> use Clang
ELSE IF macOS --> IF $CC == GCC --> use GCC (In future)

I googled and tested it long time ago for other projects, but not all scripts work in all OS.

@GiuseppeCesarano
Copy link
Contributor Author

GiuseppeCesarano commented May 29, 2023

Should be fairly easy.

This is how it would work on Linux.

makefile

CC ?= gcc

.PHONY: all

all:
	@echo "invoking $(CC)"

The CC variable should already default to the default compile command, in my case cc (gcc), if for some reason the variable isn't populated it will be populated with gcc;

On my system calling make will print:
invoking cc

Calling make CC=clang will print:
invoking clang

Now the only part missing is OS detection.

@GiuseppeCesarano
Copy link
Contributor Author

https://stackoverflow.com/questions/714100/os-detecting-makefile; doesn't look bad. The CC variable should also work on windows, but of course the default initialization should be something reasonable for windows (if for some reason it's not automatically filled), the compiler flags should also be filled accordingly.

@AlbertShown
Copy link
Contributor

AlbertShown commented May 31, 2023

AI:

There's an important consideration: Makefile doesn't natively support the detection of the operating system.
We need a way to determine the OS. One common practice is to use a shell command, such as uname, and use the output to determine the operating system. This approach has a limitation, though, as it won't work natively on Windows unless a Unix-like environment (like Cygwin or WSL) is used.

ifeq ($(OS),Windows_NT)
    CC ?= MSVC
    ifeq ($(CC), GCC)
        CC = MinGW
    endif
    ifeq ($(CC), TCC)
        CC = TCC
    endif
else
    UNAME_S := $(shell uname -s)
    ifeq ($(UNAME_S),Linux)
        CC ?= GCC
        ifeq ($(CC), CLANG)
            CC = Clang
        endif
        ifeq ($(CC), TCC)
            CC = TCC
        endif
    endif
    ifeq ($(UNAME_S),Darwin)
        CC ?= Clang
        ifeq ($(CC), GCC)
            CC = GCC
        endif
    endif
endif

all:
	@echo $(CC)

@AlbertShown
Copy link
Contributor

Even if this Makefile works, it does not guarantee it always works!

@GiuseppeCesarano
Copy link
Contributor Author

ifeq ($(CC), TCC)
    CC = TCC
endif

This pattern is redundant because it assigns the value TCC to a variable that already contains the value TCC.

The makefile will be something more like:

ifeq ($(OS),Windows_NT)
    CC ?= MSVC
    CFLAGS ?= default flags for msvc
    # windows copy; zipping and other commands should be defined
else
    UNAME_S := $(shell uname -s)
    ifeq ($(UNAME_S),Linux)
        CC ?= gcc
    endif
    ifeq ($(UNAME_S),Darwin)
        CC ?= clang
    endif
   CFLAGS ?= default flags for gcc/clang
    # unix copy; zipping and other commands should be defined
endif

all:
	@echo $(CC)

Even if this Makefile works, it does not guarantee it always works!

Under what circumstances would this not work?

By the way, as soon as I get some free time, I will implement the build system starting with the make version.

@AlbertShown
Copy link
Contributor

Under what circumstances would this not work?

If uname -s gives you linux instead of Linux :)

@AlbertShown
Copy link
Contributor

as soon as I get some free time, I will implement the build system starting with the make version.

Thank you in advance 👍

@AlbertShown
Copy link
Contributor

By the way, nmake users (Microsoft Visual Studio) won't like the idea of installing MinGW to use mingw32-make to run MSVC!
They prefer simply running nmake!

@GiuseppeCesarano
Copy link
Contributor Author

If uname -s gives you linux instead of Linux :)

I was already thinking to remove the Linux control and treat it as default case if the OS is not windows nor darwin. In that way it will also work for users on BDS systems since they would default to CC ?= gcc and it's cflags.

By the way, nmake users (Microsoft Visual Studio) won't like the idea of installing MinGW to use mingw32-make to run MSVC!
They prefer simply running nmake!

I'm not familiar with MSVC's flags and utilities, so I'll do some research on the differences.

@AlbertShown
Copy link
Contributor

remove the Linux control and treat it as default

Great idea, that should work 👍

I'm not familiar with MSVC's flags

Well, here is the first shock, MSVC uses nmake, not make, and it can not read makefile script format while it uses the same file name, which complicates things!

@GiuseppeCesarano
Copy link
Contributor Author

Should we also support msvc in gnumake? Or is nmake enough? It looks like the current system allows the user to call minigw-make with the msvc compiler.

In that case, doing it in a single file won't be pretty.

@AlbertShown
Copy link
Contributor

I don't think we need it, because the MSVC users should use nmake, while GCC/Clang users should use make.

@AlbertShown
Copy link
Contributor

I mean, we don't need to support msvc in gnu-make.

@GiuseppeCesarano
Copy link
Contributor Author

GiuseppeCesarano commented Jun 10, 2023

Just pushed the first fully functional makefile that supports Windows minigw, Linux clang and gcc, MacOS. Next on the list is Windows nmake.
For now, I would like feedback on the GNUmake file, whether the results it produces are good or if they need to be changed.

  • make -j8 will build the static version of webui and the shared version.
  • make -j8 examples will build all examples.
  • make -j8 CC=clang CXX=clang will use the clang toolchain.

The built files will be in the build folder

The changes are in the new_build_system branch

@hassandraga
Copy link
Member

Thank you @GiuseppeCesarano, for the update. That's great.

mingw32-make

mkdir -p build
process_begin: CreateProcess(NULL, mkdir -p build, ...) failed.
make (e=2): The system cannot find the file specified.
mingw32-make: *** [GNUmakefile:52: folder] Error 2

This creates a new folder named -p. We must use double quotes like I did in the old makefile scripts.
Also, we can probably use GNUmakefile for GCC/Clang, And makefile for MSVC.

@GiuseppeCesarano
Copy link
Contributor Author

Also, we can probably use GNUmakefile for GCC/Clang, And makefile for MSVC.

That was the plan, should work without problems; I will probably install a Windows WM and test it!

This creates a new folder named -p. We must use double quotes like I did in the old makefile scripts.

I don't really understand what you mean, I've seen that the paths are enclosed in " " on the older makefiles and i can do that in the new file too, but will mkdir -p "build" just work?

@hassandraga
Copy link
Member

Working on #155

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

No branches or pull requests

5 participants