diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9454261..0633fd8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -201,9 +201,24 @@ jobs: # every rule returns before looking for a payload and nothing is # downloaded; what it asserts is that all six modules compile here. - name: every rule module compiles for this host - working-directory: tests/all-rules-compile run: | set -e + # THE FIXTURE'S OWN DENOMINATOR. It asserts "every rule compiles + # here", and "every" is a list it carries -- so a seventh member + # added to this package would be covered by a step whose name says it + # already is. The list is therefore compared against the package's + # own `[features]` before the build, and the two must be equal. + feats=$(sed -n '/^\[features\]/,/^# /p' mcpp.toml \ + | grep -oE '^[a-z-]+ +=' | sed 's/ *=//' | grep -v '^default$' | sort) + used=$(sed -n '/features = \[/,/\], host-module/p' tests/all-rules-compile/mcpp.toml \ + | grep -oE '"[a-z-]+"' | tr -d '"' | sort) + [ -n "$feats" ] || { echo "FAIL: read no features out of mcpp.toml"; exit 1; } + [ "$feats" = "$used" ] || { + echo "FAIL: the fixture does not name every published feature" + echo " package: $(echo $feats)" + echo " fixture: $(echo $used)" + exit 1; } + cd tests/all-rules-compile "$MCPP" build "$MCPP" run | tee run.log grep -q '^all-rules-compile ok' run.log @@ -470,9 +485,24 @@ jobs: # `no member named 'popen' in the global namespace`, reported against a # consumer that had nothing to do with it. - name: every rule module compiles for this host - working-directory: tests/all-rules-compile run: | set -e + # THE FIXTURE'S OWN DENOMINATOR. It asserts "every rule compiles + # here", and "every" is a list it carries -- so a seventh member + # added to this package would be covered by a step whose name says it + # already is. The list is therefore compared against the package's + # own `[features]` before the build, and the two must be equal. + feats=$(sed -n '/^\[features\]/,/^# /p' mcpp.toml \ + | grep -oE '^[a-z-]+ +=' | sed 's/ *=//' | grep -v '^default$' | sort) + used=$(sed -n '/features = \[/,/\], host-module/p' tests/all-rules-compile/mcpp.toml \ + | grep -oE '"[a-z-]+"' | tr -d '"' | sort) + [ -n "$feats" ] || { echo "FAIL: read no features out of mcpp.toml"; exit 1; } + [ "$feats" = "$used" ] || { + echo "FAIL: the fixture does not name every published feature" + echo " package: $(echo $feats)" + echo " fixture: $(echo $used)" + exit 1; } + cd tests/all-rules-compile "$MCPP" build "$MCPP" run | tee run.log grep -q '^all-rules-compile ok' run.log diff --git a/mcpp.toml b/mcpp.toml index d53832c..18b7469 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,7 +1,7 @@ [package] name = "plugins" namespace = "mcpp" -version = "0.2.5" +version = "0.2.6" description = "Official mcpp build plugins: rule packages under mcpp.rules.*, build-time utilities under mcpp.tools.*, each member selected by a feature" license = "Apache-2.0" authors = ["mcpp-community"] diff --git a/rules/spirv.cppm b/rules/spirv.cppm index 865884f..070dbdb 100644 --- a/rules/spirv.cppm +++ b/rules/spirv.cppm @@ -36,9 +36,11 @@ // lines are not interchangeable. glslang's `-x --vn ` emits a COMPLETE C // declaration -- `const uint32_t [] = { ... };` -- while glslc's // `-mfmt=c` emits a BARE INITIALISER LIST, `{ ... }`, which is not a -// translation unit on its own. A rule that takes both therefore has to write -// the declaration around glslc's output itself, which `wrap_glslc_output` -// below does. +// translation unit on its own. So on both routes the compiler's output is a +// `.inc` and the PUBLIC header `.h` is written by this rule -- +// `write_header` below. It has to be that way round for a second reason: a +// generated header must be includable on its own, and glslang's complete +// declaration names `uint32_t` while including nothing. // // An earlier revision of this file supported glslang alone, and said why: // nothing in this ecosystem published glslc, "and a route with no payload @@ -447,20 +449,47 @@ inline std::vector device_shaders() { // plan time, before any action runs. Its content does not depend on the // shader's text, which is why nothing has to re-derive it when the shader // changes: ninja rebuilds the `.inc`, the `#include` picks it up. -inline bool wrap_glslc_output(const std::string& header, const std::string& inc, - const std::string& sym) { +// THE PUBLIC HEADER IS WRITTEN BY THE RULE ON BOTH ROUTES, AND IT HAS TO BE +// INCLUDABLE ON ITS OWN. +// +// glslc emits an initialiser list, so a declaration had to be written around it +// and that header was self-contained by construction. glslang's `-x --vn` emits +// a complete C declaration, so the rule wrote nothing -- and that file names +// `uint32_t` while including nothing: +// +// tri_vert.h:3:7: error: 'uint32_t' does not name a type +// +// Measured in a sandbox, on a program whose first include was the generated +// header. Every consumer that had worked put a Vulkan header ahead of it, which +// is why an incomplete header read as a working one for as long as nobody +// included it first. Two compilers producing an EQUIVALENT header is the whole +// premise of this rule choosing between them, and "equivalent" has to include +// this. +// +// So both routes now produce the same two files: `.inc` from the +// compiler, and `.h` from here. +inline bool write_header(const std::string& header, const std::string& inc, + const std::string& sym, flavour kind) { std::ofstream out{header, std::ios::trunc}; if (!out) { std::cerr << std::format("mcpp.rules.spirv: cannot write {}", header) << '\n'; return false; } - out << "// Generated by mcpp.rules.spirv. glslc emits an initialiser list;\n" - "// this declaration is what makes it a translation unit.\n" + const auto incName = std::filesystem::path(inc).filename().string(); + out << "// Generated by mcpp.rules.spirv.\n" "#pragma once\n" - "#include \n" - "static const uint32_t " << sym << "[] =\n" - "#include \"" << std::filesystem::path(inc).filename().string() << "\"\n" - ";\n"; + "#include \n"; + if (kind == flavour::glslc) { + out << "// glslc emits an initialiser list; this declaration is what makes it\n" + "// a translation unit.\n" + "static const uint32_t " << sym << "[] =\n" + "#include \"" << incName << "\"\n" + ";\n"; + } else { + out << "// glslang emits a complete `const uint32_t " << sym << "[]`; what this\n" + "// adds is the type it names and a guard.\n" + "#include \"" << incName << "\"\n"; + } return out.good(); } @@ -573,12 +602,14 @@ inline bool compile(std::span shaders, options opt = {}) { // named strings for the life of the statement that submits. const std::string id = "spirv:" + src; const std::string desc = std::string(cc.name()) + " " + src; - // For glslc the action's output is the initialiser list; for glslang it - // is the header itself. + // ONE SHAPE FOR BOTH ROUTES: the compiler writes `.inc` and this + // rule writes `.h` around it. glslang used to write the header + // itself, which made the two routes' headers differ in whether they + // could be included first -- see `write_header`. const std::string inc = base + ".inc"; - const std::string output = cc.kind == flavour::glslc ? inc : header; + const std::string output = inc; - if (cc.kind == flavour::glslc && !wrap_glslc_output(header, inc, sym)) return false; + if (!write_header(header, inc, sym, cc.kind)) return false; mcpp::action a; a.id = id.c_str(); diff --git a/tests/spirv-consumer/src/main.cpp b/tests/spirv-consumer/src/main.cpp index 55675ba..630c2d2 100644 --- a/tests/spirv-consumer/src/main.cpp +++ b/tests/spirv-consumer/src/main.cpp @@ -1,9 +1,19 @@ -// The generated header declares `const uint32_t scale_comp_spv[]`; the name -// is the shader's stem, its stage, and `_spv`, as mcpp.rules.spirv documents. -#include -#include +// THE GENERATED HEADER IS THE FIRST INCLUDE, AND THAT ORDER IS THE ASSERTION. +// +// It declares `const uint32_t scale_comp_spv[]` -- the name is the shader's +// stem, its stage, and `_spv`, as mcpp.rules.spirv documents -- and a header +// that names a type has to bring it. Putting `` above this line would +// satisfy the compiler on behalf of the header and hide whether the header can +// stand on its own; every consumer that had worked did exactly that, with a +// Vulkan header in front, and a sandbox found the state that leaves: +// +// tri_vert.h:3:7: error: 'uint32_t' does not name a type +// +// in a build the project did not write. #include "scale_comp.h" +#include + int main() { const std::uint32_t magic = scale_comp_spv[0]; std::printf("magic=%08x words=%zu\n", magic, sizeof scale_comp_spv / sizeof scale_comp_spv[0]);