-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
add precompiled c/c++ header support to zig build #17956
base: master
Are you sure you want to change the base?
Conversation
ah. there's a problem still. somehow didn't notice before but: I sometimes get this error
with some_include.h contents unchanged but probably touched by git or something. so it needs some adjustment to do either to make llvm less strict, or the zig caching system more strict to accommodate for this. |
There is indeed an option for this: |
A few points on this topic:
|
I don't think this would work.
What may work is to automatically make a pch header for system/libc/libc++ includes and provide that if none are manually specified. And that could be in the global cache probably. |
Interesting. Thanks for that insight. We can consider that idea regarding libc/libc++ in a separate proposal. |
As an other test, I've added pch support to patchdiff --git a/build.zig b/build.zig
index e7d4f8e..3774c80 100644
--- a/build.zig
+++ b/build.zig
@@ -18,7 +18,7 @@ pub fn build(b: *std.Build) void {
lib.linkLibC();
lib.linkLibCpp();
lib.addIncludePath(.{ .path = "src" });
- lib.addConfigHeader(b.addConfigHeader(.{
+ const config_h = b.addConfigHeader(.{
.style = .{ .cmake = .{ .path = "config.h.in" } },
}, .{
.HAVE_ROUND = 1,
@@ -35,7 +35,31 @@ pub fn build(b: *std.Build) void {
.USE_FFTW3F = null,
.USE_VDSP = null,
.USE_KISSFFT = null,
- }));
+ });
+ const c_flags = &.{
+ "-std=c++11",
+ "-fno-rtti",
+ "-fno-exceptions",
+ "-DHAVE_CONFIG_H",
+ "-D_SCL_SECURE_NO_WARNINGS",
+ "-D__STDC_LIMIT_MACROS",
+ "-D__STDC_CONSTANT_MACROS",
+ "-DCHROMAPRINT_NODLL",
+ };
+ lib.addConfigHeader(config_h);
+
+ const pch = b.addPrecompiledCHeader(.{
+ .name = "chromaprint",
+ .target = target,
+ .optimize = optimize,
+ .cpp_header = true,
+ }, .{
+ .file = .{ .path = "src/pch.h" },
+ .flags = c_flags,
+ });
+ pch.addIncludePath(.{ .path = "src" });
+ pch.addConfigHeader(config_h);
+
lib.addCSourceFiles(.{
.files = &.{
"src/audio_processor.cpp",
@@ -57,16 +81,8 @@ pub fn build(b: *std.Build) void {
"src/chromaprint.cpp",
"src/fft_lib_avfft.cpp",
},
- .flags = &.{
- "-std=c++11",
- "-fno-rtti",
- "-fno-exceptions",
- "-DHAVE_CONFIG_H",
- "-D_SCL_SECURE_NO_WARNINGS",
- "-D__STDC_LIMIT_MACROS",
- "-D__STDC_CONSTANT_MACROS",
- "-DCHROMAPRINT_NODLL",
- },
+ .flags = c_flags,
+ .precompiled_header = pch,
});
lib.addCSourceFiles(.{
.files = &.{
diff --git a/src/pch.h b/src/pch.h
new file mode 100644
index 0000000..489a7f3
--- /dev/null
+++ b/src/pch.h
@@ -0,0 +1,23 @@
+#include <stdint.h>
+#include <math.h>
+#include <assert.h>
+#include <stdio.h>
+#include <string.h>
+
+#include <vector>
+#include <limits>
+#include <cmath>
+#include <limits>
+#include <algorithm>
+#include <string>
+#include <cstring>
+#include <memory>
+#include <cassert>
+
+extern "C" {
+#include <libavcodec/avcodec.h>
+#include <libavcodec/avfft.h>
+#include <libavutil/mem.h>
+}
+ rebuilding only libchromaprint (and not the dependencies) I get something like: without
with
|
however there's still something going wrong somewhere. continuing on the libchromaprint example,
the modified file edit: It's much simpler than that.
so it really looks like |
ok. |
6ebb133
to
9c51931
Compare
yeah I was trying to sneak in the feature without add new explicit options, for some reason - probably because precompiled header are specific to c/c++ , not a zig language thing, I tried to modify the change to be more straight forward, that should address all review comments. |
9b2af04
to
b83acd6
Compare
ac05328
to
18f282c
Compare
I'm sorry, I have let this sit too long unreviewed and acquire conflicts. I will fix them for you and attempt to merge this, but I will first give priority to the other PRs that are closer to a mergeable state. |
No trouble: I do use the branch locally and keep it up to date. I just don't push everytime not to waste CI time.. :) |
I tried to hack together an example with https://github.com/allyourcodebase/cpython : You can see it is a bit clumsy to use:
However I do observe a small speedup:
time to rebuild with diff --git a/build.zig b/build.zig
index c7fc831039..288e078b96 100644
--- a/build.zig
+++ b/build.zig
@@ -30,7 +30,7 @@ pub fn build(b: *std.Build) void {
exe.rdynamic = true;
@setEvalBranchQuota(10000);
- exe.addConfigHeader(b.addConfigHeader(.{
+ const config_header = b.addConfigHeader(.{
.style = .{ .autoconf = b.path("pyconfig.h.in") },
.include_path = "pyconfig.h",
}, .{
@@ -628,7 +628,24 @@ pub fn build(b: *std.Build) void {
.socklen_t = null,
.uid_t = null,
.WORDS_BIGENDIAN = null,
- }));
+ });
+ exe.addConfigHeader(config_header);
+
+ const precompiled_header = b.addPrecompiledCHeader(.{
+ .name = "Python",
+ .target = target,
+ .optimize = optimize,
+ }, .{ .file = b.path("Include/Python.h"), .flags = &.{
+ "-fwrapv",
+ "-std=c11",
+ "-fvisibility=hidden",
+ "-DPy_BUILD_CORE",
+ } });
+ precompiled_header.linkLibC(); // setup libc include path
+ precompiled_header.addIncludePath(b.path("Include/internal"));
+ precompiled_header.addIncludePath(b.path("."));
+ precompiled_header.addIncludePath(b.path("Include"));
+ precompiled_header.addConfigHeader(config_header);
exe.addCSourceFiles(.{ .files = &.{
"Modules/_abc.c",
@@ -664,7 +681,6 @@ pub fn build(b: *std.Build) void {
"Modules/getbuildinfo.c",
"Modules/itertoolsmodule.c",
"Modules/main.c",
- "Modules/mathmodule.c",
"Modules/md5module.c",
"Modules/sha1module.c",
"Modules/sha256module.c",
@@ -805,7 +821,19 @@ pub fn build(b: *std.Build) void {
"-std=c11",
"-fvisibility=hidden",
"-DPy_BUILD_CORE",
+ }, .precompiled_header = precompiled_header.getEmittedBin() });
+
+ // compile without precompiled header:
+ // the file includes Python.h with NEEDS_PY_IDENTIFIER defined.
+ exe.addCSourceFiles(.{ .files = &.{
+ "Modules/mathmodule.c",
+ }, .flags = &.{
+ "-fwrapv",
+ "-std=c11",
+ "-fvisibility=hidden",
+ "-DPy_BUILD_CORE",
} });
+
exe.addCSourceFiles(.{ .files = &.{
"Modules/getpath.c",
}, .flags = &.{ |
(merged back from #20879) I included a simpler interface to use precompiled headers: It is now possible to simply add the name of the header file to precompile: exe.addCSourceFiles(.{
.files = &.{"file.c", ...},
.flags = ...,
.precompiled_header = .{ .source_header = .{.path = b.path("all.h")} },
}); In order to do this,
updated example patch for https://github.com/allyourcodebase/cpython: --- a/build.zig
+++ b/build.zig
@@ -664,7 +664,6 @@ pub fn build(b: *std.Build) void {
"Modules/getbuildinfo.c",
"Modules/itertoolsmodule.c",
"Modules/main.c",
- "Modules/mathmodule.c",
"Modules/md5module.c",
"Modules/sha1module.c",
"Modules/sha256module.c",
@@ -805,7 +804,19 @@ pub fn build(b: *std.Build) void {
"-std=c11",
"-fvisibility=hidden",
"-DPy_BUILD_CORE",
- } });
+ }, .precompiled_header = .{ .source_header = .{ .path = b.path("Include/Python.h") } } });
+
+ // files that includes Python.h with NEEDS_PY_IDENTIFIER defined
+ exe.addCSourceFiles(.{ .files = &.{
+ "Modules/mathmodule.c",
+ }, .flags = &.{
+ "-fwrapv",
+ "-std=c11",
+ "-fvisibility=hidden",
+ "-DPy_BUILD_CORE",
+ "-DNEEDS_PY_IDENTIFIER",
+ }, .precompiled_header = .{ .source_header = .{ .path = b.path("Include/Python.h") } } });
+
exe.addCSourceFiles(.{ .files = &.{
"Modules/getpath.c",
}, .flags = &.{ Time to
after:
|
c025765
to
097a177
Compare
@@ -46,11 +46,36 @@ pub const RPath = union(enum) { | |||
special: []const u8, | |||
}; | |||
|
|||
// subset of Compilation.FileExt | |||
pub const AsmSourceLang = enum { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a way to specify defines and/or flags? When using assembly with the C preprocessor, defines are definitely important; I'm not sure if general purpose flags would be of any use though, and defines are only used in assembly with the C preprocessor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so, but I never used addAssemblyFile()
myself so I don't know how it is used by people. And I don't know if there is a reason to have it separate from addCSourceFile()
- which coincidentally also allows for asm files with defines and all.
maybe it should just be removed?
Altough I don't want to include unrelated changes to the pull request, looks like I still felt compelled to add AsmSourceFile
to make the API of addAssemblyFile()
more similar to addCSourceFile()
for some reason, maybe I shouldn't have or I should go all the way. (but that looks like wasted effort if it is to be deleted)
/// "assembler" | ||
assembly, | ||
/// "assembler-with-cpp" | ||
assembly_with_cpp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is already an addAssemblyFile
, is .assembly
and .assembly_with_cpp
necessary in CSourceLang
? The only difference is the ability to provide flags and/or defines (which may be important). We probably don't want two ways to do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, as I said I'm more drawn to removing it. and maybe finding a better name for addCSourceFile
to reflect that it is not "C" source, but "not zig" source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant comment about the name: #20655 (comment)
Zig primarily has built-in first-class support for the C and C-like languages, and I don't believe there is intention to have that kind of support for other languages, so addCSourceFile may be fine.
lib/std/Build/Step/Compile.zig
Outdated
if (prev_has_xflag and link_object != .c_source_file and link_object != .c_source_files and link_object != .assembly_file) { | ||
try zig_args.append("-x"); | ||
try zig_args.append("none"); | ||
prev_has_xflag = false; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? It isn't done for cflags and it seems like a bit of a hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm I don't remember if I was cautious with having a left over "-x option" propagating the rest of the command line, or if there is a actual problem by removing it.
will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/main.zig has some relevant information on how the flag is parsed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I reckon if you mix library/objects files and sources
this line .positional => switch (file_ext orelse Compilation.classifyFileExt(mem.sliceTo(it.only_arg, 0)))
will propage the -x to unrelated files.
I'll try to make a test case.
(btw, thanks for the review, sorry for the delay)
lib/std/Build/Step/Compile.zig
Outdated
if (asm_file.lang) |lang| { | ||
try zig_args.append("-x"); | ||
try zig_args.append(lang.getLangName()); | ||
prev_has_xflag = true; | ||
} else if (prev_has_xflag) { | ||
try zig_args.append("-x"); | ||
try zig_args.append("none"); | ||
prev_has_xflag = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section of code to check the previous -x
flag is duplicated 3 times, and the same goes for the check for -cflags
. It should probably be refactored.
/// "assembler" | ||
assembly, | ||
/// "assembler-with-cpp" | ||
assembly_with_cpp, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant comment about the name: #20655 (comment)
Zig primarily has built-in first-class support for the C and C-like languages, and I don't believe there is intention to have that kind of support for other languages, so addCSourceFile may be fine.
lib/std/Build/Step/Compile.zig
Outdated
@@ -1212,6 +1223,7 @@ fn getZigArgs(compile: *Compile, fuzz: bool) ![][]const u8 { | |||
switch (other.kind) { | |||
.exe => return step.fail("cannot link with an executable build artifact", .{}), | |||
.@"test" => return step.fail("cannot link with a test", .{}), | |||
.pch => @panic("Cannot link with a precompiled header file"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason to use panic instead of step.fail like the two cases above?
@@ -817,17 +824,20 @@ pub fn linkFrameworkWeak(c: *Compile, name: []const u8) void { | |||
|
|||
/// Handy when you have many C/C++ source files and want them all to have the same flags. | |||
pub fn addCSourceFiles(compile: *Compile, options: Module.AddCSourceFilesOptions) void { | |||
assert(compile.kind != .pch); // pch can only be generated from a single C header file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it's a bit odd for compile.addCSourceFiles
(and the similar functions below) to have different behavior than compile.root_module.addCSourceFiles
, especially considering the latter is invalid as well and would probably crash on the assert in finalize. Additionally, it seems most of the functions in Step.Compile
don't make much sense for a precompiled header. It may be worth just making a seperate Step.Compile.Pch
or Step.PrecompileHeader
similar to what Step.TranslateC
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, it seems most of the functions in Step.Compile don't make much sense for a precompiled header.
mmm I don't most . The precompiled header are sensitive to most compiler options, so for instance even "linkXXX" functions may change compile flags and defines that impact the header compilation even though the actual linking doesn't take place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect any linker options to affect a precompiled header since linking would be done by the executable/library Compile step, although you might know better than I. Also, I can't think of much use for the functions below:
installHeader*
addObjCopy
checkObject
setLinkerScript, setVersionScript // only for the linker
forceUndefinedSymbol // for the linker, doesn't really make sense for a pch
isDynamicLibrary/isStaticLibrary/isDll/producesPdbFile/producesImplib // always false
link* // shouldn't change the output, unless pch's fail to compile when the symbols aren't resolved.
addCSourceFile* // can't compile anything more than the pch
addWin32ResourceFile
setVerboseLink // no linking takes place, therefore no output from the linker
getEmitted* // most of these would fail assertions on a pch
addObjectFile/addObject
That's a large amount of the user-facing API of Step.Compile which wouldn't make sense to use in this context, I think it's worth having a separate step so that the API makes it more clear on what affects the compilation of the PCH, what is impossible/doesn't make sense, and what doesn't affect compilation.
…tection. and change `addAssemblyFile()` to use a `AsmSourceFile` option struct as well. (breaking change) Language is inferred from the file extension, however: - it can be ambiguous. for instance, ".h" is often used for c headers or c++ headers. ".s" (instead of ".S") assembly files may still need the c preprocessor. (ziglang#20655) - a singular file may be interpreted with different languages depending on the context. in "single-file libraries", the source.h file can be both a c-header to include, or compiled as a C file (with a #define as toggle) (ziglang#19423)
… options in a single compile step. It tests addAssemblyFile() with options, and the CSourceFile .lang override.
…s in Compile step to de-duplicate cflags tracking.
usage example: `zig build-pch -lc++ -x c++-header test.h` `zig run -lc++ -cflags -include-pch test.pch -- main.cpp` It builds the file.pch with llvm "-fpch-validate-input-files-content", so it includes data for better integration with zig caching system.
adds a new mode to the Compile step. It shares most of the code with previous compile steps, but with extra constraints expressed by assert checks.
…led header file.
It is called after the user has fully configured the steps in the build() function.
a compile step to build the pch file will be automatically created. To benefit from precompiled headers, it is now possible to simply change exe.addCSourceFiles(.{ .files = &.{"file.c"}, .flags = ..., }); into exe.addCSourceFiles(.{ .files = &.{"file.c"}, .flags = ..., .precompiled_header = .{ .source_header = .{ path = b.path("rootincludes.h") } }, });
It gets my c++ project full rebuild time from
to
pch are a little bit different from other compile steps: