Make diagnostic system aware of un-compiled modules

[MarkusPrim]

Addresses #61 , improves over the previous implementation and now directly uses the harvester routines.

[tegonzalo]

Looks good. A few cosmetic changes. Works as expected. In the future please do not bundle a file format change with actual changes, as it's hard to disentangle where the actual changes are.

[tegonzalo]

Ah belay that. I found 3 cases where your set up breaks. Not your fault though, but we should fix them now. For susyhit, libmathematicatest and classy_exo your system fails.

The first two is because the header names are different from the backend names (macro BACKENDNAME in the header), so the names as read from the config file won't match those registered. It should be as simple as changing the backend or header names I think.

The last one is a bit more tricky, as "exo" is part of the version for some reason. I'm not sure why is that. Perhaps it could just be swallowed into the backend name to avoid these issues. @pstoecker Would that break anything?

While we are at it, there is one more diagnostic-type change that this needs. When Mathematica is absent, the status message reads "Mathematica absent" for Mathematica backends, but when pybind11 is absent the status of python backends just reads "absent/broken". It'd be great if these could also say "pybind11 absent".

[pstoecker]

The last one is a bit more tricky, as "exo" is part of the version for some reason. I'm not sure why is that. Perhaps it could just be swallowed into the backend name to avoid these issues. @pstoecker Would that break anything?

The reason is that we wanted to treat exoclass as a special version of class (or classy in our language) rather than an own backend. It actually used to be the way that classy_exo was technically a backend on it's own but we have decided that it might be easier to be a subversion of classy in cases that we have to list only special versions of classy to be suitable for a given capability.

As far as I know, there is no such restriction in the codebase such that it would not make a difference if classy_exo_2.7.2 refers to version exo_2.7.2 of classy or if it refers to version 2.7.2 of classy_exo. As long as the names of the backend functions are the same, everything should work as expected.

[MarkusPrim]

In the future please do not bundle a file format change with actual changes, as it's hard to disentangle where the actual changes are.

Sorry about that, you are 100% correct that this should have been disentangled.

[MarkusPrim]

As far as I know, there is no such restriction in the codebase such that it would not make a difference if classy_exo_2.7.2 refers to version exo_2.7.2 of classy or if it refers to version 2.7.2 of classy_exo. As long as the names of the backend functions are the same, everything should work as expected.

I think that change would automatically fix the issue at hand. @tegonzalo are you happy fixing it that way, and that we enforce version numbers in the future? The system checks for name + version number in the current implementation, so anything we add in the future without that structure will not work.

Edit: Actually my statement above is not really correct. I think the issue is that the backend defined in classy_exo_2_7_2.hpp is also named classy, and that breaks the diagnostic system here. Is it feasible to rename in this instance the backendname from classy to classy_exo?

[tegonzalo]

Is it feasible to rename in this instance the backendname from classy to classy_exo?

Yes, I think that's the way forward, just treat it as a new backend with name classy_exo

[MarkusPrim]

@pstoecker I changed the classy_exo backendname, so _exo is now part of the name not the version. Can you check if this breaks anything? Or alternatively can you tell me how I can check by myself if everything still works as expected (beyond compiling).

[pstoecker]

@MarkusPrim , thanks for the change. Looks good as far as I can tell. I will test it. But probably not before tomorrow.

[MarkusPrim]

While we are at it, there is one more diagnostic-type change that this needs. When Mathematica is absent, the status message reads "Mathematica absent" for Mathematica backends, but when pybind11 is absent the status of python backends just reads "absent/broken". It'd be great if these could also say "pybind11 absent".

@tegonzalo I think I have a working solution, please see if this is what you had in mind.

[tegonzalo]

I think you went a bit too far changing the backend names. For LibMathematicaTest and classy_exo it was perfect. But for SUSY-HIT it's too much. The backend name is with -, so it should always be referred to as such in comments and others. In fact I don't know why it was used with _ in the backend name, wouldn't it work the same with -? Or maybe it will mess up some macros. It will do some tests to see if it works fine as SUSY-HIT, but if not only the backend name and frontend file names need to be with _.

