Opened 6 years ago

Closed 6 years ago

#17591 closed enhancement (fixed)

remove gdmodule

Reported by: kcrisman Owned by:
Priority: minor Milestone: sage-6.7
Component: packages: standard Keywords:
Cc: jdemeyer Merged in:
Authors: R. Andrew Ohana Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: a12db77 (Commits) Commit: a12db779288e6a65fa1cf95f18e19ed84058be1b
Dependencies: Stopgaps:

Description

Separated from #15593.

Change History (18)

comment:1 Changed 6 years ago by tmonteil

  • Branch set to u/ohanar/optionalize
  • Commit set to d701c15354193da3ba02762ca61e6763a74b6b40

Just pointing to the branch from #15593 since most of the work is about gdmodule (the sqlalchemy part is straightforward).

comment:2 Changed 6 years ago by jdemeyer

  • Authors set to R. Andrew Ohana

comment:3 Changed 6 years ago by ohanar

  • Branch changed from u/ohanar/optionalize to u/ohanar/remove_gdmodule
  • Commit changed from d701c15354193da3ba02762ca61e6763a74b6b40 to 3e569b8a0ee56020cb6dc2861670f09081c8bccc

It is now rebased and just removes the package. All tests pass on my system.


New commits:

3e569b8remove gdmodule

comment:4 Changed 6 years ago by ohanar

  • Status changed from new to needs_review

comment:5 follow-up: Changed 6 years ago by kcrisman

  • Cc jdemeyer added

Just for the record, comment:4:ticket:15593 says

I dislike

fn = open(filename, 'wb') # check filename
fn.close()

cdef FILE *out = fopen(filename, 'wb')

gdImagePng(im, out)

fclose(out)

You can open the file using Python and access its FILE* with ​PyFile_AsFile.

Out of curiosity, is this just a Python module now instead of a separate thing?

Last edited 6 years ago by kcrisman (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 6 years ago by jdemeyer

You can open the file using Python and access its FILE* with ​PyFile_AsFile.

Robert replied that my suggestion wouldn't work in Python 3, so it's not good either. But still, I dislike

fn = open(filename, 'wb') # check filename
fn.close()

cdef FILE *out = fopen(filename, 'wb')

It would better be

cdef FILE *out = fopen(filename, 'wb')
if not FILE:
   raise ...

Out of curiosity, is this just a Python module now instead of a separate thing?

I don't understand this question.

comment:7 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to needs_work

I think that src/sage/ext/gd.pxi should be moved to src/sage/libs/gd.pxd and possibly src/sage/libs/gd.pyx for the wrapper functions.

Last edited 6 years ago by jdemeyer (previous) (diff)

comment:8 Changed 6 years ago by kcrisman

I don't understand this question.

I was just curious why this was no longer necessary - apparently gd is still used, but directly now, I guess. Why did we use gdmodule in the first place?

comment:9 Changed 6 years ago by git

  • Commit changed from 3e569b8a0ee56020cb6dc2861670f09081c8bccc to a5978146023852ed5d415ed284c82558f8698b15

Branch pushed to git repo; I updated commit sha1. New commits:

a597814move gd.pxi -> gd.pxd

comment:10 Changed 6 years ago by ohanar

  • Status changed from needs_work to needs_review
  • Summary changed from remove gdmodule as standard package to remove gdmodule

I agree that the wrapper functions aren't ideal, but I didn't want to spend that much time on it. Part of the motivation of this ticket is to work towards python 3 compatibility, since when I did my evaluation on which sage dependencies weren't compatible with python 3, gdmodule was one of the few libraries that didn't have python 3 support (that may have changed since then, haven't looked into it).

comment:11 Changed 6 years ago by aapitzsch

  • Status changed from needs_review to needs_work

Remove the references to gdmodule from COPYING.txt.

comment:12 Changed 6 years ago by git

  • Commit changed from a5978146023852ed5d415ed284c82558f8698b15 to 6c243b06d8d61a9079f4e968d74d889ca786ec0f

Branch pushed to git repo; I updated commit sha1. New commits:

6c243b0remove gdmodule from COPYING.txt

comment:13 Changed 6 years ago by ohanar

  • Status changed from needs_work to needs_review

comment:14 Changed 6 years ago by git

  • Commit changed from 6c243b06d8d61a9079f4e968d74d889ca786ec0f to a12db779288e6a65fa1cf95f18e19ed84058be1b

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a12db77remove gdmodule

comment:15 Changed 6 years ago by ohanar

This should be much simpler to review now - all our previous usage of gdmodule was removed in one of the recent display manager changes.

comment:16 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-6.5 to sage-6.7
  • Reviewers set to Jeroen Demeyer

comment:17 Changed 6 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:18 Changed 6 years ago by vbraun

  • Branch changed from u/ohanar/remove_gdmodule to a12db779288e6a65fa1cf95f18e19ed84058be1b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.