Turning on Awake module results in full recompilation

Issue #162 wontfix
Laurie Nevay created an issue

Turning the awake module on forces recompilation of the almost all the code in BDSIM. Only BDSComponentFactory.cc / .hh has optional includes for the awake module. The component factory header is included in only 3 .cc files and in no .hh files, so I would expect minimal compilation.

For example, it's unclear why the output classes need to be recompiled for this.

Comments (7)

  1. Jochem Snuverink

    I agree this is unnecessary. To explain why cmake does this: with USE_AWAKE the compiler definition flag -DUSE_AWAKE is added to the compilation flags. And since the compiler / CMake doesn't check if the flag is actually in the file (might not be so easy due to includes), it recompiles them.

    This is also the case with other compiler flags that we have: ROOTDOUBLE, USE_GDML, USE_GZSTREAM, BDSDEBUG etc.

    One could set the compiler definition to specific files instead (probably the 3 AWAKE classes, ComponentFactory, but also its dependents (since the definition is in the header): DetectorConstruction, BendBuilder). I haven't done this since this is a less generic and error-prone approach (a mistake there can result in strange crashes). I would be hesitant to go that way.

    Another option is to create a separate library, bdsim.so and bdsim-awake.so, and link the executable to the different library depending if the AWAKE-module is switched on or off. That doesn't need recompiling when switching back and forth (but still for a single switch).

    Probably there are other solutions, but I don't think there is a simple generic solution. What I usually do is to have different build directories for builds with different flags, which I think is not so bad.

  2. Jochem Snuverink

    As discussed above: I don't see an easy solution and having different build directories for different flags is acceptable. So unless we come up with a better scheme I propose to close this as "won't fix".

  3. Laurie Nevay reporter

    I'd agree with this. It's better understood now.

    Although, it's only the component factory that requires this. Could we conceptually provide some interface in the code for external modules - that provide element types and construction methods. We would then have something like this in bdsim.cc

    #ifdef USE_AWAKE BDSModules::Register(new BDSModuleAwake()); #endif

    BDSModuleAwake would inherit BDSModule - a base class that defines the required interfaces. The module could use BDSIM classes. BDSComponentFactory would test against known BDSIM element types and then ask the BDSModules interface if any registered modules could provide the required type.

    This way, only bdsim.cc would need to be recompiled and then relinked against the bdsim library.

    For now, I agree we don't need to do anything, but it may be good to define a clear pattern for users to add their own beam line elements.

  4. Jochem Snuverink

    We can certainly review the current module implementation. A scheme as you propose could certainly work although I don't see yet how in detail, but there must exist these type of software patterns.

    Actually rethinking a bit: already now we could improve to compilation of ComponentFactory only, since the two AWAKE methods CreateAwakeScreen() and CreateAwakeSpectrometer() are private.

    • We could move them over to BDSComponentFactory.cc and declare them outside of the class. That will work since we can pass the few relevant private members to the function.
    • Another solution I like perhaps better is to use the PIMPL idiom (http://en.cppreference.com/w/cpp/language/pimpl) on ComponentFactory.
  5. Jochem Snuverink

    I tried to put the private implementation of ComponentFactory in the source, so that it is the only place that has the preprocessor definition (I could push these changes if deemed useful). Then I changed CMakeLists.txt so that -DUSE_AWAKE is only added to ComponentFactory. Unfortunately, cmake still insists in recompiling all bdsim sources, which I don't fully understand but it seems to be its behaviour. The proposed ifdef in bdsim.cc will likely give the same behaviour.

    A separate library seems necessary at this point. And before changing things for the reason of recompilation, one might want to verify if cmake abides.

  6. Jochem Snuverink

    it may be good to define a clear pattern for users to add their own beam line elements.

    I think we have a reasonably clear pattern:

    • define new element names in parser.l and parser.y
    • define element types in GMAD::element and elementtype
    • construct elements in ComponentFactory within preprocessor ifdef
    • update main CMakeLists.txt with new option (few lines)
    • put source code in modules/NewModule/
    • optionally: add new keywords for the new elements, but preferably old keywords should be reused as much as possible
  7. Log in to comment