Skip to content

Build sirocco extension with C++ #40369

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 25, 2025
Merged

Build sirocco extension with C++ #40369

merged 1 commit into from
Jul 25, 2025

Conversation

antonio-rojas
Copy link
Contributor

Sirocco is a C++ library. Without this, the library symbols are not mangled and the extension is unloadable:

sage: from sage.libs.sirocco import contpath
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
Cell In[1], line 1
----> 1 from sage.libs.sirocco import contpath

ImportError: /usr/lib/python3.13/site-packages/sage/libs/sirocco.cpython-313-x86_64-linux-gnu.so: undefined symbol: homotopyPath_mp_comps

@antonio-rojas antonio-rojas requested a review from tobiasdiez July 3, 2025 18:32
@tobiasdiez
Copy link
Contributor

LGTM, although I didn't experienced any issues with the sirocco conda package.

Just to make sure the CI stays green, is it okay for you that I wait with the positive review until #39944 is merged?

@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 3, 2025

Documentation preview for this PR (built with commit 785492f; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@antonio-rojas
Copy link
Contributor Author

LGTM, although I didn't experienced any issues with the sirocco conda package.

Just to make sure the CI stays green, is it okay for you that I wait with the positive review until #39944 is merged?

That is because the sirocco feature is disabled (quite likely because of this issue) so no sirocco tests are actually being run.

From the conda runners from #39944:

Features detected for doctesting: 4ti2,bliss,conway_polynomials,database_cremona_mini_ellcurve,database_ellcurves,database_graphs,fpylll,gap_package_atlasrep,gap_package_polenta,gap_package_polycyclic,gfan,info,lrcalc_python,lrslib,mpmath,nauty,networkx,numpy,palp,pexpect,pillow,polytopes_db,pplpy,primecountpy,ptyprocess,pycosat,pyparsing,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.homfly,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.schemes,sage.symbolic,scipy,singular,sphinx,symengine_py,sympy

So I'd say this should be merged first, to make sure sirocco is properly tested in #39944

@tobiasdiez
Copy link
Contributor

Ahh thanks, then it would have been indeed better to first merge this PR here. By now reality caught up and Volker already merged #39944. Could you please update this PR - hopefully it just works with the updated conda env.

@antonio-rojas
Build sirocco extension with C++
6b5d2d9 
Sirocco is a C++ library. Without this, the library symbols are not mangled and the extension is unloadable:

```
sage: from sage.libs.sirocco import contpath
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
Cell In[1], line 1
----> 1 from sage.libs.sirocco import contpath

ImportError: /usr/lib/python3.13/site-packages/sage/libs/sirocco.cpython-313-x86_64-linux-gnu.so: undefined symbol: homotopyPath_mp_comps
```
@antonio-rojas
Copy link
Contributor Author

All good now

Features detected for doctesting: 4ti2,bliss,conway_polynomials,database_cremona_mini_ellcurve,database_ellcurves,database_graphs,fpylll,gap_package_atlasrep,gap_package_polenta,gap_package_polycyclic,gfan,info,lrcalc_python,lrslib,mpmath,nauty,networkx,numpy,palp,pexpect,pillow,polytopes_db,pplpy,primecountpy,ptyprocess,pycosat,pyparsing,sage.combinat,sage.geometry.polyhedron,sage.graphs,sage.groups,sage.libs.braiding,sage.libs.ecl,sage.libs.flint,sage.libs.gap,sage.libs.homfly,sage.libs.linbox,sage.libs.m4ri,sage.libs.ntl,sage.libs.pari,sage.libs.singular,sage.misc.cython,sage.modular,sage.modules,sage.numerical.mip,sage.plot,sage.rings.complex_double,sage.rings.finite_rings,sage.rings.function_field,sage.rings.number_field,sage.rings.padics,sage.rings.polynomial.pbori,sage.rings.real_double,sage.rings.real_mpfr,sage.schemes,sage.symbolic,scipy,singular,sirocco,sphinx,symengine_py,sympy

tobiasdiez
tobiasdiez approved these changes Jul 6, 2025
View reviewed changes
Copy link
Contributor

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 14, 2025
05b7b7c 
sagemathgh-40369: Build sirocco extension with C++
    
Sirocco is a C++ library. Without this, the library symbols are not
mangled and the extension is unloadable:

```
sage: from sage.libs.sirocco import contpath
------------------------------------------------------------------------
---
ImportError                               Traceback (most recent call
last)
Cell In[1], line 1
----> 1 from sage.libs.sirocco import contpath

ImportError: /usr/lib/python3.13/site-
packages/sage/libs/sirocco.cpython-313-x86_64-linux-gnu.so: undefined
symbol: homotopyPath_mp_comps
```
    
URL: sagemath#40369
Reported by: Antonio Rojas
Reviewer(s): Tobias Diez
@vbraun vbraun merged commit 80c979e into sagemath:develop Jul 25, 2025
21 of 26 checks passed
@antonio-rojas antonio-rojas deleted the sirocco-c++ branch July 25, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@tobiasdiez tobiasdiez tobiasdiez approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
p: critical / 2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antonio-rojas @tobiasdiez @vbraun