Drop COIN_HAS_NTY from public header CbcModel.hpp

[jschueller]

COIN_HAS_NTY is only defined during compilation of Cbc but is used in public header CbcModel.hpp
Hiding attributes to 3rd party code can lead to crashes from alignment issues
We also have to unconditionnaly include CbcSymmetry.hpp in case nauty is disabled

Fixes 1e6e301
Closes #591

cc @tkralphs @jjhforrest @svigerske

[tkralphs]

Hmm, looks like CbcSymmetry includes Nauty types, so CbcSymmetry.hpp cannot just be included unconditionally and this inclusion was not what broke things, as it was already not being included when it was working. This part was not introduced recently. CbcSymmetry was forward declared in order to allow the code to build even without including CbcSymmetry.hpp.

It looks to me like the fix that's already in master (which was probably added for a similar reason) is probably what we really want. However, let's not alter what was already previously working just only undo what was introduced in 1e6e301. @jschueller can you fix? Is it clear what I'm saying?

[tkralphs]

This made me realize that although nauty is being installed on the build machine in Github Actions, it is not being detected and linked properly, so we are not actually testing with nauty. Realistically, we should be doing tests both with and without nauty to catch exactly this kind of thing.

[tkralphs]

So not only does one needs to provide configure with --with-nauty-incdir and --with-nauty-lib in order for nauty to be detected in stable/2.10, but the test itself is also broken, since it is looking for nauty.h, which I guess was correct for an older version of nauty, but is not there now.

[tkralphs]

Which makes me wonder how you are installing nauty @jschueller?

[tkralphs]

And actually also how did you get Cbc to detect nauty, @jjhforrest?

[jschueller]

So do you want me to revert 1e6e301 or ?

nauty is detected via --with-nauty-incdir / --with-nauty-lib in archlinux:
https://github.com/archlinux/svntogit-community/blob/packages/coin-or-cbc/trunk/PKGBUILD
nauty.h is still available with latest nauty 2.8.6: https://archlinux.org/packages/community/x86_64/nauty/

[tkralphs]

nauty.h is still available with latest nauty 2.8.6: https://archlinux.org/packages/community/x86_64/nauty/

Interesting, both the Ubuntu and Red Hat packages for Nauty 2 are missing nauty.h and it's not included from Cbc either.

So do you want me to revert 1e6e301 or ?

I don't think it's necessary to revert it wholesale, it's just that some of what you did revert in the commit was not actually introduced in 1e6e301. Specifically, CbcSymmetry.hpp can't be included unless nauty is actually present. I assume everything else is OK.

[jschueller]

But if CbcSymmetry.hpp is not included the attributes types wont be resolved right ?
I dont think we have other choice than make the macro public, or ?

[tkralphs]

Since it was working before without the COIN_HAS_NTY around #include "CbcSymmetry.hpp", I would assume that only the pointer to CbcSymmetry is actually included in CbcModel and the forward declaration of CbcSymmetry here then makes it unnecessary to include the header itself. But let me just to build try locally and see what's going on.

[tkralphs]

Just including CbcSymmetry.cpp unconditionally in CbcModel.cpp seems to fix things up. I tested both with and without Nauty and also with and without multi-threading.

I was wrong about the missing nauty.h. It did seem weird. For some reason, it is packaged in the arch-specific header directory /usr/include/x86_64-linux-gnu/nauty rather than in /usr/include/nauty (where the rest of the headers are). So it seems we are all good now, but I may actually modify Github Actions to test with nauty, since it is currently not doing that.

[tkralphs]

@jschueller If you could test this locally before I merge, that would be great.

[tkralphs]

OK, now building with nauty on Linux.

[jschueller]

it works on my end locally too

[tkralphs]

There was one remaining use of COIN_HAS_NTY in CbcModel.hpp, which probably doesn't affect anything, but it's better just not have the symbol appearing at all. If everything looks OK, after this, I'll merge and make a release.

[jschueller]

great! thanks a lot