fix(DO NOT MERGE): Remove '--enable-cgal' option incompatible with header-only

[matthewfeickert]

trafficstars

• Resolves https://github.com/scikit-hep/fastjet/issues/193
• Will also fix the problem that PR #301 is stuck on
• Reverts most of PR #70

---

• After FastJet's autogen.sh has been run and the configure script generated, ./configure --help shows

  --enable-cgal           enables link with the CGAL library default=no
  --enable-cgal-header-only   enable build with header-only install of CGAL, e.g. as for CGALv5; in that case do not use --enable-cgal default=no

• c.f. https://gitlab.com/fastjet/fastjet/-/tree/67796a1d43981d25a0f6d197fdfb71571028caf0

• As CGAL is a header only library there is no need to link against it or configure it and so the --enable-cgal-header-only is all that is required.
  • c.f. https://doc.cgal.org/5.6.1/Manual/usage.html#section_headeronly
• Removal of the option conflict seems to resolve multiple issues linking errors with CGAL.

[matthewfeickert]

Given https://github.com/scikit-hep/fastjet/pull/59#issuecomment-892936463 and checking the logs for

Configuration summary:
----------------------
  Installation directory     /home/runner/work/fastjet/fastjet/src/fastjet/_fastjet_core
  Shared libraries           yes
  Static libraries           yes
  Debug flag                 yes
  CGAL support               no
  Thread safety              yes (requires C++11)
  Plugins: EECambridge       yes
           Jade              yes
           NestedDefs        yes
           SISCone           yes
           CDFCones          yes
           D0RunICone        yes
           D0RunIICone       yes
           ATLASCone         yes
           CMSIterativeCone  yes
           PxCone            no
           TrackJet          yes
           GridJet           yes
  Monolithic plugins lib     yes
  Python interface           yes
  Swig python generator      yes
---------------------------------------------------

it seems that if this is correct that --enable-cgal-header-only is actually wrong(?). Though looking at @gavinsalam's messages in https://gitlab.com/fastjet/fastjet/-/commit/461dcecfd1b37650036f3a4931d76bd4eb4559d3

explicitly mentioned that --enable-cgal-header-only is to be used instead of --enable-cgal when using CGAL v5

this seems to contradict the configuration summary. Is it possible that the configuration summary only gives information on what is processed through the ./configure and as CGAL is header only there's nothing for it to do and so it gives

  CGAL support               no

when there is nothing wrong?

edit: Ah I see in the logs that the flag -DDROP_CGAL is getting set so it is definitely not getting used.

cc @jpivarski @lgray

[matthewfeickert]

So if you use --enable-cgal-header-only but also have no CGAL headers on the machine at all (so there's no way for it to find the headers to build) then you still get

Configuration summary:
----------------------
  Installation directory     /home/runner/work/fastjet/fastjet/src/fastjet/_fastjet_core
  Shared libraries           yes 
  Static libraries           yes 
  Debug flag                 yes 
  CGAL support               no  
  Thread safety              yes (requires C++11)
  Plugins: EECambridge       yes 
           Jade              yes          
           NestedDefs        yes          
           SISCone           yes          
           CDFCones          yes          
           D0RunICone        yes          
           D0RunIICone       yes          
           ATLASCone         yes          
           CMSIterativeCone  yes          
           PxCone            no           
           TrackJet          yes          
           GridJet           yes          
  Monolithic plugins lib     yes 
  Python interface           yes 
  Swig python generator      yes 
---------------------------------------------------

so I'm not sure how to understand the comment about using --enable-cgal-header-only with CGAL v5.

[gavinsalam]

The previous correct usage was --enable-cgal --enable-cgal-header-only, but that was incorrectly documented in the configure help message. Given that header-only installations of CGAL are now the default, we have changed the behaviour so that --enable-cgal is sufficient for CGAL to be included in the build, as long as CGAL is in a standard location. (Non-default locations can still be specified, e.g. with --with-cgaldir=/opt/homebrew for CGAL from homebrew on mac)

[matthewfeickert]

Given that header-only installations of CGAL are now the default, we have changed the behaviour so that --enable-cgal is sufficient for CGAL to be included in the build, as long as CGAL is in a standard location. (Non-default locations can still be specified, e.g. with --with-cgaldir=/opt/homebrew for CGAL from homebrew on mac)

That's great, @gavinsalam. Thanks!

Is this change in a tagged release of FastJet (https://gitlab.com/fastjet/fastjet/-/tags)?

[gavinsalam]

No, not yet. We’re still processing some other merge requests. Hopefully in a week or two. Feel free to ping us again if there’s no progress. G.

On 3 Dec 2024, at 21:29, Matthew Feickert @.***> wrote:

Given that header-only installations of CGAL are now the default, we have changed the behaviour so that --enable-cgal is sufficient for CGAL to be included in the build, as long as CGAL is in a standard location. (Non-default locations can still be specified, e.g. with --with-cgaldir=/opt/homebrew for CGAL from homebrew on mac)

That's great, @gavinsalamhttps://github.com/gavinsalam. Thanks!

Is this change in a tagged release of FastJet (https://gitlab.com/fastjet/fastjet/-/tags)?

— Reply to this email directly, view it on GitHubhttps://github.com/scikit-hep/fastjet/pull/319#issuecomment-2515588093, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABKV3AJPZGMDT5FFYXCNGGL2DYPEVAVCNFSM6AAAAABOK5FZ6CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJVGU4DQMBZGM. You are receiving this because you were mentioned.Message ID: @.***>

[lgray]

@matthewfeickert using only --enable-cgal-header-only seems to remove CGAL support entirely, and given that I've been able to get things to build viable wheels we probably want to avoid removing optimizations like that.

I've marked it as do not merge since it's effectively a performance regression. We should keep it open for the ongoing discussion with Gavin Salam.

[lgray]

@matthewfeickert Since things are now in a delicate but building state. It may be worth it to comb through rather dirty build I hacked together and see if we can take the lessons from a few other of your PRs and apply them while keeping CGAL and minimizing heavy handedness with build flag environment variables.

I have a feeling we could get the macos universal wheels going with a bit of careful touch.

[lgray]

Closed by #342