Related: #2677, and the direct cause of the WITH_VTK=OFF breakage described there.
The rule in SCIRun is that every module is always built and always registered, so that a saved network always loads; modules whose optional dependency is absent compile their internals out and report themselves disabled. InterfaceWithTetGen, OsprayViewer and the Python modules all implement this. The mechanism works, but it is held together entirely by convention, and it just failed on refactor_add_vtk.
What's wrong with the current mechanism
DISABLED_WITHOUT_ABOVE_COMPILE_FLAG (src/Dataflow/Network/ModuleTraits.h:86) is positional — it names no flag, it means "whatever #ifndef happens to be directly above me". Nothing binds the two together, and nothing checks that the pairing is right.
Each gated module then has to keep four things in sync by hand:
- the
#ifndef WITH_X / DISABLED_WITHOUT_ABOVE_COMPILE_FLAG pair in the header
- an
#else error("...") in execute() with the flag name retyped into a string literal
- a CMake split so the dependency-touching TU is the only gated one
- an
ADD_DEFINITIONS(-DWITH_X) somewhere that covers every directory that includes the header
Step 4 is the one that actually bites, and it is currently wrong on master — see the ODR issue filed alongside this one. Step 3 is what refactor_add_vtk got wrong. Step 2 is unchecked prose.
There is also a rot problem that is arguably worse than any of these: the disabled branch of an #ifdef is not compiled in the enabled configuration, so it is never type-checked by the developer who is working on the feature. That is precisely how the VTK OFF path ended up broken while the ON path was fine.
And there is no introspection. ModuleProxyWidget.cc:151 greys out a disabled module and can only tell the user "you might need a different build of SCIRun" — it cannot say which flag, because that information was never captured anywhere.
Proposal
1. A generated build-flag header. The sci_defs mechanism already exists (src/include/sci_defs/*.h.in) and is the natural home. Emit one constexpr bool per WITH_ flag, included everywhere, replacing the scattered directory-scoped ADD_DEFINITIONS:
namespace SCIRun::BuildFlags {
inline constexpr bool WithVtk = @SCIRUN_WITH_VTK@;
inline constexpr bool WithTetgen = @SCIRUN_WITH_TETGEN@;
inline constexpr bool WithOspray = @SCIRUN_WITH_OSPRAY@;
}
2. One macro that names its own flag, used unconditionally — no #ifndef at the use site:
#define REQUIRES_BUILD_FLAG(flag) \
public: \
bool isImplementationDisabled() const override { return !(flag); } \
std::string disabledBuildFlag() const override { return #flag; }
REQUIRES_BUILD_FLAG(SCIRun::BuildFlags::WithVtk)
3. if constexpr in execute() instead of #ifdef, so both branches type-check in both configurations:
if constexpr (!BuildFlags::WithVtk) { error("Module requires WITH_VTK."); return; }
This needs the impl symbols declared in the off build, which is exactly the InterfaceWithTetGenImpl split that already exists — no new concept, just applied consistently.
What this buys
- The class definition becomes identical in every TU, so the ODR hazard disappears by construction rather than by discipline.
- The flag name lives in the source, so it cannot drift from the guard or the error string.
- The disabled path is compiled in every configuration and stops rotting — the specific failure mode that produced the VTK breakage.
- The GUI can name the required flag instead of shrugging, and a "what's disabled in this build and why" report becomes possible.
Optional follow-on
Put "requires": "WITH_VTK" in the .module JSON so CMake, the three factory generators and the header all read one field. That would also retire the hand-written-factory workaround for testing-only modules, which exists today only because a BUILD_TESTING guard cannot be expressed in a .module file.
Scope
Mechanism plus migration of the six existing users (InterfaceWithTetGen, OsprayViewer, four Python modules) and the two VTK modules. The if constexpr conversion can be done per-module afterwards and does not have to land at once.
Related: #2677, and the direct cause of the
WITH_VTK=OFFbreakage described there.The rule in SCIRun is that every module is always built and always registered, so that a saved network always loads; modules whose optional dependency is absent compile their internals out and report themselves disabled.
InterfaceWithTetGen,OsprayViewerand the Python modules all implement this. The mechanism works, but it is held together entirely by convention, and it just failed onrefactor_add_vtk.What's wrong with the current mechanism
DISABLED_WITHOUT_ABOVE_COMPILE_FLAG(src/Dataflow/Network/ModuleTraits.h:86) is positional — it names no flag, it means "whatever#ifndefhappens to be directly above me". Nothing binds the two together, and nothing checks that the pairing is right.Each gated module then has to keep four things in sync by hand:
#ifndef WITH_X/DISABLED_WITHOUT_ABOVE_COMPILE_FLAGpair in the header#else error("...")inexecute()with the flag name retyped into a string literalADD_DEFINITIONS(-DWITH_X)somewhere that covers every directory that includes the headerStep 4 is the one that actually bites, and it is currently wrong on master — see the ODR issue filed alongside this one. Step 3 is what
refactor_add_vtkgot wrong. Step 2 is unchecked prose.There is also a rot problem that is arguably worse than any of these: the disabled branch of an
#ifdefis not compiled in the enabled configuration, so it is never type-checked by the developer who is working on the feature. That is precisely how the VTK OFF path ended up broken while the ON path was fine.And there is no introspection.
ModuleProxyWidget.cc:151greys out a disabled module and can only tell the user "you might need a different build of SCIRun" — it cannot say which flag, because that information was never captured anywhere.Proposal
1. A generated build-flag header. The
sci_defsmechanism already exists (src/include/sci_defs/*.h.in) and is the natural home. Emit oneconstexpr boolperWITH_flag, included everywhere, replacing the scattered directory-scopedADD_DEFINITIONS:2. One macro that names its own flag, used unconditionally — no
#ifndefat the use site:REQUIRES_BUILD_FLAG(SCIRun::BuildFlags::WithVtk)3.
if constexprinexecute()instead of#ifdef, so both branches type-check in both configurations:This needs the impl symbols declared in the off build, which is exactly the
InterfaceWithTetGenImplsplit that already exists — no new concept, just applied consistently.What this buys
Optional follow-on
Put
"requires": "WITH_VTK"in the.moduleJSON so CMake, the three factory generators and the header all read one field. That would also retire the hand-written-factory workaround for testing-only modules, which exists today only because aBUILD_TESTINGguard cannot be expressed in a.modulefile.Scope
Mechanism plus migration of the six existing users (
InterfaceWithTetGen,OsprayViewer, four Python modules) and the two VTK modules. Theif constexprconversion can be done per-module afterwards and does not have to land at once.