Opened 15 months ago

Closed 14 months ago

Last modified 12 months ago

#29290 closed defect (fixed)

sage.schemes.elliptic_curves.mod_sym_num breaks Cygwin build (9.1.beta6)

Reported by: gh-darijgr Owned by:
Priority: blocker Milestone: sage-9.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:

Status badges

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):

[sagelib-9.1.beta6] [467/509] gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wno-unused -I./sage/cpython -I/home/skraeling/sage/local/lib/python3.7/site-packages/cypari2 -I./sage/libs/ntl -I/home/skraeling/sage/local/lib/python3.7/site-packages/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/site-packages/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.cygwin-3.0.7-x86_64-3.7/build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.o -fno-strict-aliasing -DCYTHON_CLINE_IN_TRACEBACK=1 -std=gnu99
[sagelib-9.1.beta6] In file included from /home/skraeling/sage/local/include/python3.7m/unicodeobject.h:58:0,
[sagelib-9.1.beta6]                  from /home/skraeling/sage/local/include/python3.7m/Python.h:99,
[sagelib-9.1.beta6]                  from build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.c:51:
[sagelib-9.1.beta6] build/cythonized/sage/schemes/elliptic_curves/mod_sym_num.c:1838:64: error: expected identifier or ‘(’ before numeric constant
[sagelib-9.1.beta6]    __pyx_t_4sage_7schemes_15elliptic_curves_11mod_sym_num_llong _N;
[sagelib-9.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 15 months ago by chapoton

  • Cc embray added

comment:2 Changed 15 months ago by gh-darijgr

Still broken on beta7.

comment:3 Changed 14 months ago by mkoeppe

  • Priority changed from major to blocker

comment:4 Changed 14 months ago by mkoeppe

  • Cc dimpase fbissey added

comment:5 Changed 14 months ago by mkoeppe

  • Cc wuthrich added

comment:6 Changed 14 months ago by mkoeppe

  • Branch set to u/mkoeppe/sage_schemes_elliptic_curves_mod_sym_num_breaks_cygwin_build__9_1_beta6_

comment:7 Changed 14 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to 11078b571fee12efff3cd6dee5610b828511feeb
  • Status changed from new to needs_review

New commits:

11078b5src/sage/schemes/elliptic_curves.mod_sym_num.pyx: Rename _N to fix identifier clash with a C macro

comment:8 Changed 14 months ago by fbissey

I have to ask, why _N4711?

comment:9 follow-up: Changed 14 months ago by mkoeppe

to mask the odor

comment:10 in reply to: ↑ 9 Changed 14 months ago by fbissey

Replying to mkoeppe:

to mask the odor

It sure does smell, but I may miss some context if there is an allusion.

comment:11 follow-up: Changed 14 months ago by 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.

comment:12 in reply to: ↑ 11 Changed 14 months ago by dimpase

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 follow-up: Changed 14 months ago by wuthrich

  • 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:

5ff09abtrac #29290: Rename _N to fix identifier clash with a C macro in cygwin

comment:14 follow-up: Changed 14 months ago by dimpase

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 14 months ago by mkoeppe

Replying to wuthrich:

I change the names _N to something meaningful.

Thanks! I was hoping for that.

comment:16 in reply to: ↑ 14 Changed 14 months ago by wuthrich

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 14 months ago by mkoeppe

There's a stray comma at the end of a line in the patch

comment:18 Changed 14 months ago by mkoeppe

  • Branch changed from u/wuthrich/trac_29290 to u/mkoeppe/trac_29290

comment:19 Changed 14 months ago by mkoeppe

  • Authors changed from Matthias Koeppe to Matthias Koeppe, Christian Wuthrich
  • Commit changed from 5ff09ab2c52ce77cc87f5a36e3cc17ca432df865 to 7b36d39d17299b3471e0cf381b9702fe3e474031

New commits:

7b36d39Remove trailing comma

comment:20 Changed 14 months ago by mkoeppe

  • Reviewers set to Christian Wuthrich, Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:21 Changed 14 months ago by vbraun

  • Branch changed from u/mkoeppe/trac_29290 to 7b36d39d17299b3471e0cf381b9702fe3e474031
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 Changed 12 months ago by mkoeppe

  • Authors changed from Matthias Koeppe, Christian Wuthrich to Matthias Koeppe, Chris Wuthrich
  • Commit 7b36d39d17299b3471e0cf381b9702fe3e474031 deleted
  • Reviewers changed from Christian Wuthrich, Matthias Koeppe to Chris Wuthrich, Matthias Koeppe
Note: See TracTickets for help on using tickets.