Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove --enable-cgal for rc6, re-enable in rc7 #59

Merged
merged 3 commits into from
Aug 5, 2021

Conversation

lgray
Copy link
Collaborator

@lgray lgray commented Aug 4, 2021

https://gitlab.com/fastjet/fastjet/-/blob/fastjet-3.3.4/INSTALL#L55-60

Critically it is necessary to remove this line for builds to work on centos7.
It does not affect building on debian-likes.

@lgray
Copy link
Collaborator Author

lgray commented Aug 4, 2021

Would recommend that a new rc6 is made once this PR is merged!

@lgray
Copy link
Collaborator Author

lgray commented Aug 4, 2021

After some discussion with @jpivarski he told me the documentation is wrong and this simply disabled CGAL.

Still, in that case, for keeping things moving along with coffea / coffea-casa, I would prefer a Release Candidate with CGAL simply turned off, and push binary-wheels to rc7 with cgal turned on.

Coffea casa will be insensitive to this progression of versions.

@jpivarski
Copy link
Member

We can temporarily accept this, to make an RC release, but earlier experiments showed that removing --enable-cgal disables CGAL entirely, even though --enable-cgal-header-only is still there. The text of the documentation you highlighted suggests that you only need one option, but we found earlier that you do need both (and cross-checked with Gavin).

We'll eventually need to ship wheels with CGAL (since this is a library and we don't know if it's going to be used in high-multiplicity applications), and will therefore have to turn this option back on. We had some way of verifying that CGAL is, in fact, included, and that was working on GitHub Actions. As long as it does, the binary wheels it builds will be correct. But that's after this next RC release.

@aryan26roy, could you please merge this and make a new RC release? We'll just have to remember to change it back later.

@lgray lgray changed the title remove --enable-cgal, it is incorrect for CGAL 5 remove --enable-cgal for rc6, re-enable in rc7 Aug 4, 2021
@lgray lgray mentioned this pull request Aug 4, 2021
@aryan26roy
Copy link
Collaborator

@jpivarski , alright I will make a new release with cgal turned off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants