#29290 closed defect (fixed)
sage.schemes.elliptic_curves.mod_sym_num breaks Cygwin build (9.1.beta6)
Reported by:  ghdarijgr  Owned by:  

Priority:  blocker  Milestone:  sage9.1 
Component:  elliptic curves  Keywords:  cython, elliptic curves, schemes 
Cc:  embray, dimpase, fbissey, wuthrich  Merged in:  
Authors:  Matthias Koeppe, Chris Wuthrich  Reviewers:  Chris Wuthrich, Matthias Koeppe 
Report Upstream:  N/A  Work issues:  
Branch:  7b36d39 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
Installing sage 9.1.beta6 on Cygwin currently chokes on sage/schemes/elliptic_curves/mod_sym_num.c (which comes from sage/schemes/elliptic_curves/mod_sym_num.pyx):
[sagelib9.1.beta6] [467/509] gcc Wnounusedresult Wsigncompare DNDEBUG g fwrapv O3 Wall Wnounused I./sage/cpython I/home/skraeling/sage/local/lib/python3.7/sitepackages/cypari2 I./sage/libs/ntl I/home/skraeling/sage/local/lib/python3.7/sitepackages/cysignals I/home/skraeling/sage/local/include I/home/skraeling/sage/src I/home/skraeling/sage/src/sage/ext I/home/skraeling/sage/local/include/python3.7m I/home/skraeling/sage/local/lib/python3.7/sitepackages/numpy/core/include Ibuild/cythonized I/home/skraeling/sage/local/include/python3.7m c build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.c o build/temp.cygwin3.0.7x86_643.7/build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.o fnostrictaliasing DCYTHON_CLINE_IN_TRACEBACK=1 std=gnu99 [sagelib9.1.beta6] In file included from /home/skraeling/sage/local/include/python3.7m/unicodeobject.h:58:0, [sagelib9.1.beta6] from /home/skraeling/sage/local/include/python3.7m/Python.h:99, [sagelib9.1.beta6] from build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.c:51: [sagelib9.1.beta6] build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.c:1838:64: error: expected identifier or â€˜(â€™ before numeric constant [sagelib9.1.beta6] __pyx_t_4sage_7schemes_15elliptic_curves_11mod_sym_num_llong _N; [sagelib9.1.beta6] ^
... and tons and tons of other errors that all seem to come from _N
attributes in that pyx file.
I don't understand C well enough to trace this back to its origins, but it looks like a clash of variable names.
Change History (22)
comment:1 Changed 2 years ago by
 Cc embray added
comment:2 Changed 2 years ago by
comment:3 Changed 2 years ago by
 Priority changed from major to blocker
comment:4 Changed 2 years ago by
 Cc dimpase fbissey added
comment:5 Changed 2 years ago by
 Cc wuthrich added
comment:6 Changed 2 years ago by
 Branch set to u/mkoeppe/sage_schemes_elliptic_curves_mod_sym_num_breaks_cygwin_build__9_1_beta6_
comment:7 Changed 2 years ago by
 Commit set to 11078b571fee12efff3cd6dee5610b828511feeb
 Status changed from new to needs_review
New commits:
11078b5  src/sage/schemes/elliptic_curves.mod_sym_num.pyx: Rename _N to fix identifier clash with a C macro

comment:8 Changed 2 years ago by
I have to ask, why _N4711
?
comment:9 followup: ↓ 10 Changed 2 years ago by
to mask the odor
comment:10 in reply to: ↑ 9 Changed 2 years ago by
Replying to mkoeppe:
to mask the odor
It sure does smell, but I may miss some context if there is an allusion.
comment:11 followup: ↓ 12 Changed 2 years ago by
I would really prefer if that is _N_E
or _conductor
or _NE
rather than the very confusing _N4711
.
I don't think I can review the patch as I don't have cygwin.
comment:12 in reply to: ↑ 11 Changed 2 years ago by
Replying to wuthrich:
I would really prefer if that is
_N_E
or_conductor
or_NE
rather than the very confusing_N4711
.I don't think I can review the patch as I don't have cygwin.
all you need to build on cygwin is a github account, if you followed the latest trends.
comment:13 followup: ↓ 15 Changed 2 years ago by
 Branch changed from u/mkoeppe/sage_schemes_elliptic_curves_mod_sym_num_breaks_cygwin_build__9_1_beta6_ to u/wuthrich/trac_29290
 Commit changed from 11078b571fee12efff3cd6dee5610b828511feeb to 5ff09ab2c52ce77cc87f5a36e3cc17ca432df865
I change the names _N to something meaningful.
New commits:
5ff09ab  trac #29290: Rename _N to fix identifier clash with a C macro in cygwin

comment:14 followup: ↓ 16 Changed 2 years ago by
here https://git.sagemath.org/sage.git/tree/src/doc/en/developer/portability_testing.rst?id=1f7e16a72e39fb0a154cfb3d4eec1c3cbe9877da#n831 (it's already merged in the latest develop) you can read how to run tests on Cygwin etc via GitHub.
Basically, you need a fork of sage repo on GitHub, then you can make a pull request to the develop branch of your fork from a branch incorporating this ticket, and then in about 12 hours you get the results with tests etc.
comment:15 in reply to: ↑ 13 Changed 2 years ago by
comment:16 in reply to: ↑ 14 Changed 2 years ago by
Basically, you need a fork of sage repo on GitHub, then you can make a pull request to the develop branch of your fork from a branch incorporating this ticket, and then in about 12 hours you get the results with tests etc.
Thanks for the explanation, but I really need to focus the little time I have left on different things than this. Sorry.
comment:17 Changed 2 years ago by
There's a stray comma at the end of a line in the patch
comment:18 Changed 2 years ago by
 Branch changed from u/wuthrich/trac_29290 to u/mkoeppe/trac_29290
comment:19 Changed 2 years ago by
 Commit changed from 5ff09ab2c52ce77cc87f5a36e3cc17ca432df865 to 7b36d39d17299b3471e0cf381b9702fe3e474031
New commits:
7b36d39  Remove trailing comma

comment:20 Changed 2 years ago by
 Reviewers set to Christian Wuthrich, Matthias Koeppe
 Status changed from needs_review to positive_review
comment:21 Changed 2 years ago by
 Branch changed from u/mkoeppe/trac_29290 to 7b36d39d17299b3471e0cf381b9702fe3e474031
 Resolution set to fixed
 Status changed from positive_review to closed
comment:22 Changed 2 years ago by
 Commit 7b36d39d17299b3471e0cf381b9702fe3e474031 deleted
 Reviewers changed from Christian Wuthrich, Matthias Koeppe to Chris Wuthrich, Matthias Koeppe
Still broken on beta7.