[tegonzalo]

No, C++ does not allow - in variable names. That is the conundrum I tried to resolve by the global replacement of - to _.

Ok, yes, I suspected that. So then as I said, only make the changes where it's necessary, all other referrals to SUSY-HIT should be with -

[tegonzalo]

Also, I've just noticed this from the previous PR that I missed. Is this a typo or what exactly is it meant here?

gambit/Core/src/diagnostics.cpp

Line 176 in 75bbfdd

<< "All your backend are belong to us."

I have no idea.

Ok yes sorry, git blame told me it was you, but I just noticed you just formatted it. This dates back the creation of this file so I haven't yet figured out who wrote it to ask them what they intended it to mean. I'll keep digging.

[MarkusPrim]

Also, I've just noticed this from the previous PR that I missed. Is this a typo or what exactly is it meant here?

gambit/Core/src/diagnostics.cpp

Line 176 in 75bbfdd

<< "All your backend are belong to us."

I have no idea.

Ok yes sorry, git blame told me it was you, but I just noticed you just formatted it. This dates back the creation of this file so I haven't yet figured out who wrote it to ask them what they intended it to mean. I'll keep digging.

I believe it to be an easert egg ;)

[pathron]

Haha, I think that is very very old one :).

[ahye]

https://en.wikipedia.org/wiki/All_your_base_are_belong_to_us

[pstoecker]

I have tested the changes to classy_exo_2.7.2. Everything is fine from what I have tested. The only thing that needed to be changed is to adjust the logger and error messages as these assumed that exo is part of VERSION. I have fixed it in my latest commit.

While testing I have seen that 16324a3 has broken the gambit build. In particular adding "Core" to exclude_dirs in line 568 of Utils/scripts/harvesting_tools.py caused that make_module_rollcall is never called and Core/include/gambit/Core/module_rollcall.hpp will not be created. This is only noticeable when you start from scratch so this issue might have been overlooked when this file was already in place. This could be fixed by just deleting "Core" from exclude_dirs.

[MarkusPrim]

I have tested the changes to classy_exo_2.7.2. Everything is fine from what I have tested. The only thing that needed to be changed is to adjust the logger and error messages as these assumed that exo is part of VERSION. I have fixed it in my latest commit.

Merci!

While testing I have seen that 16324a3 has broken the gambit build. In particular adding "Core" to exclude_dirs in line 568 of Utils/scripts/harvesting_tools.py caused that make_module_rollcall is never called and Core/include/gambit/Core/module_rollcall.hpp will not be created. This is only noticeable when you start from scratch so this issue might have been overlooked when this file was already in place. This could be fixed by just deleting "Core" from exclude_dirs.

Another good catch. I never do full rebuilds locally. I was hoping our CI catches issues like this, but it seems a bit shaky at the moment.

[tegonzalo]

I finally had time to look into this and fix it. Besides a bug from the harvesting script, there was nothing else to do, as @MarkusPrim already left this in a good state. It is then ready to merge from my point of view.

There are some problems with the OSX CI tests, but I don't really understand them, they seem to be unrelated and something to do with internal problems of the system? @ChrisJChang do you know anything about it?

[ChrisJChang]

There are some problems with the OSX CI tests, but I don't really understand them, they seem to be unrelated and something to do with internal problems of the system? @ChrisJChang do you know anything about it?

I only restarted the arm64 Mac CI today, which had required me to go back and install all the dependencies again. I suspect I forgot something, I will investigate this. I don't think this is specific to this PR, since the same error occurs on master. The x64 Mac pro fails the CI jobs when it gets to building rivet for the ColliderBit_CMSSM.yaml test. This is an issue that was noticed on another PR, and is being worked on.

[tegonzalo]

Ok, then I'll merge this PR to master