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

BaseTools: ninja build system for faster builds #6530

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

Conversation

xpahos
Copy link
Contributor

@xpahos xpahos commented Dec 12, 2024

Description

The current makefiles are executed sequentially. The VfrCompile build also includes an antlr build that generates code over a long period of time. Ninja allows to parallelise builds of different utilities and build them 2.5 times faster.

  • Breaking change?
    • Breaking change - Does this PR cause a break in build or boot behavior?
    • Examples: Does it add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does this PR have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does this PR include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

[] for i in $(seq 1 500); do make -C BaseTools clean >/dev/null 2>&1; time make -C BaseTools >/dev/null; done

real	0m22.448s
real	0m22.244s
real	0m22.269s
real	0m22.344s
real	0m22.224s
[] for i in $(seq 1 500); do make -C BaseTools clean >/dev/null 2>&1; time make -C BaseTools ninja >/dev/null; done

real	0m9.488s
real	0m9.453s
real	0m9.582s
real	0m9.605s
real	0m9.644s

Integration Instructions

N/A

@bexcran
Copy link
Contributor

bexcran commented Dec 12, 2024

I've been wanting to switch the build system to Ninja for a while, so this is a step in the right direction!
PatchCheck is failing:

Code format is not valid:
 * Trailing whitespace found
   File: BaseTools/Source/C/LzmaCompress/GNUmakefile
   Line: override NINJA_CXXSRCS := 
 * Trailing whitespace found
   File: BaseTools/Source/C/Makefiles/app.makefile
   Line: NINJA_DEPS = 
 * Trailing whitespace found
   File: BaseTools/Source/C/Makefiles/app.makefile
   Line:   NINJA_DEPS +=  || 
 * Trailing whitespace found
   File: BaseTools/Source/C/Makefiles/header.makefile
   Line: NINJA_DIR = 
 * Trailing whitespace found
   File: BaseTools/Source/C/VfrCompile/GNUmakefile
   Line: 	@echo "build $(NINJA_DIR)/VfrUtilityLib.o: cxx_vfr_$(NINJA_ID) $(NINJA_DIR)/VfrUtilityLib.cpp || $(NINJA_DIR)/EfiVfrParser.o" >> build.ninja 

@xpahos xpahos force-pushed the base-tools-ninja branch 2 times, most recently from 4f2c500 to 16be978 Compare December 12, 2024 12:22
@xpahos
Copy link
Contributor Author

xpahos commented Dec 12, 2024

I've been wanting to switch the build system to Ninja for a while, so this is a step in the right direction! PatchCheck is failing:

Code format is not valid:
 * Trailing whitespace found
   File: BaseTools/Source/C/LzmaCompress/GNUmakefile
   Line: override NINJA_CXXSRCS := 
 * Trailing whitespace found
   File: BaseTools/Source/C/Makefiles/app.makefile
   Line: NINJA_DEPS = 
 * Trailing whitespace found
   File: BaseTools/Source/C/Makefiles/app.makefile
   Line:   NINJA_DEPS +=  || 
 * Trailing whitespace found
   File: BaseTools/Source/C/Makefiles/header.makefile
   Line: NINJA_DIR = 
 * Trailing whitespace found
   File: BaseTools/Source/C/VfrCompile/GNUmakefile
   Line: 	@echo "build $(NINJA_DIR)/VfrUtilityLib.o: cxx_vfr_$(NINJA_ID) $(NINJA_DIR)/VfrUtilityLib.cpp || $(NINJA_DIR)/EfiVfrParser.o" >> build.ninja 

I was thinking about bazel/buck2, they are much more flexible than Ninja. But it would be hard to support two build systems. Ninja seems to be good enough to be a meta-generated layer that runs in parallel.

@bexcran
Copy link
Contributor

bexcran commented Dec 12, 2024

I was thinking about bazel/buck2, they are much more flexible than Ninja. But it would be hard to support two build systems. Ninja seems to be good enough to be a meta-generated layer that runs in parallel.

I'm hoping we can switch entirely from GNU Make to Ninja in future.
Ninja seems a good choice because Qemu also uses it, and so it's something I think people are likely to already have installed.

@lgao4
Copy link
Contributor

lgao4 commented Dec 17, 2024

This change is just for BaseTools. Have you evaluated to apply ninja for platform build?

@xpahos
Copy link
Contributor Author

xpahos commented Dec 17, 2024

This change is just for BaseTools. Have you evaluated to apply ninja for platform build?

I can't figure out how to do the dependency graph properly yet. I found a hack to generate compile_commands.json, so far I'm trying to do something with it. The problem is that ninja doesn't have the most convenient dependencies. When building a large graph, the dependencies look like an endless series of objects. I wanted to start with BaseTools as they are more close to the Linux world. It's still difficult with the main project, as it's a project with a lot of history, and in some places I don't really understand why some things are done the way they are. But as a quick option, I'll try to make a similar compile_commands.json.

@lgao4
Copy link
Contributor

lgao4 commented Jan 7, 2025

@YangGangUEFI , have you chance to evaluate ninja build system?

@YangGangUEFI
Copy link
Contributor

@YangGangUEFI , have you chance to evaluate ninja build system?

Hi, I'll take a look at this. (There are probably two main things: 1. build.ninja file for Ninja, 2. integrating with the current EDK2 build process)

BTW: For these changes in BaseTools ninja support, I also see there is another old PR BaseTools: CMake Support. which can also support Ninja in BaseTools. (Don't touch current makefiles, add additional BaseTools/CMakeLists.txt file, but need CMake installed) Do we need evaluate it with current PR?

@xpahos
Copy link
Contributor Author

xpahos commented Jan 7, 2025

BTW: For these changes in BaseTools ninja support, I also see there is another old PR #181 which can also support Ninja in BaseTools. (Don't touch current makefiles, add additional BaseTools/CMakeLists.txt file, but need CMake installed) Do we need evaluate it with current PR?

I think CMake is a more generic way to create BaseTools on different platforms. The current PR can only work on POSIX systems, because I have no experience with Windows as a development platform. And honestly, the Makefile code looks really ugly. The only advantage of using Makefiles directly is synchronisation with current Makefiles, you don't need to separately maintain CMake.
On the other hand, CMake as an additional dependency doesn't look the best. Right now I'm working on generating ninja files for the main source tree (the state is very unstable). I'm generating the ninja files directly, without CMake. It will be a bit weird if BaseTools is generated from CMake, and the main source tree from current python scripts.

@YangGangUEFI
Copy link
Contributor

don't need to separately maintain CMake.
On the other hand, CMake as an additional dependency doesn't look the best.

Agree with this.

I'm generating the ninja files directly, without CMake. It will be a bit weird if BaseTools is generated from CMake, and the main source tree from current python scripts.

On this point, it seemly use Makefiles directly with ninja is more better.
(As that old PR mentioned: "to provide additional flexibility.", if we need it in the future we can also add it independently.)

One more question, is there a way to use the ninja -j parameter(without modifying the Makefile)?

The current makefiles are executed sequentially. The VfrCompile
build also includes an antlr build that generates code over a long
period of time. Ninja allows to parallelise builds of different
utilities and build them 2.5 times faster.

Signed-off-by: Alexander Gryanko <[email protected]>
@xpahos
Copy link
Contributor Author

xpahos commented Jan 9, 2025

One more question, is there a way to use the ninja -j parameter(without modifying the Makefile)?

I added NINJAFLAGS which can only work with Make version 4.2+ for the -j option. In versions before 4.2, the -j flag is not added to MAKEFLAGS. By default, ninja uses the number of cores.

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.

4 participants