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

cgal 5.0 #46251

Closed
wants to merge 4 commits into from
Closed

cgal 5.0 #46251

wants to merge 4 commits into from

Conversation

maxGimeno
Copy link
Contributor

This is a WIP PR to upgrade CGAL to the new version.
Just checking with the CI that everything is OK, waiting for the true Release to upgrade for real.

@fxcoudert
Copy link
Member

Formulas that need a revision bump (one commit per formula): graph-tool sfcgal pgrouting

@maxGimeno
Copy link
Contributor Author

After investigation, I found out that sfcgal was not compatible with c++11, but they are working on it see this PR for example.
On the other hand, graph-tools doesn't seem to be compatible wither with CGAL 5.0, I think their configure file explicitly looks for a .so , but we are now header only.

@fxcoudert
Copy link
Member

CGAL is now header-only? No library?

@maxGimeno
Copy link
Contributor Author

Exactly. It is still possible to build it, but it would be odd to provide the lib version on package managers when we document it as header-only, IMHO.

@fxcoudert
Copy link
Member

What's the point of these cmake flags, then?

      -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON
      -DCMAKE_INSTALL_NAME_DIR=#{HOMEBREW_PREFIX}/lib

@maxGimeno
Copy link
Contributor Author

None probably, I wasn't sure of their effect, so I left theim here but thought they probably became useless.

@maxGimeno
Copy link
Contributor Author

The release 5.0 is now officially out, and I updated the PR.

@Bo98
Copy link
Member

Bo98 commented Nov 23, 2019

Thanks for this. We're currently building Python 3.8 (see #45337), which involves revision bumping almost all Python formulae, so we're going to wait on this until that's finished to avoid merge conflicts. I'll let you know when we're ready.

@Bo98 Bo98 added do not merge python Python use is a significant feature of the PR or issue labels Nov 23, 2019
@fxcoudert
Copy link
Member

@maxGimeno graph-tool is not rebuilding, because its revision was increased by another PR. Please rebase on top of master.

@SMillerDev
Copy link
Member

Please don't use merge commits. It makes the CI go crazy.

@maxGimeno
Copy link
Contributor Author

@SMillerDev O, sorry. I rebased on master.

@fxcoudert
Copy link
Member

@maxGimeno in the process of rebasing, you lost the graph-tool revision bump.
While you redo, can you please make sure you have 1 commit per formula (file), with:

  • cgal 5.0 as commit message for the main formula
  • pgrouting: revision for cgal as commit message for the others

@fxcoudert fxcoudert removed do not merge python Python use is a significant feature of the PR or issue labels Jan 4, 2020
@fxcoudert
Copy link
Member

@BrewTestBot test this please

@fxcoudert
Copy link
Member

sfcgal fails to build:

  Could not find a configuration file for package "CGAL" that is compatible
  with requested version "4.3".

  The following configuration files were considered but not accepted:

    /usr/local/lib/cmake/CGAL/CGALConfig.cmake, version: unknown
    /usr/local/opt/cgal/lib/cmake/CGAL/CGALConfig.cmake, version: unknown

graph-tool fails to build:

configure: error: CGAL library not found

Formula/graph-tool.rb Outdated Show resolved Hide resolved
@maxGimeno
Copy link
Contributor Author

Ok, I just found out that graph-tool does not work in core/master either, when it does not come from a bottle. The compilation fails with the following :

graph_cairo_draw.cc:1868:34: error: use of undeclared identifier 'CoroGenerator'
    return boost::python::object(CoroGenerator(dispatch));

I saw in #47274 that graph-tool is not yet migrated to python 3.8, it may be related.

@maxGimeno
Copy link
Contributor Author

I did a patch and tried to install graph-tool from brew. Everything works but I'd like somebody from graph-tool to review my patch, to be sure.

@fxcoudert
Copy link
Member

Have the two patches been reported upstream? Are they accepted?

@maxGimeno
Copy link
Contributor Author

Sfcgal and pgrouting already have patches in their master branch, but I still need to report to graph-tool... And find a working fix for pgrouting.

@maxGimeno maxGimeno force-pushed the upgrade_cgal branch 3 times, most recently from 2e05515 to ce9ddee Compare January 20, 2020 12:58
@maxGimeno maxGimeno force-pushed the upgrade_cgal branch 3 times, most recently from 44acd70 to d1652f6 Compare January 21, 2020 11:13
Formula/cgal.rb Outdated Show resolved Hide resolved
@maxGimeno
Copy link
Contributor Author

@fxcoudert This time it seems ready \o/

@lrineau
Copy link
Contributor

lrineau commented Jan 21, 2020

@fxcoudert Hi FX.

Have the two patches been reported upstream? Are they accepted?

In the last version, Maxime has added a comment for each of the three patches.

  • ✔️ SFCGAL and pgRouting are already fixed upstream (in their master branches, no new releases so far).
  • ⛔ The issue has been reported to graph-tool, but the upstream developer wants to wait for the arrival of CGAL-5.0 in ArchLinux, and ArchLinux is waiting for a new release of OpenSCAD (fixed with CGAL-5.0 in its master branch, a release 2020.01 is planned).

Is that blocking the integration of this PR? This PR also fixes the build of pgRouting (broken in the master of homebre-core) independently of CGAL-5.0.

@chenrui333 chenrui333 added the ready to merge PR can be merged once CI is green label Jan 22, 2020
@chenrui333
Copy link
Member

@BrewTestBot test this please

@chenrui333
Copy link
Member

Thanks @maxGimeno @lrineau!!

@chenrui333 chenrui333 changed the title UpgradeCGAL to 5.0 cgal 5.0 Jan 22, 2020
@lock lock bot added the outdated PR was locked due to age label Feb 28, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants