FairRootGroup / FairRoot

C++ simulation, reconstruction and analysis framework for particle physics experiments
http://fairroot.gsi.de
Other
57 stars 96 forks source link

Fix a runtime error with newer XCode versions #1478

Closed fuhlig1 closed 9 months ago

fuhlig1 commented 10 months ago

When using boost/program_options in a header file the generated dictinonary can't be loaded at runtime and the macro crashes. Guard the usage of boost/program_options in the header file such that code is only used during compilation but not when using cling. The problem was already reported to the ROOT team and will not be fixed since the underlying problem is obviously in the libc++ shipped by Apple. (see https://github.com/root-project/root/issues/12762)


Checklist:

fuhlig1 commented 10 months ago

The problem showed up with the latest XCode version 15. According to the bug report reported to the ROOT team the problem already appeared with XCode 13.3.

The fix should be ported to v18.6_patches and v18.8_patches.

fuhlig1 commented 10 months ago

It doesn't hurt to be switched off for any system and compiler. The data members are not needed by the interpreter so they can be hidden.

dennisklein commented 10 months ago

Well, someone else needs to approve this. I find it aweful (the ifdef cling), why do we add such files to the LinkDef list at all? And why FairSimConfig (suggests to be an official FairRoot framework class) in examples/tutorial1?

fuhlig1 commented 10 months ago

Well, someone else needs to approve this. I find it aweful (the ifdef cling), why do we add such files to the LinkDef list at all? And why FairSimConfig (suggests to be an official FairRoot framework class) in examples/tutorial1?

I also don't like the fix. We need ROOT to generate the glue code such that the class can be used from the macro. When this class was added and why it was put to the examples I can't tell.

ChristianTackeGSI commented 10 months ago

So, having read the root issue, it seems that this is a plain bug in apple's libraries.

We generally don't support broken things (https://github.com/root-project/root/issues/12762#issuecomment-1606919636)

I think, we should follow that!

If you still want to fix this for macos, I see only one way forward: Hide the whole boost program options stuff from the interpreter without using preprocessor guards.

  1. Either try to go for some forward declaration of class FairSimConfig; in the relevant macros / etc.

    This might not be trivial, because things like

    FairSimConfig config;
    DoSomething(config);

    don't work with a declaration.

    One would probably need to go for something like

    void DoSomething(FairSimConfig const* config = nullptr)
    {
       FairSimConfig default_config;
       if (!config) {
           config = &default_config;
       }
    }

    which isn't pleasant either.

  2. Do a pimpl style thing

    #include <memory>
    class FairExampleProgramOptions;
    class FairSimConfig
    {
     private:
       std::unique_ptr<FairExampleProgramOptions> po;
    };

    … and move all of FairExampleProgramOptions into the .cxx.

I really don't know how to go forward here.

ChristianTackeGSI commented 10 months ago

And yes, we have a huge amount of names in the example directory that look like they're official FairRoot classes/etc. My personal favourite: FairStack. I think, I have even seen one project using #include <FairStack.h>.

We should really start renaming (and/or moving them into fairroot::examples::?) them!

dennisklein commented 10 months ago

I think, we should follow that!

If you still want to fix this for macos, I see only one way forward: Hide the whole boost program options stuff from the interpreter without using preprocessor guards.

I like this approach as well. Another idea how to hide it:

ChristianTackeGSI commented 10 months ago

Moving this into local variables sounds even better!

Just FairSimConfig::PrintHelpMessage uses fDescription as well.

struct FairExampleTut1OptionDescription
{
    po::options_description fDescription{"Options"};
    FairExampleTut1OptionDescription() { /* setup fDescription */ }
}

int FairSimConfig::ParseCommandLine(int argc, char* argv[])
{
    FairExampleTut1OptionDescription description;
    po::variables_map map;
    // … description.fDescription), map …
}

void FairSimConfig::PrintHelpMessage()
{
    FairExampleTut1OptionDescription description
    std::cout << description.fDescription << std::endl;
}
dennisklein commented 10 months ago

One could cache the help message in a string. Just a bit wasteful, if it is never printed though.

ChristianTackeGSI commented 10 months ago

One could cache the help message in a string. Just a bit wasteful, if it is never printed though.

Classic cpu time vs. space optimization.

I don't really care, a few kB vs. a few µs.

fuhlig1 commented 10 months ago

I used the second option and initialise the description two times.

ChristianTackeGSI commented 10 months ago

Those two are still open:

fuhlig1 commented 10 months ago

Those two are still open:

Where do I find the results of the formatter. Since the CMake version is not matching no format check is done.

ChristianTackeGSI commented 10 months ago

Where do I find the results of the formatter. Since the CMake version is not matching no format check is done.

  1. Check the "Some checks were not successful" section at the end of just this PR
  2. https://alfa-ci.gsi.de/blue/organizations/jenkins/FairRootGroup%2FFairRoot/detail/PR-1478/5/pipeline/24
fuhlig1 commented 10 months ago

Where do I find the results of the formatter. Since the CMake version is not matching no format check is done.

  1. Check the "Some checks were not successful" section at the end of just this PR
  2. https://alfa-ci.gsi.de/blue/organizations/jenkins/FairRootGroup%2FFairRoot/detail/PR-1478/5/pipeline/24

That I did and I clicked on the Details link at the end of the check/format line expecting that this will bring me to the results of this test. Obviously this only brings me to the overview page where I have to chose the correct test.

ChristianTackeGSI commented 9 months ago

There has been plenty of time for new comments. So I merged it now.

ChristianTackeGSI commented 9 months ago

Backported to 18.8: #1483 Backported to 18.6: 7a0391683b2f187db9848aa9ef3fee875a47f6aa (without PR, hope that was okay)